Merge pull request #238 from gravypod/patch-3

Fixing snowleo's comments on my last PR

7 files changed, 139 insertions, 76 deletions

diff --git a/Essentials/src/net/ess3/bukkit/BukkitPlugin.java b/Essentials/src/net/ess3/bukkit/BukkitPlugin.java
index ce831d010..9c7e20cd3 100644
--- a/Essentials/src/net/ess3/bukkit/BukkitPlugin.java
+++ b/Essentials/src/net/ess3/bukkit/BukkitPlugin.java
@@ -183,7 +183,7 @@ public class BukkitPlugin extends JavaPlugin implements IPlugin
 	@Override
 	public void cancelTask(BukkitTask taskId)
 	{
-		getServer().getScheduler().cancelTask(taskId.getTaskId());
+		taskId.cancel();
 	}
 
 	@Override
diff --git a/Essentials/src/net/ess3/commands/Commandbanip.java b/Essentials/src/net/ess3/commands/Commandbanip.java
index ee5defa95..94f05e76e 100644
--- a/Essentials/src/net/ess3/commands/Commandbanip.java
+++ b/Essentials/src/net/ess3/commands/Commandbanip.java
@@ -29,7 +29,7 @@ public class Commandbanip extends EssentialsCommand
 			{
 				throw new Exception(_("playerNotFound"));
 			}
-			ess.getServer().banIP(ipAddress /*TODO: This shouldnt be needed, test player.getData().getIpAddress()*/);
+			ess.getServer().banIP(ipAddress);
 			sender.sendMessage(_("banIpAddress"));
 		}
 	}
diff --git a/Essentials/src/net/ess3/commands/Commandjails.java b/Essentials/src/net/ess3/commands/Commandjails.java
index 4563b237d..cb2b6b6f1 100644
--- a/Essentials/src/net/ess3/commands/Commandjails.java
+++ b/Essentials/src/net/ess3/commands/Commandjails.java
@@ -10,6 +10,6 @@ public class Commandjails extends EssentialsCommand
 	@Override
 	protected void run(final CommandSender sender, final String commandLabel, final String[] args) throws Exception
 	{
-		sender.sendMessage(ChatColor.GRAY /* "§7" */ + Util.joinList(" ", ess.getJails().getList()));
+		sender.sendMessage(ChatColor.GRAY + Util.joinList(" ", ess.getJails().getList()));
 	}
 }
diff --git a/Essentials/src/net/ess3/commands/Commandtempban.java b/Essentials/src/net/ess3/commands/Commandtempban.java
index 98201228b..b579c6992 100644
--- a/Essentials/src/net/ess3/commands/Commandtempban.java
+++ b/Essentials/src/net/ess3/commands/Commandtempban.java
@@ -41,11 +41,11 @@ public class Commandtempban extends EssentialsCommand
 		final long banTimestamp = DateUtil.parseDateDiff(time, true);
 
 		final String banReason = _("tempBanned", DateUtil.formatDateDiff(banTimestamp));
-		UserData userData = user.getData();
-		userData.setBan(new Ban());
-		Ban ban = userData.getBan();
+		final Ban ban = new Ban();
+		final UserData userData = user.getData();
 		ban.setReason(banReason);
 		ban.setTimeout(banTimestamp);
+		userData.setBan(ban);
 		user.setBanned(true);
 		user.queueSave();
 		user.getPlayer().kickPlayer(banReason);
diff --git a/Essentials/src/net/ess3/economy/Trade.java b/Essentials/src/net/ess3/economy/Trade.java
index 4b864f691..8ccc6f205 100644
--- a/Essentials/src/net/ess3/economy/Trade.java
+++ b/Essentials/src/net/ess3/economy/Trade.java
@@ -280,7 +280,7 @@ public class Trade
 		{
 			if (pay.getItemStack() != null)
 			{
-				ItemStack is = pay.getItemStack();
+				final ItemStack is = pay.getItemStack();
 				sb.append(is.getAmount()).append(",");
 				sb.append(is.getType().toString()).append(",");
 				sb.append(is.getDurability());
diff --git a/Essentials/src/net/ess3/metrics/Metrics.java b/Essentials/src/net/ess3/metrics/Metrics.java
index f20d3a86b..bff2f7d01 100644
--- a/Essentials/src/net/ess3/metrics/Metrics.java
+++ b/Essentials/src/net/ess3/metrics/Metrics.java
@@ -23,7 +23,6 @@ package net.ess3.metrics;
  * The views and conclusions contained in the software and documentation are those of the authors and contributors and
  * should not be interpreted as representing official policies, either expressed or implied, of anybody else.
  */
-
 import java.io.*;
 import java.net.Proxy;
 import java.net.URL;
@@ -36,6 +35,7 @@ import org.bukkit.configuration.InvalidConfigurationException;
 import org.bukkit.configuration.file.YamlConfiguration;
 import org.bukkit.plugin.Plugin;
 import org.bukkit.plugin.PluginDescriptionFile;
+import org.bukkit.scheduler.BukkitTask;
 
 
 /**
@@ -52,7 +52,7 @@ public class Metrics
 	/**
 	 * The current revision number
 	 */
-	private final static int REVISION = 5;
+	private final static int REVISION = 6;
 	/**
 	 * The base url of the metrics domain
 	 */
@@ -62,10 +62,6 @@ public class Metrics
 	 */
 	private static final String REPORT_URL = "/report/%s";
 	/**
-	 * The file where guid and opt out is stored in
-	 */
-	private static final String CONFIG_FILE = "plugins/PluginMetrics/config.yml";
-	/**
 	 * The separator to use for custom data. This MUST NOT change unless you are hosting your own version of metrics and
 	 * want to change it.
 	 */
@@ -99,13 +95,17 @@ public class Metrics
 	 */
 	private final String guid;
 	/**
+	 * Debug mode
+	 */
+	private final boolean debug;
+	/**
 	 * Lock for synchronization
 	 */
 	private final Object optOutLock = new Object();
 	/**
-	 * Id of the scheduled task
+	 * The scheduled task
 	 */
-	private volatile int taskId = -1;
+	private volatile BukkitTask task = null;
 
 	public Metrics(final Plugin plugin) throws IOException
 	{
@@ -117,29 +117,31 @@ public class Metrics
 		this.plugin = plugin;
 
 		// load the config
-		configurationFile = new File(CONFIG_FILE);
+		configurationFile = getConfigFile();
 		configuration = YamlConfiguration.loadConfiguration(configurationFile);
 
 		// add some defaults
 		configuration.addDefault("opt-out", false);
 		configuration.addDefault("guid", UUID.randomUUID().toString());
+		configuration.addDefault("debug", false);
 
 		// Do we need to create the file?
 		if (configuration.get("guid", null) == null)
 		{
-			configuration.options().header("http://metrics.griefcraft.com").copyDefaults(true);
+			configuration.options().header("http://mcstats.org").copyDefaults(true);
 			configuration.save(configurationFile);
 		}
 
 		// Load the guid then
 		guid = configuration.getString("guid");
+		debug = configuration.getBoolean("debug", false);
 	}
 
 	/**
 	 * Construct and create a Graph that can be used to separate specific plotters to their own graphs on the metrics
 	 * website. Plotters can be added to the graph object returned.
 	 *
-	 * @param name
+	 * @param name The name of the graph
 	 * @return Graph object created. Will never return NULL under normal circumstances unless bad parameters are given
 	 */
 	public Graph createGraph(final String name)
@@ -162,7 +164,7 @@ public class Metrics
 	/**
 	 * Adds a custom data plotter to the default graph
 	 *
-	 * @param plotter
+	 * @param plotter The plotter to use to plot custom data
 	 */
 	public void addCustomData(final Plotter plotter)
 	{
@@ -194,8 +196,7 @@ public class Metrics
 			}
 
 			// Begin hitting the server with glorious data
-			taskId = plugin.getServer().getScheduler().runTaskTimerAsynchronously(
-					plugin, new Runnable()
+			task = plugin.getServer().getScheduler().runTaskTimerAsynchronously(plugin, new Runnable()
 			{
 				private boolean firstPost = true;
 
@@ -207,10 +208,15 @@ public class Metrics
 						synchronized (optOutLock)
 						{
 							// Disable Task, if it is running and the server owner decided to opt-out
-							if (isOptOut() && taskId > 0)
+							if (isOptOut() && task != null)
 							{
-								plugin.getServer().getScheduler().cancelTask(taskId);
-								taskId = -1;
+								task.cancel();
+								task = null;
+								// Tell all plotters to stop gathering information.
+								for (Graph graph : graphs)
+								{
+									graph.onOptOut();
+								}
 							}
 						}
 
@@ -225,17 +231,20 @@ public class Metrics
 					}
 					catch (IOException e)
 					{
-						plugin.getLogger().log(Level.INFO, "[Metrics] " + e.getMessage());
+						if (debug)
+						{
+							Bukkit.getLogger().log(Level.INFO, "[Metrics] " + e.getMessage());
+						}
 					}
 				}
-			}, 0, PING_INTERVAL * 1200).getTaskId();
+			}, 0, PING_INTERVAL * 1200);
 		}
 	}
 
 	/**
 	 * Has the server owner denied plugin metrics?
 	 *
-	 * @return
+	 * @return true if metrics should be opted out of it
 	 */
 	public boolean isOptOut()
 	{
@@ -244,16 +253,22 @@ public class Metrics
 			try
 			{
 				// Reload the metrics file
-				configuration.load(CONFIG_FILE);
+				configuration.load(getConfigFile());
 			}
 			catch (IOException ex)
 			{
-				plugin.getLogger().log(Level.INFO, "[Metrics] " + ex.getMessage());
+				if (debug)
+				{
+					Bukkit.getLogger().log(Level.INFO, "[Metrics] " + ex.getMessage());
+				}
 				return true;
 			}
 			catch (InvalidConfigurationException ex)
 			{
-				plugin.getLogger().log(Level.INFO, "[Metrics] " + ex.getMessage());
+				if (debug)
+				{
+					Bukkit.getLogger().log(Level.INFO, "[Metrics] " + ex.getMessage());
+				}
 				return true;
 			}
 			return configuration.getBoolean("opt-out", false);
@@ -278,7 +293,7 @@ public class Metrics
 			}
 
 			// Enable Task, if it is not running
-			if (taskId < 0)
+			if (task == null)
 			{
 				start();
 			}
@@ -299,35 +314,81 @@ public class Metrics
 			if (!isOptOut())
 			{
 				configuration.set("opt-out", true);
-				final File file = new File(CONFIG_FILE);
-				configuration.save(file);
+				configuration.save(configurationFile);
 			}
 
 			// Disable Task, if it is running
-			if (taskId >= 0)
+			if (task != null)
 			{
-				this.plugin.getServer().getScheduler().cancelTask(taskId);
-				taskId = -1;
+				task.cancel();
+				task = null;
 			}
 		}
 	}
 
 	/**
+	 * Gets the File object of the config file that should be used to store data such as the GUID and opt-out status
+	 *
+	 * @return the File object for the config file
+	 */
+	public File getConfigFile()
+	{
+		// I believe the easiest way to get the base folder (e.g craftbukkit set via -P) for plugins to use
+		// is to abuse the plugin object we already have
+		// plugin.getDataFolder() => base/plugins/PluginA/
+		// pluginsFolder => base/plugins/
+		// The base is not necessarily relative to the startup directory.
+		File pluginsFolder = plugin.getDataFolder().getParentFile();
+
+		// return => base/plugins/PluginMetrics/config.yml
+		return new File(new File(pluginsFolder, "PluginMetrics"), "config.yml");
+	}
+
+	/**
 	 * Generic method that posts a plugin to the metrics website
 	 */
 	private void postPlugin(final boolean isPing) throws IOException
 	{
-		// The plugin's description file containg all of the plugin data such as name, version, author, etc
-		final PluginDescriptionFile description = plugin.getDescription();
+		// Server software specific section
+		PluginDescriptionFile description = plugin.getDescription();
+		String pluginName = description.getName();
+		boolean onlineMode = Bukkit.getServer().getOnlineMode(); // TRUE if online mode is enabled
+		String pluginVersion = description.getVersion();
+		String serverVersion = Bukkit.getVersion();
+		int playersOnline = Bukkit.getServer().getOnlinePlayers().length;
+
+		// END server software specific section -- all code below does not use any code outside of this class / Java
 
 		// Construct the post data
 		final StringBuilder data = new StringBuilder();
+
+		// The plugin's description file containg all of the plugin data such as name, version, author, etc
 		data.append(encode("guid")).append('=').append(encode(guid));
-		encodeDataPair(data, "version", description.getVersion());
-		encodeDataPair(data, "server", Bukkit.getVersion());
-		encodeDataPair(data, "players", Integer.toString(plugin.getServer().getOnlinePlayers().length));
+		encodeDataPair(data, "version", pluginVersion);
+		encodeDataPair(data, "server", serverVersion);
+		encodeDataPair(data, "players", Integer.toString(playersOnline));
 		encodeDataPair(data, "revision", String.valueOf(REVISION));
 
+		// New data as of R6
+		String osname = System.getProperty("os.name");
+		String osarch = System.getProperty("os.arch");
+		String osversion = System.getProperty("os.version");
+		String java_version = System.getProperty("java.version");
+		int coreCount = Runtime.getRuntime().availableProcessors();
+
+		// normalize os arch .. amd64 -> x86_64
+		if (osarch.equals("amd64"))
+		{
+			osarch = "x86_64";
+		}
+
+		encodeDataPair(data, "osname", osname);
+		encodeDataPair(data, "osarch", osarch);
+		encodeDataPair(data, "osversion", osversion);
+		encodeDataPair(data, "cores", Integer.toString(coreCount));
+		encodeDataPair(data, "online-mode", Boolean.toString(onlineMode));
+		encodeDataPair(data, "java_version", java_version);
+
 		// If we're pinging, append it
 		if (isPing)
 		{
@@ -344,10 +405,6 @@ public class Metrics
 			{
 				final Graph graph = iter.next();
 
-				// Because we have a lock on the graphs set already, it is reasonable to assume
-				// that our lock transcends down to the individual plotters in the graphs also.
-				// Because our methods are private, no one but us can reasonably access this list
-				// without reflection so this is a safe assumption without adding more code.
 				for (Plotter plotter : graph.getPlotters())
 				{
 					// The key name to send to the metrics server
@@ -366,7 +423,7 @@ public class Metrics
 		}
 
 		// Create the url
-		final URL url = new URL(BASE_URL + String.format(REPORT_URL, description.getName()));
+		URL url = new URL(BASE_URL + String.format(REPORT_URL, encode(pluginName)));
 
 		// Connect to the website
 		URLConnection connection;
@@ -422,13 +479,12 @@ public class Metrics
 				}
 			}
 		}
-		//if (response.startsWith("OK")) - We should get "OK" followed by an optional description if everything goes right
 	}
 
 	/**
 	 * Check if mineshafter is present. If it is, we need to bypass it to send POST requests
 	 *
-	 * @return
+	 * @return true if mineshafter is installed on the server
 	 */
 	private boolean isMineshafterPresent()
 	{
@@ -452,10 +508,9 @@ public class Metrics
 	 * encodeDataPair(data, "version", description.getVersion());
 	 * </code>
 	 *
-	 * @param buffer
-	 * @param key
-	 * @param value
-	 * @return
+	 * @param buffer the stringbuilder to append the data pair onto
+	 * @param key the key value
+	 * @param value the value
 	 */
 	private static void encodeDataPair(final StringBuilder buffer, final String key, final String value) throws UnsupportedEncodingException
 	{
@@ -465,8 +520,8 @@ public class Metrics
 	/**
 	 * Encode text as UTF-8
 	 *
-	 * @param text
-	 * @return
+	 * @param text the text to encode
+	 * @return the encoded text, as UTF-8
 	 */
 	private static String encode(final String text) throws UnsupportedEncodingException
 	{
@@ -497,7 +552,7 @@ public class Metrics
 		/**
 		 * Gets the graph's name
 		 *
-		 * @return
+		 * @return the Graph's name
 		 */
 		public String getName()
 		{
@@ -507,7 +562,7 @@ public class Metrics
 		/**
 		 * Add a plotter to the graph, which will be used to plot entries
 		 *
-		 * @param plotter
+		 * @param plotter the plotter to add to the graph
 		 */
 		public void addPlotter(final Plotter plotter)
 		{
@@ -517,7 +572,7 @@ public class Metrics
 		/**
 		 * Remove a plotter from the graph
 		 *
-		 * @param plotter
+		 * @param plotter the plotter to remove from the graph
 		 */
 		public void removePlotter(final Plotter plotter)
 		{
@@ -527,7 +582,7 @@ public class Metrics
 		/**
 		 * Gets an <b>unmodifiable</b> set of the plotter objects in the graph
 		 *
-		 * @return
+		 * @return an unmodifiable {@link Set} of the plotter objects
 		 */
 		public Set<Plotter> getPlotters()
 		{
@@ -551,6 +606,13 @@ public class Metrics
 			final Graph graph = (Graph)object;
 			return graph.name.equals(name);
 		}
+
+		/**
+		 * Called when the server owner decides to opt-out of Metrics while the server is running.
+		 */
+		protected void onOptOut()
+		{
+		}
 	}
 
 
@@ -575,7 +637,7 @@ public class Metrics
 		/**
 		 * Construct a plotter with a specific plot name
 		 *
-		 * @param name
+		 * @param name the name of the plotter to use, which will show up on the website
 		 */
 		public Plotter(final String name)
 		{
@@ -583,9 +645,11 @@ public class Metrics
 		}
 
 		/**
-		 * Get the current value for the plotted point
+		 * Get the current value for the plotted point. Since this function defers to an external function it may or may
+		 * not return immediately thus cannot be guaranteed to be thread friendly or safe. This function can be called
+		 * from any thread so care should be taken when accessing resources that need to be synchronized.
 		 *
-		 * @return
+		 * @return the current value for the point to be plotted.
 		 */
 		public abstract int getValue();
 
@@ -609,7 +673,7 @@ public class Metrics
 		@Override
 		public int hashCode()
 		{
-			return getColumnName().hashCode() + getValue();
+			return getColumnName().hashCode();
 		}
 
 		@Override
diff --git a/Essentials/src/net/ess3/utils/Util.java b/Essentials/src/net/ess3/utils/Util.java
index 3aeb82f2b..c633f5f4b 100644
--- a/Essentials/src/net/ess3/utils/Util.java
+++ b/Essentials/src/net/ess3/utils/Util.java
@@ -81,51 +81,50 @@ public final class Util
 
 	public static ItemStack convertBlockToItem(final Block block)
 	{
-		final ItemStack is = new ItemStack(
-				block.getType(), 1/*, (short)0, block.getData()*/); // TODO: Do we need block.getData? that constructor is depreciated
-
-		short s = 0;
+		final ItemStack is = new ItemStack(block.getType(), 1, block.getData());
+		
+		final short durability = 0;
 
 		switch (is.getType())
 		{
 		case WOODEN_DOOR:
 			is.setType(Material.WOOD_DOOR);
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case IRON_DOOR_BLOCK:
 			is.setType(Material.IRON_DOOR);
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case SIGN_POST:
 		case WALL_SIGN:
 			is.setType(Material.SIGN);
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case CROPS:
 			is.setType(Material.SEEDS);
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case CAKE_BLOCK:
 			is.setType(Material.CAKE);
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case BED_BLOCK:
 			is.setType(Material.BED);
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case REDSTONE_WIRE:
 			is.setType(Material.REDSTONE);
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case REDSTONE_TORCH_OFF:
 		case REDSTONE_TORCH_ON:
 			is.setType(Material.REDSTONE_TORCH_ON);
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case DIODE_BLOCK_OFF:
 		case DIODE_BLOCK_ON:
 			is.setType(Material.DIODE);
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case DOUBLE_STEP:
 			is.setType(Material.STEP);
@@ -151,7 +150,7 @@ public final class Util
 		case FENCE:
 		case FENCE_GATE:
 		case NETHER_FENCE:
-			is.setDurability(s);
+			is.setDurability(durability);
 			break;
 		case FIRE:
 			return null;