Bug fix and changes #27

2015-06-23T01:20:01-04:00

ShadowRanger commented

2015-06-23 01:20:01 -04:00

(Migrated from github.com)

Added automatic config updater
Only use UUIDFetcher when a player isn't online
Fixed null UUID error
Use new, non-deprecated Material identification system
Improved some chat output messages (added color)
Small optimizations
General refactoring
Added new searchender command code (not yet implemented, wip)

I just want to run this pull request by you lishid first before it gets merged. I made it so new configs have a different layout and don't use full capitals in the settings. I made it like this because generally YAML config file don't usually have capital letters in the keys so I thought it would be a good idea to make this change to keep with common YAML format standards. But I wanted to make sure you were happy with it before I went ahead and did anything. Let me know if you prefer the old way with capitals and I will happily change it. :)

- Added automatic config updater - Only use UUIDFetcher when a player isn't online - Fixed null UUID error - Use new, non-deprecated Material identification system - Improved some chat output messages (added color) - Small optimizations - General refactoring - Added new searchender command code (not yet implemented, wip) I just want to run this pull request by you lishid first before it gets merged. I made it so new configs have a different layout and don't use full capitals in the settings. I made it like this because generally YAML config file don't usually have capital letters in the keys so I thought it would be a good idea to make this change to keep with common YAML format standards. But I wanted to make sure you were happy with it before I went ahead and did anything. Let me know if you prefer the old way with capitals and I will happily change it. :)

lishid commented

2015-06-23 01:47:49 -04:00

(Migrated from github.com)

I'm fine with YAML/config keys either way. Might be worth double checking the bukkitdev page for the instructions if these gets changed.

I'd also say it would be useful if you can run the migration against a previous/old config file and see how well the new file come out, and if there are any inconsistencies.

I'm fine with YAML/config keys either way. Might be worth double checking the bukkitdev page for the instructions if these gets changed. I'd also say it would be useful if you can run the migration against a previous/old config file and see how well the new file come out, and if there are any inconsistencies.

lishid commented

2015-06-23 01:50:18 -04:00

(Migrated from github.com)

Other than that, I'm really happy this code is being cleaned up. Now I can feel less ashamed of the code here. :)

ShadowRanger commented

2015-06-23 02:19:34 -04:00

(Migrated from github.com)

Alright, I made those changes. 55deabe56b
Let me know if you see anything I missed out.

Alright, I made those changes. https://github.com/ShadowRanger/OpenInv/commit/55deabe56ba8730cb3ab560b4a49ed701dacb990 Let me know if you see anything I missed out.

ShadowRanger commented

2015-06-23 02:21:44 -04:00

(Migrated from github.com)

Okay I'm just about to test the config updater to make sure it works on old config. Will let you know when done. Shouldn't take long.

ShadowRanger commented

2015-06-23 02:27:02 -04:00

(Migrated from github.com)

Just tested the config updater and it looks good to me.

Old config.yml: http://pastebin.com/Px5fRx3x
New config.yml: http://pastebin.com/uG7hXmdE

Just tested the config updater and it looks good to me. Old config.yml: http://pastebin.com/Px5fRx3x New config.yml: http://pastebin.com/uG7hXmdE

lishid commented

2015-06-23 02:30:31 -04:00

(Migrated from github.com)

Hmm I'd be more curious to test the toggles. That's the more error-prone part I'd say.

ShadowRanger commented

2015-06-23 02:34:17 -04:00

(Migrated from github.com)

With the openinv item I can definitely add some output if the item name provided isn't valid. I definitely agree with that.

As for toggles, what part are you not sure about?

With the openinv item I can definitely add some output if the item name provided isn't valid. I definitely agree with that. As for toggles, what part are you not sure about?

lishid commented

2015-06-23 02:37:48 -04:00

(Migrated from github.com)

Oh just generally testing that an old config with toggles saved for anychest/silentchest do get migrated correctly. If this goes out to the world and the migration screwed up, the old config toggles are lost forever (unless plugin config backups were a thing)

ShadowRanger commented

2015-06-23 02:41:43 -04:00

(Migrated from github.com)

Yeah I understand what you mean. Whilst on that note, 1 issue that I realized would be a problem for some people is using this plugin with their server in offline mode with the config update/migration. Due to the fact that currently the toggle statuses are saved with lowercased player names, the UUIDFetcher is unable to get the UUID of the player if their username contains uppercased letters because of the fetcher being case sensetive.

As for config backups, that could definitely be possible. We could simply create a backup file of the old config.yml and name it "config_old.yml" so that if anything goes wrong people still have a backup to go back to if need be.

Yeah I understand what you mean. Whilst on that note, 1 issue that I realized would be a problem for some people is using this plugin with their server in offline mode with the config update/migration. Due to the fact that currently the toggle statuses are saved with lowercased player names, the UUIDFetcher is unable to get the UUID of the player if their username contains uppercased letters because of the fetcher being case sensetive. As for config backups, that could definitely be possible. We could simply create a backup file of the old config.yml and name it "config_old.yml" so that if anything goes wrong people still have a backup to go back to if need be.

lishid commented

2015-06-23 02:57:57 -04:00

(Migrated from github.com)

That does sound problematic... I'm thinking of removing the catch in UUIDUtil and make it bubble up the exception. Then you can deal with the failure by not actually saving the config back and disable the plugin with an error message "Your server must be online to migrate the plugin from an old version".

Do you happen to know what bukkit currently does when you ask to get a player by name? Any chance we can use that as a fallback if querying UUIDs is not available?

That does sound problematic... I'm thinking of removing the catch in UUIDUtil and make it bubble up the exception. Then you can deal with the failure by not actually saving the config back and disable the plugin with an error message "Your server must be online to migrate the plugin from an old version". Do you happen to know what bukkit currently does when you ask to get a player by name? Any chance we can use that as a fallback if querying UUIDs is not available?

ShadowRanger commented

2015-06-23 03:07:01 -04:00

(Migrated from github.com)

Yeah I can do something like that with the disabling of the plugin.

As for how Bukkit handles player retrieval by name:
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/CraftServer.java#1243

    @Override
    @Deprecated
    public OfflinePlayer getOfflinePlayer(String name) {
        Validate.notNull(name, "Name cannot be null");

        // If the name given cannot ever be a valid username give a dummy return, for scoreboard plugins
        if (!validUserPattern.matcher(name).matches()) {
            return new CraftOfflinePlayer(this, new GameProfile(invalidUserUUID, name));
        }

        OfflinePlayer result = getPlayerExact(name);
        if (result == null) {
            // This is potentially blocking :(
            GameProfile profile = MinecraftServer.getServer().getUserCache().getProfile(name);
            if (profile == null) {
                // Make an OfflinePlayer using an offline mode UUID since the name has no profile
                result = getOfflinePlayer(new GameProfile(UUID.nameUUIDFromBytes(("OfflinePlayer:" + name).getBytes(Charsets.UTF_8)), name));
            } else {
                // Use the GameProfile even when we get a UUID so we ensure we still have a name
                result = getOfflinePlayer(profile);
            }
        } else {
            offlinePlayers.remove(result.getUniqueId());
        }

        return result;
    }

I guess we could use that as a fallback. Just might not be as accurate but if it's the only option then might be worth it.

Yeah I can do something like that with the disabling of the plugin. As for how Bukkit handles player retrieval by name: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/CraftServer.java#1243 ``` java @Override @Deprecated public OfflinePlayer getOfflinePlayer(String name) { Validate.notNull(name, "Name cannot be null"); // If the name given cannot ever be a valid username give a dummy return, for scoreboard plugins if (!validUserPattern.matcher(name).matches()) { return new CraftOfflinePlayer(this, new GameProfile(invalidUserUUID, name)); } OfflinePlayer result = getPlayerExact(name); if (result == null) { // This is potentially blocking :( GameProfile profile = MinecraftServer.getServer().getUserCache().getProfile(name); if (profile == null) { // Make an OfflinePlayer using an offline mode UUID since the name has no profile result = getOfflinePlayer(new GameProfile(UUID.nameUUIDFromBytes(("OfflinePlayer:" + name).getBytes(Charsets.UTF_8)), name)); } else { // Use the GameProfile even when we get a UUID so we ensure we still have a name result = getOfflinePlayer(profile); } } else { offlinePlayers.remove(result.getUniqueId()); } return result; } ``` I guess we could use that as a fallback. Just might not be as accurate but if it's the only option then might be worth it.

lishid commented

2015-06-23 03:15:09 -04:00

(Migrated from github.com)

Seems like an OK fallback, it looks through both online and offline users. In the case it doesn't find one I think we're ok throwing it out suppose that we can't connect to MC to find the UUID.

ShadowRanger commented

2015-06-23 03:34:31 -04:00

(Migrated from github.com)

Yeah, I think so. I'll do it tonight (within the next few hours) as I have to do some things in real life first. :)

ShadowRanger commented

2015-06-23 05:10:16 -04:00

(Migrated from github.com)

Alright I just pushed another commit. The ConfigUpdater now creates a backup of the old config.yml just in case something goes wrong. Additionally, the UUIDFetcher is now case insensetive. It can now retrieve player UUID's by their name without requiring exact upper/lower casing.

lishid commented

2015-06-23 13:21:24 -04:00

(Migrated from github.com)

Looking good!

ShadowRanger commented

2015-06-23 19:30:38 -04:00

(Migrated from github.com)

Are you happy with it being merged yet? Or is there anything else you'd like me to add or change? I'm just eager to release it because some people are complaining of it not working and I fixed the problem which was with the UUID fetching.

lishid commented

2015-06-23 20:01:22 -04:00

(Migrated from github.com)

Oh totally! If you're confident the migration is ready, I'm fine with next steps.

ShadowRanger commented

2015-06-24 02:13:42 -04:00

(Migrated from github.com)

Alright cool well what I plan on doing is running a few more tests today and just making sure everything is working okay as well as some finishing touches and then i'll release it on BukkitDev :)

lishid commented

2015-06-24 02:23:17 -04:00

(Migrated from github.com)

Cool cool!

ShadowRanger commented

2015-06-24 05:28:35 -04:00

(Migrated from github.com)

Alright just added another commit to this pull request. Did a bit more refactoring as well as added a new searchender command and added more outputting for config conversion.

I'm pretty sure that this pull request is ready to be merged however I'd like it if you could please just go over it to make sure there are no mistakes or I have missed out anything.

With the config migration part, I just want to make sure that you're happy with how it performs the update. As in, backing up the old config and then converting all possible usernames to UUID's and so on so forth.

Other than that I think it's almost good to go.

Alright just added another commit to this pull request. Did a bit more refactoring as well as added a new searchender command and added more outputting for config conversion. I'm pretty sure that this pull request is ready to be merged however I'd like it if you could please just go over it to make sure there are no mistakes or I have missed out anything. With the config migration part, I just want to make sure that you're happy with how it performs the update. As in, backing up the old config and then converting all possible usernames to UUID's and so on so forth. Other than that I think it's almost good to go.

lishid commented

2015-06-24 14:38:39 -04:00

(Migrated from github.com)

Looks good to me.

ShadowRanger commented

2015-06-25 08:00:55 -04:00

(Migrated from github.com)

Disregard the last comments I made on here that I deleted. I changed my mind about it and don't think we need to make a whole separate version for 1.7.10 compatibility.

lishid commented

2015-06-25 14:10:25 -04:00

(Migrated from github.com)

We could release a single version for earlier version compatibility if the work involves minimal changes to the code other than swapping out the craftbukkit jar. Not totally sure that's the case, but either way I wouldn't want to promise maintaining a compatible version in the future.

lishid commented

2015-06-25 14:11:52 -04:00

(Migrated from github.com)

Wait, 1.7.10 is super old already, right? It's like a year old or something...

ShadowRanger commented

2015-06-25 19:32:53 -04:00

(Migrated from github.com)

Yeah it is very old that's why I changed my mind and now don't think we should do that.

lishid commented

2015-06-25 19:33:23 -04:00

(Migrated from github.com)

That's fair enough.

ShadowRanger commented

2015-06-25 19:35:06 -04:00

(Migrated from github.com)

Plus the guy who asked for it just commented calling the people on the comments 'assholes' in Russian.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: minster586/OpenInv#27