Skip to content

Command Spy Persistence #2040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: devel
Choose a base branch
from
Open

Command Spy Persistence #2040

wants to merge 11 commits into from

Conversation

Wild1145
Copy link
Member

This resolves #785

{
FPlayer playerdata = plugin.pl.getPlayer(event.getPlayer());
playerdata.setCommandSpy(playerdata.cmdspyEnabled() == true);
} else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting.

@@ -108,6 +113,10 @@ public boolean hasLoginMessage()
return loginMessage != null && !loginMessage.isEmpty();
}

public boolean commandSpyIsActive{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing ()'s and { must be on next line.

FPlayer playerdata = plugin.pl.getPlayer(event.getPlayer());
playerdata.setCommandSpy(playerdata.cmdspyEnabled() == false);
}
} else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting.

}
}

public boolean setCommandSpyActive(boolean status, Player player){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ on next line (formatting).

@@ -108,6 +113,10 @@ public boolean hasLoginMessage()
return loginMessage != null && !loginMessage.isEmpty();
}

public boolean commandSpyIsActive{
return commandSpyStatus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing semicolon.

{
admin.commandSpyIsActive = true;
}
else{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ on next line (formatting).

if (playerdata.cmdspyEnabled() == true)
{
plugin.al.setCommandSpyActive(true, playerSender);
} else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else on next line.

@Wild1145
Copy link
Member Author

This is why you dont switch between IDE's... Whoops

public boolean setCommandSpyActive(boolean status, Player player){
final Admin admin = getAdmin(player);

if (status == true)
Copy link
Member

@lemonsked lemonsked May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just do if(status)

@@ -17,6 +17,14 @@ public boolean run(CommandSender sender, Player playerSender, Command cmd, Strin

FPlayer playerdata = plugin.pl.getPlayer(playerSender);
playerdata.setCommandSpy(!playerdata.cmdspyEnabled());
if (playerdata.cmdspyEnabled() == true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== true is unneeded

@JeromSar JeromSar changed the title Command Spy Persistace Command Spy Persistence May 31, 2017
@Wild1145
Copy link
Member Author

@OxLemonxO There we are, mind taking a look for me now?

@@ -108,6 +115,12 @@ public boolean hasLoginMessage()
return loginMessage != null && !loginMessage.isEmpty();
}

public boolean commandSpyIsActive()

Copy link
Member

@lemonsked lemonsked May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra newline for no reason

@@ -96,6 +102,7 @@ public void saveTo(ConfigurationSection cs)
cs.set("ips", Lists.newArrayList(ips));
cs.set("last_login", FUtil.dateToString(lastLogin));
cs.set("login_message", loginMessage);
cs.set("commandspy_isactive", commandSpyStatus);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just commandspy?

@@ -108,6 +115,12 @@ public boolean hasLoginMessage()
return loginMessage != null && !loginMessage.isEmpty();
}

public boolean commandSpyIsActive()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary newline.

@@ -108,6 +115,12 @@ public boolean hasLoginMessage()
return loginMessage != null && !loginMessage.isEmpty();
}

public boolean commandSpyIsActive()
Copy link
Member

@JeromSar JeromSar May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be hasCommandSpy()

@@ -38,6 +40,9 @@
@Getter
@Setter
private String loginMessage = null;
@Getter
@Setter
private boolean commandSpyStatus = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just commandSpy?

@@ -238,6 +240,34 @@ public boolean isAdminImpostor(Player player)
return getEntryByName(player.getName()) != null && !isAdmin(player);
}

public boolean isCommandSpyActive(Player player)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method does to little. It is redundant and all dependent operations should probably inline any operations.

}
}

public boolean setCommandSpyActive(boolean status, Player player)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted above, this method does too little.

@Wild1145
Copy link
Member Author

@JeromSar Mind taking a look, I think I've found the best way now.

@@ -15,15 +16,17 @@
public boolean run(CommandSender sender, Player playerSender, Command cmd, String commandLabel, String[] args, boolean senderIsConsole)
{

Admin admin = getAdmin((Player) sender);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use playerSender

@JeromSar JeromSar changed the base branch from tfm5.1-mc1.12 to devel June 2, 2017 13:28
@ghost
Copy link

ghost commented Jun 5, 2017

This is completely uneeded code? FPlayer data is stored per server session not player session now ( this isn't on the github)

@mayokek
Copy link

mayokek commented Jun 5, 2017

@commodore64x, then make an issue about it and a PR as all changes should be updated on GitHub

Admin admin = getAdmin((Player) event.getPlayer());
FPlayer playerdata = plugin.pl.getPlayer(event.getPlayer());

if (plugin.al.isAdmin(event.getPlayer()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire block should be changed to something like:

boolean admin = plugin.al.isAdmin(event.getPlayer();
playerdata.setCommandSpy(admin && admin.hasCommandSpy());

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given I'm struggling to understand what that even does... I will leave the PR as is for now, its already broken and needs more work, but I can't see what I've screwed up.

{
if (admin.hasCommandSpy())
{
playerdata.setCommandSpy(playerdata.cmdspyEnabled());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement does nothing. Possibly a typo.

@@ -38,6 +40,9 @@
@Getter
@Setter
private String loginMessage = null;
@Getter
@Setter
private Boolean commandSpy = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a boolean.

@@ -108,6 +115,16 @@ public boolean hasLoginMessage()
return loginMessage != null && !loginMessage.isEmpty();
}

public Boolean hasCommandSpy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commandSpy does not need getters/setters. We have lombok for that.

@@ -147,4 +164,4 @@ public boolean isValid()
&& !ips.isEmpty()
&& lastLogin != null;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline.

@Wild1145
Copy link
Member Author

Wild1145 commented Jun 6, 2017

I'll pull this into Netbeans soon and see whats gone wrong, IntelliJ hasnt really done a great job when I was at work by the looks of it.

As for lombok, I've never heard of that before, so might not be so helpful there.

@lemonsked
Copy link
Member

Instead of making a new getter for commandspy, the annotation @Getter already created one.
You can use isCommandSpy

@ghost
Copy link

ghost commented Aug 14, 2017

This isn't actually needed, there is a one line code change that will fix a vast amount of issues.

@Wild1145
Copy link
Member Author

@commodore64x It however is not implemented at the current time. Until such a time that it is, this does resolve a valid and approved issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make cmdspy remember if an admin toggled itself
4 participants