Reduce technical debt

* Reduce duplicate code
* Use more specific functional interfaces where available
* Fix some potential NPEs
* Remove some unnecessary/nonfunctional code
* Merge inventory listeners - no longer need to keep separate due to event availability
* Removed TODO items that probably won't ever be implemented. Good ideas, too drastic changes or too much work to maintain.
This commit is contained in:
Jikoo 2020-09-15 11:43:43 -04:00
parent a5b02ab26a
commit 1a6d513603
11 changed files with 146 additions and 259 deletions

View file

@ -18,6 +18,7 @@ package com.lishid.openinv.util;
import com.lishid.openinv.internal.IInventoryAccess;
import com.lishid.openinv.internal.ISpecialEnderChest;
import com.lishid.openinv.internal.ISpecialInventory;
import com.lishid.openinv.internal.ISpecialPlayerInventory;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
@ -33,9 +34,8 @@ public class InventoryAccess implements IInventoryAccess {
static {
String packageName = Bukkit.getServer().getClass().getPackage().getName();
String version = packageName.substring(packageName.lastIndexOf('.') + 1);
try {
craftInventory = Class.forName("org.bukkit.craftbukkit." + version + ".inventory.CraftInventory");
craftInventory = Class.forName(packageName + ".inventory.CraftInventory");
} catch (ClassNotFoundException ignored) {}
try {
getInventory = craftInventory.getDeclaredMethod("getInventory");
@ -47,62 +47,42 @@ public class InventoryAccess implements IInventoryAccess {
}
public static boolean isPlayerInventory(@NotNull Inventory inventory) {
if (craftInventory.isAssignableFrom(inventory.getClass())) {
try {
return getInventory.invoke(inventory) instanceof ISpecialPlayerInventory;
} catch (ReflectiveOperationException ignored) {}
}
return grabFieldOfTypeFromObject(ISpecialPlayerInventory.class, inventory) != null;
return getPlayerInventory(inventory) != null;
}
public static ISpecialPlayerInventory getPlayerInventory(@NotNull Inventory inventory) {
Object inv = null;
if (craftInventory.isAssignableFrom(inventory.getClass())) {
try {
inv = getInventory.invoke(inventory);
} catch (ReflectiveOperationException ignored) {}
}
if (inv == null) {
inv = grabFieldOfTypeFromObject(ISpecialPlayerInventory.class, inventory);
}
if (inv instanceof ISpecialPlayerInventory) {
return (ISpecialPlayerInventory) inv;
}
return null;
public static @Nullable ISpecialPlayerInventory getPlayerInventory(@NotNull Inventory inventory) {
return getSpecialInventory(ISpecialPlayerInventory.class, inventory);
}
public static boolean isEnderChest(@NotNull Inventory inventory) {
if (craftInventory.isAssignableFrom(inventory.getClass())) {
try {
return getInventory.invoke(inventory) instanceof ISpecialEnderChest;
} catch (ReflectiveOperationException ignored) {}
}
return grabFieldOfTypeFromObject(ISpecialEnderChest.class, inventory) != null;
return getEnderChest(inventory) != null;
}
public static ISpecialEnderChest getEnderChest(@NotNull Inventory inventory) {
Object inv = null;
if (craftInventory.isAssignableFrom(inventory.getClass())) {
public static @Nullable ISpecialEnderChest getEnderChest(@NotNull Inventory inventory) {
return getSpecialInventory(ISpecialEnderChest.class, inventory);
}
private static <T extends ISpecialInventory> @Nullable T getSpecialInventory(@NotNull Class<T> expected, @NotNull Inventory inventory) {
Object inv;
if (craftInventory != null && getInventory != null && craftInventory.isAssignableFrom(inventory.getClass())) {
try {
inv = getInventory.invoke(inventory);
if (expected.isInstance(inv)) {
return expected.cast(inv);
}
} catch (ReflectiveOperationException ignored) {}
}
if (inv == null) {
inv = grabFieldOfTypeFromObject(ISpecialEnderChest.class, inventory);
}
inv = grabFieldOfTypeFromObject(ISpecialPlayerInventory.class, inventory);
if (inv instanceof ISpecialEnderChest) {
return (ISpecialEnderChest) inv;
if (expected.isInstance(inv)) {
return expected.cast(inv);
}
return null;
}
private static <T> T grabFieldOfTypeFromObject(final Class<T> type, final Object object) {
private static <T> @Nullable T grabFieldOfTypeFromObject(final Class<T> type, final Object object) {
// Use reflection to find the IInventory
Class<?> clazz = object.getClass();
T result = null;
@ -142,4 +122,5 @@ public class InventoryAccess implements IInventoryAccess {
public boolean isSpecialPlayerInventory(@NotNull Inventory inventory) {
return isPlayerInventory(inventory);
}
}

View file

@ -27,9 +27,7 @@ import com.lishid.openinv.internal.IAnySilentContainer;
import com.lishid.openinv.internal.ISpecialEnderChest;
import com.lishid.openinv.internal.ISpecialInventory;
import com.lishid.openinv.internal.ISpecialPlayerInventory;
import com.lishid.openinv.listeners.InventoryClickListener;
import com.lishid.openinv.listeners.InventoryCloseListener;
import com.lishid.openinv.listeners.InventoryDragListener;
import com.lishid.openinv.listeners.InventoryListener;
import com.lishid.openinv.listeners.PlayerListener;
import com.lishid.openinv.listeners.PluginListener;
import com.lishid.openinv.util.Cache;
@ -105,7 +103,6 @@ public class OpenInv extends JavaPlugin implements IOpenInv {
if (!OpenInv.this.disableSaving() && !value.isOnline()) {
value.saveData();
}
return true;
});
private InternalAccessor accessor;
@ -127,6 +124,7 @@ public class OpenInv extends JavaPlugin implements IOpenInv {
if (this.inventories.containsKey(key)) {
Iterator<HumanEntity> iterator = this.inventories.get(key).getBukkitInventory().getViewers().iterator();
//noinspection WhileLoopReplaceableByForEach
while (iterator.hasNext()) {
HumanEntity human = iterator.next();
// If player has permission or is in the same world, allow continued access
@ -140,6 +138,7 @@ public class OpenInv extends JavaPlugin implements IOpenInv {
if (this.enderChests.containsKey(key)) {
Iterator<HumanEntity> iterator = this.enderChests.get(key).getBukkitInventory().getViewers().iterator();
//noinspection WhileLoopReplaceableByForEach
while (iterator.hasNext()) {
HumanEntity human = iterator.next();
if (Permissions.CROSSWORLD.hasPermission(human) || human.getWorld().equals(player.getWorld())) {
@ -222,22 +221,18 @@ public class OpenInv extends JavaPlugin implements IOpenInv {
return this.accessor != null && this.accessor.isSupported();
}
@Nullable
@Override
public Player loadPlayer(@NotNull final OfflinePlayer offline) {
public @Nullable Player loadPlayer(@NotNull final OfflinePlayer offline) {
String key = this.getPlayerID(offline);
if (this.playerCache.containsKey(key)) {
return this.playerCache.get(key);
}
// TODO: wrap Player to ensure all methods can safely be called offline
Player loaded;
if (offline.isOnline()) {
loaded = offline.getPlayer();
this.playerCache.put(key, loaded);
return loaded;
Player player = offline.getPlayer();
if (player != null) {
this.playerCache.put(key, player);
return player;
}
if (!this.isSupportedVersion()) {
@ -251,33 +246,18 @@ public class OpenInv extends JavaPlugin implements IOpenInv {
Future<Player> future = Bukkit.getScheduler().callSyncMethod(this,
() -> OpenInv.this.accessor.getPlayerDataManager().loadPlayer(offline));
int ticks = 0;
while (!future.isDone() && !future.isCancelled() && ticks < 10) {
++ticks;
try {
Thread.sleep(50L);
} catch (InterruptedException e) {
e.printStackTrace();
return null;
}
}
if (!future.isDone() || future.isCancelled()) {
return null;
}
try {
loaded = future.get();
player = future.get();
} catch (InterruptedException | ExecutionException e) {
e.printStackTrace();
return null;
}
if (loaded != null) {
this.playerCache.put(key, loaded);
if (player != null) {
this.playerCache.put(key, player);
}
return loaded;
return player;
}
@Override
@ -317,8 +297,7 @@ public class OpenInv extends JavaPlugin implements IOpenInv {
return this.languageManager.getValue(key, getLocale(sender), replacements);
}
@Nullable
private String getLocale(@NotNull CommandSender sender) {
private @Nullable String getLocale(@NotNull CommandSender sender) {
if (sender instanceof Player) {
return this.accessor.getPlayerDataManager().getLocale((Player) sender);
} else {
@ -369,10 +348,7 @@ public class OpenInv extends JavaPlugin implements IOpenInv {
// Register listeners
pm.registerEvents(new PlayerListener(this), this);
pm.registerEvents(new PluginListener(this), this);
pm.registerEvents(new InventoryClickListener(), this);
pm.registerEvents(new InventoryCloseListener(this), this);
// Bukkit will handle missing events for us, attempt to register InventoryDragEvent without a version check
pm.registerEvents(new InventoryDragListener(), this);
pm.registerEvents(new InventoryListener(this), this);
// Register commands to their executors
OpenInvCommand openInv = new OpenInvCommand(this);

View file

@ -21,7 +21,7 @@ import com.lishid.openinv.util.TabCompleter;
import java.util.Collections;
import java.util.List;
import java.util.function.BiConsumer;
import java.util.function.Function;
import java.util.function.Predicate;
import org.bukkit.OfflinePlayer;
import org.bukkit.command.Command;
import org.bukkit.command.CommandSender;
@ -46,7 +46,7 @@ public class ContainerSettingCommand implements TabExecutor {
Player player = (Player) sender;
boolean any = command.getName().startsWith("any");
Function<Player, Boolean> getSetting = any ? plugin::getPlayerAnyChestStatus : plugin::getPlayerSilentChestStatus;
Predicate<Player> getSetting = any ? plugin::getPlayerAnyChestStatus : plugin::getPlayerSilentChestStatus;
BiConsumer<OfflinePlayer, Boolean> setSetting = any ? plugin::setPlayerAnyChestStatus : plugin::setPlayerSilentChestStatus;
if (args.length > 0) {
@ -62,12 +62,12 @@ public class ContainerSettingCommand implements TabExecutor {
}
} else {
setSetting.accept(player, !getSetting.apply(player));
setSetting.accept(player, !getSetting.test(player));
}
String onOff = plugin.getLocalizedMessage(player, getSetting.apply(player) ? "messages.info.on" : "messages.info.off");
String onOff = plugin.getLocalizedMessage(player, getSetting.test(player) ? "messages.info.on" : "messages.info.off");
if (onOff == null) {
onOff = String.valueOf(getSetting.apply(player));
onOff = String.valueOf(getSetting.test(player));
}
plugin.sendMessage(sender, "messages.info.settingState","%setting%", any ? "AnyContainer" : "SilentContainer", "%state%", onOff);

View file

@ -31,6 +31,7 @@ import org.bukkit.inventory.Inventory;
import org.bukkit.inventory.ItemStack;
import org.bukkit.inventory.meta.ItemMeta;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
/**
* Command adding the ability to search online players' inventories for enchantments of a specific
@ -124,8 +125,9 @@ public class SearchEnchantCommand implements TabExecutor {
return true;
}
private boolean containsEnchantment(Inventory inventory, Enchantment enchant, int minLevel) {
private boolean containsEnchantment(Inventory inventory, @Nullable Enchantment enchant, int minLevel) {
for (ItemStack item : inventory.getContents()) {
//noinspection ConstantConditions // Spigot improperly annotated, should be ItemStack @NotNull []
if (item == null || item.getType() == Material.AIR) {
continue;
}

View file

@ -1,40 +0,0 @@
/*
* Copyright (C) 2011-2020 lishid. All rights reserved.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, version 3.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
package com.lishid.openinv.listeners;
import com.lishid.openinv.util.InventoryAccess;
import com.lishid.openinv.util.Permissions;
import org.bukkit.entity.HumanEntity;
import org.bukkit.event.EventHandler;
import org.bukkit.event.EventPriority;
import org.bukkit.event.Listener;
import org.bukkit.event.inventory.InventoryClickEvent;
import org.bukkit.inventory.Inventory;
public class InventoryClickListener implements Listener {
@EventHandler(priority = EventPriority.NORMAL, ignoreCancelled = true)
public void onInventoryClick(InventoryClickEvent event) {
HumanEntity entity = event.getWhoClicked();
Inventory inventory = event.getInventory();
if (InventoryAccess.isPlayerInventory(inventory) && !Permissions.EDITINV.hasPermission(entity)
|| InventoryAccess.isEnderChest(inventory) && !Permissions.EDITENDER.hasPermission(entity)) {
event.setCancelled(true);
}
}
}

View file

@ -1,46 +0,0 @@
/*
* Copyright (C) 2011-2020 lishid. All rights reserved.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, version 3.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
package com.lishid.openinv.listeners;
import com.lishid.openinv.IOpenInv;
import org.bukkit.entity.Player;
import org.bukkit.event.EventHandler;
import org.bukkit.event.Listener;
import org.bukkit.event.inventory.InventoryCloseEvent;
public class InventoryCloseListener implements Listener {
private final IOpenInv plugin;
public InventoryCloseListener(final IOpenInv plugin) {
this.plugin = plugin;
}
@EventHandler
public void onInventoryClose(final InventoryCloseEvent event) {
if (!(event.getPlayer() instanceof Player)) {
return;
}
Player player = (Player) event.getPlayer();
if (this.plugin.getPlayerSilentChestStatus(player)) {
this.plugin.getAnySilentContainer().deactivateContainer(player);
}
}
}

View file

@ -16,24 +16,57 @@
package com.lishid.openinv.listeners;
import com.lishid.openinv.IOpenInv;
import com.lishid.openinv.util.InventoryAccess;
import com.lishid.openinv.util.Permissions;
import org.bukkit.entity.HumanEntity;
import org.bukkit.entity.Player;
import org.bukkit.event.EventHandler;
import org.bukkit.event.EventPriority;
import org.bukkit.event.Listener;
import org.bukkit.event.inventory.InventoryClickEvent;
import org.bukkit.event.inventory.InventoryCloseEvent;
import org.bukkit.event.inventory.InventoryDragEvent;
import org.bukkit.event.inventory.InventoryInteractEvent;
import org.bukkit.inventory.Inventory;
/**
* Listener for InventoryDragEvents to prevent unpermitted modification of special inventories.
* Listener for inventory-related events to prevent modification of inventories where not allowed.
*
* @author Jikoo
*/
public class InventoryDragListener implements Listener {
public class InventoryListener implements Listener {
private final IOpenInv plugin;
public InventoryListener(final IOpenInv plugin) {
this.plugin = plugin;
}
@EventHandler
public void onInventoryClose(final InventoryCloseEvent event) {
if (!(event.getPlayer() instanceof Player)) {
return;
}
Player player = (Player) event.getPlayer();
if (this.plugin.getPlayerSilentChestStatus(player)) {
this.plugin.getAnySilentContainer().deactivateContainer(player);
}
}
@EventHandler(priority = EventPriority.NORMAL, ignoreCancelled = true)
public void onInventoryClick(InventoryClickEvent event) {
onInventoryInteract(event);
}
@EventHandler(priority = EventPriority.NORMAL, ignoreCancelled = true)
public void onInventoryDrag(InventoryDragEvent event) {
onInventoryInteract(event);
}
private void onInventoryInteract(InventoryInteractEvent event) {
HumanEntity entity = event.getWhoClicked();
Inventory inventory = event.getInventory();
if (InventoryAccess.isPlayerInventory(inventory) && !Permissions.EDITINV.hasPermission(entity)

View file

@ -24,7 +24,8 @@ import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.function.Function;
import java.util.function.Consumer;
import java.util.function.Predicate;
/**
* A minimal thread-safe time-based cache implementation backed by a HashMap and TreeMultimap.
@ -36,16 +37,17 @@ public class Cache<K, V> {
private final Map<K, V> internal;
private final Multimap<Long, K> expiry;
private final long retention;
private final Function<V, Boolean> inUseCheck, postRemoval;
private final Predicate<V> inUseCheck;
private final Consumer<V> postRemoval;
/**
* Constructs a Cache with the specified retention duration, in use function, and post-removal function.
*
* @param retention duration after which keys are automatically invalidated if not in use
* @param inUseCheck Function used to check if a key is considered in use
* @param postRemoval Function used to perform any operations required when a key is invalidated
* @param inUseCheck Predicate used to check if a key is considered in use
* @param postRemoval Consumer used to perform any operations required when a key is invalidated
*/
public Cache(final long retention, final Function<V, Boolean> inUseCheck, final Function<V, Boolean> postRemoval) {
public Cache(final long retention, final Predicate<V> inUseCheck, final Consumer<V> postRemoval) {
this.internal = new HashMap<>();
this.expiry = TreeMultimap.create(Long::compareTo, (k1, k2) -> Objects.equals(k1, k2) ? 0 : 1);
@ -136,7 +138,7 @@ public class Cache<K, V> {
public void invalidateAll() {
synchronized (this.internal) {
for (V value : this.internal.values()) {
this.postRemoval.apply(value);
this.postRemoval.accept(value);
}
this.expiry.clear();
this.internal.clear();
@ -161,7 +163,7 @@ public class Cache<K, V> {
iterator.remove();
if (this.inUseCheck.apply(this.internal.get(entry.getValue()))) {
if (this.inUseCheck.test(this.internal.get(entry.getValue()))) {
inUse.add(entry.getValue());
continue;
}
@ -172,7 +174,7 @@ public class Cache<K, V> {
continue;
}
this.postRemoval.apply(value);
this.postRemoval.accept(value);
}
long nextExpiry = now + this.retention;

View file

@ -24,7 +24,6 @@ import java.util.Map;
import java.util.Set;
import org.bukkit.OfflinePlayer;
import org.bukkit.configuration.ConfigurationSection;
import org.bukkit.scheduler.BukkitRunnable;
public class ConfigUpdater {
@ -36,7 +35,8 @@ public class ConfigUpdater {
public void checkForUpdates() {
final int version = plugin.getConfig().getInt("config-version", 1);
if (version >= plugin.getConfig().getDefaults().getInt("config-version")) {
ConfigurationSection defaults = plugin.getConfig().getDefaults();
if (defaults == null || version >= defaults.getInt("config-version")) {
return;
}
@ -50,9 +50,7 @@ public class ConfigUpdater {
plugin.getLogger().warning("Could not back up config.yml before updating!");
}
new BukkitRunnable() {
@Override
public void run() {
plugin.getServer().getScheduler().runTaskAsynchronously(plugin, () -> {
if (version < 2) {
updateConfig1To2();
}
@ -63,32 +61,23 @@ public class ConfigUpdater {
updateConfig3To4();
}
new BukkitRunnable() {
@Override
public void run() {
plugin.getServer().getScheduler().runTask(plugin, () -> {
plugin.saveConfig();
plugin.getLogger().info("Configuration update complete!");
}
}.runTaskLater(plugin, 1L); // Run on 1 tick delay; on older versions Bukkit's scheduler is not guaranteed FIFO
}
}.runTaskAsynchronously(plugin);
});
});
}
private void updateConfig3To4() {
new BukkitRunnable() {
@Override
public void run() {
plugin.getServer().getScheduler().runTask(plugin, () -> {
plugin.getConfig().set("notify", null);
plugin.getConfig().set("settings.locale", "en_US");
plugin.getConfig().set("config-version", 4);
}
}.runTask(plugin);
});
}
private void updateConfig2To3() {
new BukkitRunnable() {
@Override
public void run() {
plugin.getServer().getScheduler().runTask(plugin, () -> {
plugin.getConfig().set("config-version", 3);
plugin.getConfig().set("items.open-inv", null);
plugin.getConfig().set("ItemOpenInv", null);
@ -96,14 +85,11 @@ public class ConfigUpdater {
plugin.getConfig().set("settings.disable-saving",
plugin.getConfig().getBoolean("DisableSaving", false));
plugin.getConfig().set("DisableSaving", null);
}
}.runTask(plugin);
});
}
private void updateConfig1To2() {
new BukkitRunnable() {
@Override
public void run() {
plugin.getServer().getScheduler().runTask(plugin, () -> {
// Get the old config settings
boolean notifySilentChest = plugin.getConfig().getBoolean("NotifySilentChest", true);
boolean notifyAnyChest = plugin.getConfig().getBoolean("NotifyAnyChest", true);
@ -113,24 +99,23 @@ public class ConfigUpdater {
plugin.getConfig().set("config-version", 2);
plugin.getConfig().set("notify.any-chest", notifyAnyChest);
plugin.getConfig().set("notify.silent-chest", notifySilentChest);
}
}.runTask(plugin);
});
updateToggles("AnyChest", "toggles.any-chest");
updateToggles("SilentChest", "toggles.silent-chest");
}
private void updateToggles(final String sectionName, final String newSectionName) {
ConfigurationSection section = plugin.getConfig().getConfigurationSection(sectionName);
// Ensure section exists
if (!plugin.getConfig().isConfigurationSection(sectionName)) {
if (section == null) {
return;
}
ConfigurationSection section = plugin.getConfig().getConfigurationSection(sectionName);
Set<String> keys = section.getKeys(false);
// Ensure section has content
if (keys == null || keys.isEmpty()) {
if (keys.isEmpty()) {
return;
}
@ -143,25 +128,20 @@ public class ConfigUpdater {
}
}
new BukkitRunnable() {
@Override
public void run() {
plugin.getServer().getScheduler().runTask(plugin, () -> {
// Wipe old ConfigurationSection
plugin.getConfig().set(sectionName, null);
// Prepare new ConfigurationSection
ConfigurationSection newSection;
if (plugin.getConfig().isConfigurationSection(newSectionName)) {
newSection = plugin.getConfig().getConfigurationSection(newSectionName);
} else {
ConfigurationSection newSection = plugin.getConfig().getConfigurationSection(newSectionName);
if (newSection == null) {
newSection = plugin.getConfig().createSection(newSectionName);
}
// Set new values
for (Map.Entry<String, Boolean> entry : toggles.entrySet()) {
newSection.set(entry.getKey(), entry.getValue());
}
}
}.runTask(plugin);
});
}
}

View file

@ -40,7 +40,6 @@ public class InternalAccessor {
this.version = packageName.substring(packageName.lastIndexOf('.') + 1);
try {
// TODO: implement support for CraftMagicNumbers#getMappingsVersion
Class.forName("com.lishid.openinv.internal." + this.version + ".SpecialPlayerInventory");
Class.forName("com.lishid.openinv.internal." + this.version + ".SpecialEnderChest");
this.playerDataManager = this.createObject(IPlayerDataManager.class, "PlayerDataManager");

View file

@ -41,7 +41,7 @@ public class LanguageManager {
private final OpenInv plugin;
private final String defaultLocale;
private Map<String, YamlConfiguration> locales;
private final Map<String, YamlConfiguration> locales;
public LanguageManager(@NotNull OpenInv plugin, @NotNull String defaultLocale) {
this.plugin = plugin;