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

Already on GitHub? Sign in to your account

[C+B] Add a Scoreboard API, adds BUKKIT-3776 #794

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants

@tonybruess tonybruess referenced this pull request in Bukkit/CraftBukkit Mar 16, 2013

Closed

[C+B] Add a Scoreboard API, adds BUKKIT-3776 #1058

@natemort natemort and 2 others commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
@@ -0,0 +1,143 @@
+package org.bukkit.scoreboard;
+
+import org.apache.commons.lang.Validate;
+import org.bukkit.OfflinePlayer;
+
+public interface Objective {
+ static enum Criteria {
+ PLAYER_KILL_COUNT("playerKillCount"),
+ TOTAL_KILL_COUNT("totalKillCount"),
+ DUMMY("dummy"),
+ HEALTH("health"),
+ DEATH_COUNT("deathCount");
+
+ private final String criteria;
@natemort

natemort Mar 16, 2013

Member

This is an implementational detail, and should not be in Bukkit.

@tonybruess

tonybruess Mar 16, 2013

How would you recommend moving it into CraftBukkit? I thought about it for a long time and couldn't figure out the best way to do so.

@natemort

natemort Mar 16, 2013

Member

Static methods within a wrapper class would work, perhaps inside of CraftObjective.

@amaranth

amaranth Mar 18, 2013

Contributor

Look at how sounds are handled for an example of how to do this. The PR for particles also works like sounds if you want to see an exact diff of how to do it.

@natemort natemort and 1 other commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
+ return value;
+ }
+ }
+
+ throw new IllegalArgumentException("No valid criteria found for " + criteria);
+ }
+ }
+
+ static enum Display {
+ NONE(null, null),
+ LIST("list", 0),
+ SIDEBAR("sidebar", 1),
+ BELOW_NAME("belowName", 2);
+
+ private final String display;
+ private final Integer position;
@natemort

natemort Mar 16, 2013

Member

Both display and position are implementational, and should not be in Bukkit.

@tonybruess

tonybruess Mar 16, 2013

See my comment on the Criteria enum.

@natemort natemort commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
+
+ public static Display fromInt(int position) {
+ Validate.notNull(position);
+
+ for(Display value : Display.values()) {
+ if(position == value.position.intValue()) {
+ return value;
+ }
+ }
+
+ throw new IllegalArgumentException("No valid display found for " + position);
+ }
+ }
+
+ /*
+ * Get the objective's name
@natemort

natemort Mar 16, 2013

Member

"Get" should be "Gets". This applies to all your javadocs. Also, add periods to the end.

@natemort natemort and 3 others commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+public interface Scoreboard {
+ /**
+ * Create a team
+ *
+ * @param name Team's new name, can not be null
+ * @param displayName Team's display name, can be null
+ * @return The newly created team, if successful
+ * @throws IllegalArgumentException if name/display name is too long or the team already exists
+ */
+ public Team createTeam(String name, String displayName);
+
+ /**
+ * Get a team by name
+ *
+ * @param name Team's name
+ * @return The team that was requested, can be null
@natemort

natemort Mar 16, 2013

Member

When can it be null? Simply stating that it can be null is incredibly vague.

@tonybruess

tonybruess Mar 16, 2013

If you pass a team name and the team doesn't exist, it will return null. Seems straight foward to me.

@natemort

natemort Mar 16, 2013

Member

Document it then.

@tonybruess

tonybruess Mar 16, 2013

Pardon me, but how is it not straight forward that if you pass a team name that doesn't exist you'll get null with the current documentation?

@lol768

lol768 Mar 17, 2013

Agreed, it's overly vague. "Can be null" - when, why?

@riking

riking Mar 18, 2013

Contributor
    /**
     * Get a team by name
     *
     * @param name Team's name
     * @return The team that was requested, or null if not found
     */

@natemort natemort commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+ * @param player Player that's getting a team, can not be null
+ * @param team Player's new team, can be null to remove player from a team
+ */
+ public void setTeam(OfflinePlayer player, Team team);
+
+ /**
+ * Remove a team from the scoreboard
+ *
+ * @param team Team to remove, can not be null
+ */
+ public void removeTeam(Team team);
+
+ /**
+ * Create an objective
+ *
+ * @param name Name to create, can not be null
@natemort

natemort Mar 16, 2013

Member

"Name to create". So are we creating names, or Objectives?

@natemort natemort commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+ */
+ public void setTeam(OfflinePlayer player, Team team);
+
+ /**
+ * Remove a team from the scoreboard
+ *
+ * @param team Team to remove, can not be null
+ */
+ public void removeTeam(Team team);
+
+ /**
+ * Create an objective
+ *
+ * @param name Name to create, can not be null
+ * @param criteria Criteria of the objective, can not be null
+ * @param displayName Display name, can be null
@natemort

natemort Mar 16, 2013

Member

"Display name of the Objective, can be null."

@natemort natemort commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+
+ /**
+ * Remove a team from the scoreboard
+ *
+ * @param team Team to remove, can not be null
+ */
+ public void removeTeam(Team team);
+
+ /**
+ * Create an objective
+ *
+ * @param name Name to create, can not be null
+ * @param criteria Criteria of the objective, can not be null
+ * @param displayName Display name, can be null
+ * @return The new objective that was created
+ * @throws IllegalArgumentException if objective exists or name is too long or display name is too long
@natemort

natemort Mar 16, 2013

Member

"If an objective with this name already exists, the name is too long, or the display name is too long."

@natemort natemort and 2 others commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+ /**
+ * Create an objective
+ *
+ * @param name Name to create, can not be null
+ * @param criteria Criteria of the objective, can not be null
+ * @param displayName Display name, can be null
+ * @return The new objective that was created
+ * @throws IllegalArgumentException if objective exists or name is too long or display name is too long
+ */
+ public Objective createObjective(String name, Objective.Criteria criteria, String displayName);
+
+ /**
+ * Get an objective by name
+ *
+ * @param name Name of objective
+ * @return The objective, can be null
@natemort

natemort Mar 16, 2013

Member

When can it be null?

@tonybruess

tonybruess Mar 16, 2013

When you pass an objective name and it doesn't exist...

@natemort

natemort Mar 16, 2013

Member

Document it then.

@tonybruess

tonybruess Mar 16, 2013

How is it not clear?

@riking

riking Mar 18, 2013

Contributor
/**
 * Get an objective by name
 *
 * @param name Name of objective
 * @return The objective, or null if not found
 */

@natemort natemort commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Team.java
+ /*
+ * Set the team's suffix
+ *
+ * @param suffix New suffix, can not be null
+ */
+ public void setSuffix(String suffix);
+
+ /*
+ * Get whether or not the team can hurt members
+ *
+ * @return true if friendly fire is on, false otherwise
+ */
+ public boolean canFriendlyFire();
+
+ /*
+ * Set whether or not the team can hurt members
@natemort

natemort Mar 16, 2013

Member

"Sets whether or not the team can hurt other members of this team."

@natemort natemort commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Team.java
+ /*
+ * Get whether or not the team can hurt members
+ *
+ * @return true if friendly fire is on, false otherwise
+ */
+ public boolean canFriendlyFire();
+
+ /*
+ * Set whether or not the team can hurt members
+ *
+ * @param friendlyFire
+ */
+ public void setFriendlyFire(boolean friendlyFire);
+
+ /*
+ * Get whether or not the team can see invisible members
@natemort

natemort Mar 16, 2013

Member

"Gets whether or not members of this team can see other members of this team who are invisible."

@natemort natemort commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Team.java
+ /*
+ * Set whether or not the team can hurt members
+ *
+ * @param friendlyFire
+ */
+ public void setFriendlyFire(boolean friendlyFire);
+
+ /*
+ * Get whether or not the team can see invisible members
+ *
+ * @return true if team can see invisible members, false otherwise
+ */
+ public boolean canSeeFriendlyInvisibles();
+
+ /*
+ * Set if the team can see invisible members
@natemort

natemort Mar 16, 2013

Member

"Sets whether or not members of this team can see other members of this team who are invisible."

@natemort natemort commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
@@ -0,0 +1,143 @@
+package org.bukkit.scoreboard;
+
+import org.apache.commons.lang.Validate;
+import org.bukkit.OfflinePlayer;
+
+public interface Objective {
+ static enum Criteria {
@natemort

natemort Mar 16, 2013

Member

Declaring this as static is redundant, and doesn't follow how any other enums are declared within Bukkit.

@natemort natemort commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
+ }
+
+ public static Criteria fromString(String criteria) {
+ Validate.notNull(criteria);
+
+ for(Criteria value : Criteria.values()) {
+ if(criteria.equalsIgnoreCase(value.criteria)) {
+ return value;
+ }
+ }
+
+ throw new IllegalArgumentException("No valid criteria found for " + criteria);
+ }
+ }
+
+ static enum Display {
@natemort

natemort Mar 16, 2013

Member

Declaring this as static is redundant, and doesn't follow how any other enums are declared within Bukkit.

@natemort natemort and 1 other commented on an outdated diff Mar 16, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
@@ -0,0 +1,80 @@
+package org.bukkit.scoreboard;
+
+import java.util.Set;
+
+import org.bukkit.OfflinePlayer;
+
+public interface Scoreboard {
+ /**
+ * Create a team
+ *
+ * @param name Team's new name, can not be null
+ * @param displayName Team's display name, can be null
+ * @return The newly created team, if successful
@natemort

natemort Mar 16, 2013

Member

So, if it's not successful, then what?

@tonybruess

tonybruess Mar 16, 2013

An exception will be thrown before you get there

@natemort natemort commented on an outdated diff Mar 18, 2013

src/main/java/org/bukkit/Server.java
@@ -689,4 +690,11 @@
* @see ItemFactory
*/
ItemFactory getItemFactory();
+
+ /**
+ * Get the server's scoreboard
@natemort

natemort Mar 18, 2013

Member

"Get" should be "Gets", as per the Oracle standard on Javadocs.

@natemort natemort commented on an outdated diff Mar 18, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
+ }
+ }
+
+ throw new IllegalArgumentException("No valid display found for " + position);
+ }
+ }
+
+ /*
+ * Gets the objective's name
+ *
+ * @return The team's name
+ */
+ public String getName();
+
+ /*
+ * Get the objective's display name
@natemort

natemort Mar 18, 2013

Member

Should be "Gets"

@natemort natemort commented on an outdated diff Mar 18, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
+ /*
+ * Gets the objective's name
+ *
+ * @return The team's name
+ */
+ public String getName();
+
+ /*
+ * Get the objective's display name
+ *
+ * @return The objective's display name, can be null
+ */
+ public String getDisplayName();
+
+ /*
+ * Set the objective's display name
@natemort

natemort Mar 18, 2013

Member

Should be "Sets"

@natemort natemort commented on an outdated diff Mar 18, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
+ *
+ * @param player Player to get a score for, can not be null
+ * @return Player's score
+ */
+ public int getScore(OfflinePlayer player);
+
+ /*
+ * Set the score for a player
+ *
+ * @param player Player to set a score for, can not be null
+ * @param score New score
+ */
+ public void setScore(OfflinePlayer player, int score);
+
+ /*
+ * Remove the score for a player
@natemort

natemort Mar 18, 2013

Member

Should be "Removes".

@natemort natemort commented on an outdated diff Mar 18, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+ * Get a team by name
+ *
+ * @param name Team's name
+ * @return The team that was requested, can be null
+ */
+ public Team getTeam(String name);
+
+ /**
+ * Gets all teams
+ *
+ * @return All the teams
+ */
+ public Set<Team> getTeams();
+
+ /**
+ * Put a player on a team
@natemort

natemort Mar 18, 2013

Member

Should be "Puts".

@natemort natemort commented on an outdated diff Mar 18, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+ * Gets all teams
+ *
+ * @return All the teams
+ */
+ public Set<Team> getTeams();
+
+ /**
+ * Put a player on a team
+ *
+ * @param player Player that's getting a team, can not be null
+ * @param team Player's new team, can be null to remove player from a team
+ */
+ public void setTeam(OfflinePlayer player, Team team);
+
+ /**
+ * Remove a team from the scoreboard
@natemort

natemort Mar 18, 2013

Member

Should be "Removes".

@natemort natemort commented on an outdated diff Mar 18, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+ * Put a player on a team
+ *
+ * @param player Player that's getting a team, can not be null
+ * @param team Player's new team, can be null to remove player from a team
+ */
+ public void setTeam(OfflinePlayer player, Team team);
+
+ /**
+ * Remove a team from the scoreboard
+ *
+ * @param team Team to remove, can not be null
+ */
+ public void removeTeam(Team team);
+
+ /**
+ * Create an objective
@natemort

natemort Mar 18, 2013

Member

Should be "Creates".

I'll have time to update this tomorrow.

Updated, fixed some javadoc misshaps, working on moving some implementation details out of Bukkit and adding per-player scoreboards.

Implementation details moved out of Bukkit.

@Anxuiz Anxuiz and 1 other commented on an outdated diff Mar 23, 2013

src/main/java/org/bukkit/Server.java
@@ -1,11 +1,7 @@
package org.bukkit;
import java.io.File;
-import java.util.Iterator;
@Anxuiz

Anxuiz Mar 23, 2013

Unnecessary diffs

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
@@ -0,0 +1,85 @@
+package org.bukkit.scoreboard;
+
+import org.bukkit.OfflinePlayer;
+
+public interface Objective {
@Anxuiz

Anxuiz Mar 23, 2013

What is an Objective? This interface needs javadoc description.

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
+ private final boolean readOnly;
+
+ Criteria() {
+ this.readOnly = false;
+ }
+
+ Criteria(boolean readOnly) {
+ this.readOnly = readOnly;
+ }
+
+ public boolean isReadOnly() {
+ return this.readOnly;
+ }
+ }
+
+ enum Display {
@Anxuiz

Anxuiz Mar 23, 2013

Javadoc documentation would be nice

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
+ public boolean isReadOnly() {
+ return this.readOnly;
+ }
+ }
+
+ enum Display {
+ NONE,
+ LIST,
+ SIDEBAR,
+ BELOW_NAME;
+ }
+
+ /*
+ * Gets the objective's name
+ *
+ * @return The team's name

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Objective.java
+ enum Display {
+ NONE,
+ LIST,
+ SIDEBAR,
+ BELOW_NAME;
+ }
+
+ /*
+ * Gets the objective's name
+ *
+ * @return The team's name
+ */
+ public String getName();
+
+ /*
+ * Gets the objective's display name
@Anxuiz

Anxuiz Mar 23, 2013

What is the difference between display name and name?

@Technius

Technius Mar 25, 2013

It's an optional display name that is displayed instead of the objective's name.

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
@@ -0,0 +1,87 @@
+package org.bukkit.scoreboard;
+
+import java.util.Set;
+
+import org.bukkit.OfflinePlayer;
+
+public interface Scoreboard {
@Anxuiz

Anxuiz Mar 23, 2013

What is a Scoreboard? Interface needs documentation

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+package org.bukkit.scoreboard;
+
+import java.util.Set;
+
+import org.bukkit.OfflinePlayer;
+
+public interface Scoreboard {
+ /**
+ * Creates a team
+ *
+ * @param name Team's new name, can not be null
+ * @param displayName Team's display name, can be null
+ * @return The newly created team, if successful
+ * @throws IllegalArgumentException if name/display name is too long or the team already exists
+ */
+ public Team createTeam(String name, String displayName);
@Anxuiz

Anxuiz Mar 23, 2013

I suggest an overload for no display name instead of specifying null.

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+
+import org.bukkit.OfflinePlayer;
+
+public interface Scoreboard {
+ /**
+ * Creates a team
+ *
+ * @param name Team's new name, can not be null
+ * @param displayName Team's display name, can be null
+ * @return The newly created team, if successful
+ * @throws IllegalArgumentException if name/display name is too long or the team already exists
+ */
+ public Team createTeam(String name, String displayName);
+
+ /**
+ * Gets a team by name
@Anxuiz

Anxuiz Mar 23, 2013

Exact match? Case sensitive?

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+ public Team getTeam(String name);
+
+ /**
+ * Gets all teams
+ *
+ * @return All the teams
+ */
+ public Set<Team> getTeams();
+
+ /**
+ * Puts a player on a team
+ *
+ * @param player Player that's getting a team, can not be null
+ * @param team Player's new team, can be null to remove player from a team
+ */
+ public void setTeam(OfflinePlayer player, Team team);
@Anxuiz

Anxuiz Mar 23, 2013

Maybe setPlayerTeam instead?

@Anxuiz

Anxuiz Mar 23, 2013

Also would be good to add removePlayerTeam instead of null for team.

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+
+ /**
+ * Removes a team from the scoreboard
+ *
+ * @param team Team to remove, can not be null
+ */
+ public void removeTeam(Team team);
+
+ /**
+ * Creates an objective
+ *
+ * @param name Name of the objective that will be created, can not be null
+ * @param criteria Criteria of the objective, can not be null
+ * @param displayName Display name of the objective, can be null
+ * @return The new objective that was created
+ * @throws IllegalArgumentException if an objective with this name already exists, the name is too long, or the display name is too long
@Anxuiz

Anxuiz Mar 23, 2013

What is "too long"?

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+ */
+ public void removeTeam(Team team);
+
+ /**
+ * Creates an objective
+ *
+ * @param name Name of the objective that will be created, can not be null
+ * @param criteria Criteria of the objective, can not be null
+ * @param displayName Display name of the objective, can be null
+ * @return The new objective that was created
+ * @throws IllegalArgumentException if an objective with this name already exists, the name is too long, or the display name is too long
+ */
+ public Objective createObjective(String name, Objective.Criteria criteria, String displayName);
+
+ /**
+ * Gets an objective by name
@Anxuiz

Anxuiz Mar 23, 2013

Exact match? Case sensitive?

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Scoreboard.java
+ */
+ public Set<Objective> getObjectives();
+
+ /**
+ * Removes an objective
+ *
+ * @param objective The objective to be removed, can not be null
+ */
+ public void removeObjective(Objective objective);
+
+ /**
+ * Resets a player's scores
+ *
+ * @param player The player whose scores will be reset, can not be null
+ */
+ public void resetScores(OfflinePlayer player);
@Anxuiz

Anxuiz Mar 23, 2013

What exactly does this do? How is it different that using setScore on the Objective?

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Team.java
@@ -0,0 +1,82 @@
+package org.bukkit.scoreboard;
+
+
+public interface Team {
@Anxuiz

Anxuiz Mar 23, 2013

Need interface javadoc.

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Team.java
+package org.bukkit.scoreboard;
+
+
+public interface Team {
+
+ /*
+ * Get the team's name
+ *
+ * @return The team's name
+ */
+ public String getName();
+
+ /*
+ * Get the team's display name
+ *
+ * @return The team's display name
@Anxuiz

Anxuiz Mar 23, 2013

Can this be null?

@Anxuiz Anxuiz commented on the diff Mar 23, 2013

src/main/java/org/bukkit/scoreboard/Team.java
+ /*
+ * Get the team's display name
+ *
+ * @return The team's display name
+ */
+ public String getDisplayName();
+
+ /*
+ * Set the team's display name
+ *
+ * @param name New display name, can not be null
+ */
+ public void setDisplayName(String name);
+
+ /*
+ * Get the team's prefix

@tonybruess tonybruess closed this Mar 30, 2013

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