Create FaviconCache class to reduce code duplication #354
Conversation
This class wraps a "LoadingCache" object, which is used by each of the plugins to cache favicons. As far as I can tell, the "FaviconHelper.loadSafely" method is only used by this favicon caching code, so I integrated the code from this method into the new FaviconCache class and deleted the method.
|
FWIW, the reason why I didn't reply to this one yet isn't because of
but rather because I wonder, if you want to do this refactoring, shouldn't you go one step further? The I wanted to give some more specific suggestions but I'm a bit short of time for all this Minecraft stuff :) |
|
Sure! I'll see what I can do! |
|
I see a potential complication. Making this change would involve making changes to the ServerListPlusCore.CACHE_TYPES field (a map that deals with I don't know if this falls into the "too complicated" category, but I can work on it. The solution might involve getting rid of the CACHE_TYPES map altogether. |
|
Ugh, well I guess you could change the favicon one to be like |
|
Oh, duh. Yes, that would certainly work. Did not even think of that! |
|
Hmm yes closer but what I was thinking about basically was to:
Does that make sense? |
|
Ah yes, I understand. My question is: Why is |
SLP contains some funny code from when I thought I was much smarter than the Java runtime. I guess back then I thought this would save some memory or whatever. Actually, the |
|
Oh wait, I get it. I guess this is supposed to handle the case when the cache configuration is changed so the cache is recreated with the new configuration. Doesn't help that I removed at least the public part of the cache configuration long ago and almost no one will change it because you need to manually add it back to the configuration :D |
Bukkit/src/main/java/net/minecrell/serverlistplus/bukkit/BukkitPlugin.java
Outdated
Show resolved
Hide resolved
Bungee/src/main/java/net/minecrell/serverlistplus/bungee/BungeePlugin.java
Outdated
Show resolved
Hide resolved
* Set `ServerListPlusCore` field in constructor instead of setter method * Clear cache by calling `FaviconCache.clear()` instead of passing null into `reloadFaviconCache` * Rename `ServerListPlusPlugin.reloadFaviconCache` to `createFaviconCache` See: Minecrell#354 (comment)
…ntains` method back to `getIfPresent`
Bungee/src/main/java/net/minecrell/serverlistplus/bungee/BungeePlugin.java
Outdated
Show resolved
Hide resolved
Server/src/main/java/net/minecrell/serverlistplus/server/ServerListPlusServer.java
Outdated
Show resolved
Hide resolved
Sponge/src/main/java/net/minecrell/serverlistplus/sponge/SpongePlugin.java
Outdated
Show resolved
Hide resolved
Velocity/src/main/java/net/minecrell/serverlistplus/velocity/VelocityPlugin.java
Outdated
Show resolved
Hide resolved
Server/src/main/java/net/minecrell/serverlistplus/server/ServerListPlusServer.java
Outdated
Show resolved
Hide resolved
|
So, I'd say this was worth it :D It looks good to me now, thanks for making all the changes. Have you tested this on one of the implementations at least? |
|
My pleasure! I'm not sure how I'd go about doing that. I could write a unit test for |
|
No I mean, compile and install the plugin on some server and see if it still loads and setting some favicon works (the wiki has examples for this). :) If you're lazy you could try the standalone server in the |
The `ServerListPlusCore` constructor takes a reference to a plugin. Inside of this constructor, the plugin's `createFaviconCache` method is called. In the case of the `ServerListPlusServer` plugin, this method assumes that the plugin's `ServerListPlusCore` field is non-null, which was a wrong assumption (the `ServerListPlusCore` object's constructor is still being executed). This caused a null reference to be passed into the `FaviconCache` constructor. The solution is to pass the plugin object into the `FaviconCache` constructor so that `ServerListPlusPlugin.getCore()` can be called.
|
Oh gosh that sounds daunting. For now, I tested it using the Server sub-project. When I try to run the I found and fixed a null pointer error (see commit). After fixing that, the favicon loaded successfully for me. |
Well, I'm afraid that's the typical development flow for many MC-related plugins. For some platforms there are like Gradle plugins that make starting a test server more easy so you don't need to do that manually. And of course ideally you would write so many unit and integration tests that manual testing is not necessary. But I suspect most MC-related projects are not written that way.
Are you sure you started the correct JAR? Kind of strange. The one from CI looks fine: https://ci.codemc.io/job/Minecrell/job/ServerListPlus/lastSuccessfulBuild/artifact/Server/build/libs/ServerListPlus-3.5.0-SNAPSHOT-Server.jar
Heh yes, I ran into those several times already as well. It was a terrible idea to do all the initialization in the |
Ah, I was using I'll give creating a Minecraft server a shot. Is there any particular server you recommend? |
Tbh I'm really curious how you decided to contribute to this if you have (I guess?) never used ServerListPlus before or even set up a MC server? :D I don't really want to suggest any of them specifically (except the standalone server, which is entirely made by myself :D). The proxy-based ones (Bungee and Velocity) are probably really easy to start with SLP installed, but you won't be able to join the server (which isn't a problem since SLP is only for the server list entry). The others are all actual MC mods that provide a full MC server. In terms of SLP functionality, all of them should be equivalent and work equally well. That's really one of the key features of SLP. |
I found your project by browsing GitHub by Java projects and sorting by recently updated. Your project happened to be near the top of the list. Based on the number of stars, it looked popular but not super popular (no offense), so I thought I'd have a better chance of making a contribution. I've been away from the full-time programming profession for a while, and the handful of open source projects I maintain are pretty stable. So I haven't been coding much lately and wanted to get back into it. Also, most of my projects have been solo affairs, so I thought it would be good to get some collaboration experience. Like you, I used to play Minecraft, but don't play it much anymore. Some years ago I created a Java Swing app for my favorite Minecraft server that helps you manage your in-game shop transactions (the server has a player-run economy). I still use it to check up on my shop (which still has customers :D). And whenever the server updates to a new Minecraft version, I like to login and update my shop with the new items. :) |
|
I managed to get Velocity up and running. Favicon loads without issue. |
Huh, that was a pretty strange coincidence given that I haven't been making much changes to SLP for the last years. :D FWIW, it might be even a bit more fun to contribute to a more "modern", actively maintained project where you don't get told "This is old code so I'd rather not change it more than necessary". ;) There are lots of cool open-source out there, just need to search for a bit. Anyway, thanks a lot for getting this PR into good shape! I will merge this one and close the other one, as discussed. :) |
Thanks for the suggestion! I'll have to look around some more.
My pleasure, thanks for the awesome feedback! |
This new class wraps a "LoadingCache" object, which is used by each of the plugins to cache favicons.
As far as I can tell, the "FaviconHelper.loadSafely" method is only used by this favicon caching code, so I integrated the code from this method into the new FaviconCache class and deleted the method.