assets MUST NOT be loaded in static code

[andidevi]

There are issues when loading asses in static code as that is exposed to a race condition. Loading assets with SkinLoader must not happen before class Klooni is initiated. In other cases Gdx.graphics will be null. It happens that this condition is accidently met for the current state of the code.

The order classes are loaded and initialized is subject to the class loader involved and that may change in the future, especially as this is an Android app and Google is experimenting much with it's runtime.

There are more reason to not load assets in static code, ie. error handling and the like.

Example of issues (initiate effects in static code from class Klooni):
SkinLoader static
Exception in thread "main" java.lang.ExceptionInInitializerError
at io.github.lonamiwebs.klooni.effects.WaterdropEffectFactory.(WaterdropEffectFactory.java:38)
at io.github.lonamiwebs.klooni.Klooni.(Klooni.java:46)
at io.github.lonamiwebs.klooni.desktop.DesktopLauncher.main(DesktopLauncher.java:34)
Caused by: java.lang.NullPointerException
at io.github.lonamiwebs.klooni.SkinLoader.(SkinLoader.java:43)
... 3 more

[Lonami]

Is this still an issue after #43 got merged? (I see you moved the Waterdrop static initialization into the constructor which is nice).

Also regarding the FIXME you introduced:

Klooni1010/core/src/io/github/lonamiwebs/klooni/Klooni.java

Lines 46 to 47 in ef2fb8f

	// FIXME theme should NOT be static as it might load textures which will expose it to the race condition iff GDX got initialized before or not
	public static Theme theme;

Sure, it's static, but it's not loading anything. It defaults to null, and is loaded later on create:

Klooni1010/core/src/io/github/lonamiwebs/klooni/Klooni.java

Lines 82 to 95 in ef2fb8f

	public void create() {
	onDesktop = Gdx.app.getType().equals(Application.ApplicationType.Desktop);
	prefs = Gdx.app.getPreferences("io.github.lonamiwebs.klooni.game");
	// Load the best match for the skin (depending on the device screen dimensions)
	skin = SkinLoader.loadSkin();
	// Use only one instance for the theme, so anyone using it uses the most up-to-date
	Theme.skin = skin; // Not the best idea
	final String themeName = prefs.getString("themeName", "default");
	if (Theme.exists(themeName))
	theme = Theme.getTheme(themeName);
	else
	theme = Theme.getTheme("default");

So, is this still wrong?

[andidevi]

The text is according to your TODO that have been there before. I did'n have any look into it beside the code of SkinLoader.

As to to the comment in Line 89 says it is a singleton, you should not allow public write access to it and use a public getter ... and have a look if that's really a performance hit or not ;)

As it isn't loaded that early there's no need for static at all. If other classes need access, they should simply know Klooni. This would lead to more refactoring, ie. making the cellTexture a parameter of Cell.draw or supplying it at cell creation and changing the parameter of set to the already resolved texture to having Cells and maybe other to not needing to know Klooni.

Alternatively you could have a private static variable to store the Klooni instance and supply it with a public static getter.

... much changes and I don't see a practical benefit now. As a big difference to effects you only have one Theme class and there's no need to change as long as that one size fits all.