search

Minecrell / ServerListPlus

A flexible Minecraft plugin to customize the appearance of your server in the server list
https://git.io/slp
GNU General Public License v3.0
236 stars 60 forks source link

Create FaviconCache class to reduce code duplication #354

Closed mangstadt closed 3 years ago

mangstadt commented 3 years ago

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.

stephan-gh commented 3 years ago

FWIW, the reason why I didn't reply to this one yet isn't because of

The cache code is really old and to be absolutely honest I don't fully remember how it works exactly either, so I would kind of prefer to leave it alone so things don't explode

but rather because I wonder, if you want to do this refactoring, shouldn't you go one step further?

The getFaviconCache() and reloadFaviconCache() methods seem kind of pointless with this refactoring, you might as well change ServerListPlusPlugin to return your new FaviconCache class directly and let that implement deleting/clearing the underlying LoadingCache.

I wanted to give some more specific suggestions but I'm a bit short of time for all this Minecraft stuff :)

mangstadt commented 3 years ago

Sure! I'll see what I can do!

mangstadt commented 3 years ago

I see a potential complication. Making this change would involve making changes to the ServerListPlusCore.CACHE_TYPES field (a map that deals with Cache objects).

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.

stephan-gh commented 3 years ago

Ugh, well I guess you could change the favicon one to be like return core.getPlugin().getFaviconCache().getLoadingCacheThatIsStoredSomewhereInTheFaviconCache();?

mangstadt commented 3 years ago

Oh, duh. Yes, that would certainly work. Did not even think of that!

stephan-gh commented 3 years ago

Hmm yes closer but what I was thinking about basically was to:

  1. Create the FaviconCache once in every plugin (could be private final FaviconCache<...> faviconCache = new ...)
  2. Have the LoadingCache nullable inside FaviconCache
  3. Implement the reloadFaviconCache() in FaviconCache so you can get rid of that duplication

Does that make sense?

mangstadt commented 3 years ago

Ah yes, I understand.

My question is: Why is LoadingCache set to null at all? Wouldn't it be better to immediately assign a new instance to the variable instead? Setting it to null just opens up the potential for NPEs.

stephan-gh commented 3 years ago

My question is: Why is LoadingCache set to null at all? Wouldn't it be better to immediately assign a new instance to the variable instead? Setting it to null just opens up the potential for NPEs.

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 clear() call should already get rid of most of the garbage and there is no real need to ever create a new instance at all. IMO.

stephan-gh commented 3 years ago

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

stephan-gh commented 3 years ago

+184 −185

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?

mangstadt commented 3 years ago

My pleasure! I'm not sure how I'd go about doing that. I could write a unit test for FaviconCache if that would help?

stephan-gh commented 3 years ago

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 Server sub-project. It's kind of convenient for testing.

mangstadt commented 3 years ago

Oh gosh that sounds daunting. For now, I tested it using the Server sub-project.

When I try to run the ServerListPlus-3.5.0-Server.jar file (after running gradlew), I get a "no main manifest attribute" error (MANIFEST.MF in the JAR is empty for me). However, I was able to run the standalone server directly from source via my IDE.

I found and fixed a null pointer error (see commit). After fixing that, the favicon loaded successfully for me.

stephan-gh commented 3 years ago

Oh gosh that sounds daunting.

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.

When I try to run the ServerListPlus-3.5.0-Server.jar file (after running gradlew), I get a "no main manifest attribute" error (MANIFEST.MF in the JAR is empty for me).

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

I found and fixed a null pointer error (see commit).

Heh yes, I ran into those several times already as well. It was a terrible idea to do all the initialization in the ServerListPlusCore constructor :P

mangstadt commented 3 years ago

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

Ah, I was using build/libs/ServerListPlus-3.5.0-SNAPSHOT.jar, my bad!

I'll give creating a Minecraft server a shot. Is there any particular server you recommend?

stephan-gh commented 3 years ago

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.

mangstadt commented 3 years ago

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 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. :)

mangstadt commented 3 years ago

I managed to get Velocity up and running. Favicon loads without issue.

stephan-gh commented 3 years ago

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.

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. :)

mangstadt commented 3 years ago

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.

Thanks for the suggestion! I'll have to look around some more.

Anyway, thanks a lot for getting this PR into good shape!

My pleasure, thanks for the awesome feedback!