Added FCManager and support for FC serialization #3118

vampcat · 2017-10-04T20:04:41Z

Closes issue #2893

This PR primarily deals with adding a new FlexibleConfigManager class, that is responsible for managing all the FCs, as well as saving/loading them.

GooeyHub · 2017-10-04T20:12:47Z

Hooray Jenkins reported success with all tests good!

GooeyHub · 2017-10-04T20:24:50Z

Hooray Jenkins reported success with all tests good!

oniatus · 2017-10-05T19:10:40Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfig.java

+
+    Map<SimpleUri, String> getUnusedSettings();
+
+    Map<SimpleUri, Setting> getActiveSettings();


I don't understand the difference between an unused and an active setting from the api, maybe add some javadoc to explain the concept?

I think some javadoc is warranted indeed, but I would also work on the terms used.

I think "settingsFromFile" and simply "settings" would go some way to mitigate this issue.
Other options might be "temporarilyParkedSettings" which is pretty accurate if I think about it.

I was planning on doing that after the code stabilizes a bit, but I'll add some comments explaining the basic logic for reviewers' benifit.

oniatus · 2017-10-05T19:14:04Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigImpl.java

+            if (unusedSettings.containsKey(id)) {
+                setting.setValueFromString(unusedSettings.remove(id));
+            }
+        }


This could be an unexpected sideeffect because a caller of fc.add(setting) could expect that an add call keeps the value of the setting the same.

I think this will need to be clarified in the method's javadoc @oniatus, as it is the intended behavior.

The config file is loaded in memory, but its values aren't immediately imported into an existing FC - the config file contains too little information for that, i.e. it doesn't contain settings types. Instead, as systems add settings to the FCs (on startup or on game startup), IF a corresponding value is found in the file, that value overrides the setting's default.

Concrete example: display resolution. When the engine attempts to add, on startup, a display resolution setting to the appropriate FC instance, the corresponding setting instance is indeed added. But if the FC finds a value for it in the file, it will reset the setting to that value. Otherwise the setting maintains the default value.

I reiterate this happens on startup or on game startups, when modules are loaded. Nothing should add settings at any other time - another notion we might want to make clear in the javadocs.

I'm not sure how else we can handle the saving/loading part.

Can we make it clear in the javadocs that adding a settings will loads the non-default value that was saved earlier, and that if you want the default value, you must call .resetValue() right after?

I guess you can, but I don't think we should encourage anybody to override a user's choice without his or her consent.

My previous comment wasn't quite concrete enough. I'd say: yes document the behaviour in the javadoc, no do not suggest to call resetValue() if necessary - it shouldn't be necessary unless the user wants it.

oniatus · 2017-10-05T19:14:44Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigImpl.java

+            return jsonObject;
+        }
+    }
+    */


dead lines, can be removed if not needed.

oniatus · 2017-10-05T19:15:08Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

+import org.terasology.engine.SimpleUri;
+
+public interface FlexibleConfigManager {
+    void addFlexibleConfig(SimpleUri flexibleConfigUri, FlexibleConfig fc);


use flexibleConfig for the second parameter, similar to the first one.

the interface could define the behavior when two configs with the same uri are added (throw RTE)

I second what @oniatus is saying and further refine it with the following declaration:

void addFlexibleConfig(SimpleUri configId, FlexibleConfig config) throws RuntimeException;

Potentially we could also use UnsupportedOperationException, to be a bit more specific. Or we could go with a custom exception such as DuplicateConfigException.

Also, I'd rename the method to simply addConfig.

When you say "the config can define the behaviour", you mean I should add a throws RuntimeException here?

Okay, so based on a quick tour of the rest of the FC code, the throws clause is added in the javadocs everywhere, and nowhere in the declaration.

Adding it in the code here makes things inconsistent - although we can can obviously go back and add it to the code everywhere.
Thoughts?

Sorry for the delay about this.

I think in this case it is warranted for the interface to show the exception thrown as it is part of the intended behavior of this method AND it diverges from the behavior of a typical map that would just overwrite the previously existing value for a given key.

I had not considered the second part - the divergence. Definitely makes way more sense to me now :D

oniatus · 2017-10-05T19:19:12Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManagerImpl.java

+            gson = new GsonBuilder().setPrettyPrinting().create();
+        }
+
+        return gson;


I don't think lazy initialization provides a benefit here. Gson is not that expensive to build and also not that expensive to keep.
I would inline the initialization logic to the field and use direct field access instead.

oniatus · 2017-10-05T19:22:32Z

engine/src/main/java/org/terasology/config/flexible/SettingImpl.java

+            value = (T)(Double)Double.parseDouble(valueString);
+        } else if (value instanceof String) {
+            value = (T)valueString;
+        } else {


I don't like this block for three reasons:

It violates OCP because every new type needs a code change

The double cast looks odd

It supports only int, float, double and string but this is only visible in the code

I don't have a better alternative right now but I am sure we will find one :)

Yeah, I don't like this part of the code either - one of the first that I plan on reworking, as soon as i get a bit of free time.

I agree with @oniatus assessment.

I now have the even stronger feeling we might want to go back to a Setting class that is abstract, -not- generic-based, and is the base for classes such as FloatSetting, IntSetting etc.

I guess one alternative would be delegate the parsing to type-specific parsing objects which can be registered somewhere at runtime and retrieved/used in this method.

Any other possibility?

"delegate the parsing to type-specific parsing objects which can be registered somewhere at runtime"
Sounds a lot like gson adapters - can't say I'm the biggest fan of that.

I've implemented a hybrid generic-and-concerete solution which I feel is a decent compromise. Have a look at it.

oniatus · 2017-10-05T19:23:56Z

engine/src/main/java/org/terasology/engine/subsystem/common/ConfigurationSubsystem.java

+        FlexibleConfig renderingFlexibleConfig = new FlexibleConfigImpl();
+        flexibleConfigManager.addFlexibleConfig(new SimpleUri("engine:rendering"), renderingFlexibleConfig);
+
+        flexibleConfigManager.load();


can't this be part of config.load()? We already have an integration to config.save()

One thing we'll have to be careful about is that FCs get initialized -before- config gets loaded. Not sure if this is possible for every single config.

I think the aim is to eventually eliminate Config entirely and leave only the FCmanager to fill those shoes. So, I think it makes sense to keep them in parallel for the time being.

emanuele3d

Quite a few issues I'm afraid, but various are deeper architectural issues that would have been hard to foresee earlier and is good to discuss in this format.

emanuele3d · 2017-10-05T20:26:41Z

engine/src/main/java/org/terasology/config/Config.java

@@ -142,6 +144,8 @@ public void save() {
        } catch (IOException e) {
            logger.error("Failed to save config", e);
        }
+
+        CoreRegistry.get(FlexibleConfigManager.class).save();


An instance of this config class is created in ConfigurationSubsystem. Please, let's pass the Context to it, on construction, so that we don't have to use the CoreRegistry.

emanuele3d · 2017-10-05T20:28:59Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfig.java

+
+    Map<SimpleUri, String> getUnusedSettings();
+
+    Map<SimpleUri, Setting> getActiveSettings();


I think some javadoc is warranted indeed, but I would also work on the terms used.

I think "settingsFromFile" and simply "settings" would go some way to mitigate this issue.
Other options might be "temporarilyParkedSettings" which is pretty accurate if I think about it.

emanuele3d · 2017-10-05T20:42:57Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigImpl.java

+            if (unusedSettings.containsKey(id)) {
+                setting.setValueFromString(unusedSettings.remove(id));
+            }
+        }


I think this will need to be clarified in the method's javadoc @oniatus, as it is the intended behavior.

The config file is loaded in memory, but its values aren't immediately imported into an existing FC - the config file contains too little information for that, i.e. it doesn't contain settings types. Instead, as systems add settings to the FCs (on startup or on game startup), IF a corresponding value is found in the file, that value overrides the setting's default.

Concrete example: display resolution. When the engine attempts to add, on startup, a display resolution setting to the appropriate FC instance, the corresponding setting instance is indeed added. But if the FC finds a value for it in the file, it will reset the setting to that value. Otherwise the setting maintains the default value.

I reiterate this happens on startup or on game startups, when modules are loaded. Nothing should add settings at any other time - another notion we might want to make clear in the javadocs.

emanuele3d · 2017-10-05T20:51:08Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigImpl.java

@@ -91,4 +101,37 @@ public boolean remove(SimpleUri id) {
    public boolean contains(SimpleUri id) {
        return settingMap.containsKey(id);
    }
+
+    @Override
+    public void setUnusedSettings(Map<SimpleUri, String> unusedSettings) {


I'm thinking this method should be private. Only the FC should be responsible for reading its config file and setting this internal object. Nothing external should be able to do it.

emanuele3d · 2017-10-05T20:57:06Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

+import org.terasology.engine.SimpleUri;
+
+public interface FlexibleConfigManager {
+    void addFlexibleConfig(SimpleUri flexibleConfigUri, FlexibleConfig fc);


I second what @oniatus is saying and further refine it with the following declaration:

void addFlexibleConfig(SimpleUri configId, FlexibleConfig config) throws RuntimeException;

Potentially we could also use UnsupportedOperationException, to be a bit more specific. Or we could go with a custom exception such as DuplicateConfigException.

emanuele3d · 2017-10-05T21:52:14Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManagerImpl.java

+                    }
+                }
+
+                // Add all the non-default settings that were not used in this session


I'd change to:
// Add any temporarily parked setting that hasn't been used in this session of the application.

That been said, I wonder if that's a valid behavior. It could potentially lead to the accumulation of "zombie" settings that have been added to an FC once and are never used again. Is this a valid scenario? How should we react to it?

This is like old save files - we can't delete them based on suspicion of being a zombie, since they might very well be a vampire instead.

I think we should leave this untouched, since I'd rather live with 10 extra settings than lose all my settings because I did not load a module (and hence it's associated settings) for (say) 10 consecutive games.

Hmmmm. I wonder if only the engine should add settings to an engine config and only modules should add settings to their very own module config. This should eliminate zombie settings as configs are atomically loaded and atomically discarded.

Programmatically this might not have consequences, but when it's time to group settings, potentially from different configs, into a single UI, that's where the uiPath idea would become a necessity.

What do you think?

This should eliminate zombie settings as configs are atomically loaded and atomically discarded.

I'm sure I'm missing something, but I can't see how different config files can prevent zombie settings from piling up. (The only possible way I can see is deleting config associated with myModule when user deletes myModule, but that's not a normal scenario, since I don't expect user to be deleting modules all the time.)

emanuele3d · 2017-10-05T22:02:29Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManagerImpl.java

+    }
+
+    private Path getPathForFlexibleConfig(SimpleUri flexibleConfigUri) {
+        Path filePath = PathManager.getInstance().getHomePath().resolve("configs").resolve(flexibleConfigUri.getModuleName().toString()).resolve(flexibleConfigUri.getObjectName().toString() + ".cfg");


Wowowowow. This line is too short. Can you make it any longer please? 😄

At least let's split it over multiple lines, i.e.

...getHomePath.resolve(blah) .resolve(blahblah) .resolve(blahblahblah)

emanuele3d · 2017-10-05T22:17:59Z

engine/src/main/java/org/terasology/config/flexible/SettingImpl.java

+            value = (T)(Double)Double.parseDouble(valueString);
+        } else if (value instanceof String) {
+            value = (T)valueString;
+        } else {


I agree with @oniatus assessment.

I now have the even stronger feeling we might want to go back to a Setting class that is abstract, -not- generic-based, and is the base for classes such as FloatSetting, IntSetting etc.

I guess one alternative would be delegate the parsing to type-specific parsing objects which can be registered somewhere at runtime and retrieved/used in this method.

Any other possibility?

emanuele3d · 2017-10-05T22:23:48Z

engine/src/main/java/org/terasology/engine/subsystem/common/ConfigurationSubsystem.java

+        FlexibleConfig renderingFlexibleConfig = new FlexibleConfigImpl();
+        flexibleConfigManager.addFlexibleConfig(new SimpleUri("engine:rendering"), renderingFlexibleConfig);
+
+        flexibleConfigManager.load();


I think the aim is to eventually eliminate Config entirely and leave only the FCmanager to fill those shoes. So, I think it makes sense to keep them in parallel for the time being.

emanuele3d · 2017-10-05T22:41:29Z

engine/src/main/java/org/terasology/engine/subsystem/common/ConfigurationSubsystem.java

+        FlexibleConfigManager flexibleConfigManager = new FlexibleConfigManagerImpl();
+        rootContext.put(FlexibleConfigManager.class, flexibleConfigManager);
+
+        FlexibleConfig renderingFlexibleConfig = new FlexibleConfigImpl();


Hmmm. I'm thinking that there might be reasons to split the existing rendering config in two separate configs, one for generic, always needed settings, i.e. resolution, and another for all those settings that kick in only in-game.

The only problem I see is that this would mean some settings become visible to the user only in-game, which is not quite the typical behavior of settings: you can normally tweak them before you enter the game proper.

On the other hand, given what @vampcat and I have been discussing about render nodes providing their own settings to the RenderingFC, at this stage we only instantiate those nodes when the renderer is alive, which leads anyway to this behavior.

I'm definitely up for the split.

And I do think the behaviour of not showing settings in the main menu is the desired one, at least in the long run.

I don't see how else we can do it admittedly. But it will be a bit unusual I feel. Perhaps on loading/creating a game we'll need a brief alert, fading in/out near the bottom of the screen, informing the user that additional settings have become available, perhaps with a checkbox so that the user doesn't have to see the message every time.

Yeah, the alert seems like a logical idea. That should be easily doable via NUI I think.

( To be fair, since I haven't dived into it too much, I might be totally and horribly wrong :D )

GooeyHub · 2017-10-06T12:08:54Z

Uh oh, something went wrong with the build. Need to check on that

emanuele3d · 2017-10-12T13:17:34Z

@vampcat, please let me know when you think it's time to review the latest changes.

GooeyHub · 2017-10-15T12:56:47Z

Uh oh, something went wrong with the build. Need to check on that

vampcat · 2017-10-15T13:00:15Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigImpl.java

-        this.temporarilyParkedSettings = Maps.newHashMap();
-    }
+    private Map<SimpleUri, Setting> settings = Maps.newHashMap();
+    private Map<SimpleUri, String> temporarilyParkedSettings = Maps.newHashMap();


Morale behind shifting this here: When something is initialized like this, it's clear at the first glance that it can never be null. However, if it's initialized in the constructor, that raises the question of whether this can ever be null, and whether checks for the same should be added.

(P.S. I could not find any real reason to prefer one style over the other.)

...hmmm... the code could still assign null (intentionally or by mistake) to such variable.

It's no big deal though, let's not worry about this anymore.

True. I was sorely tempted to make it final as well in the same line, but I refrained.

Not a big deal, but can't deny that I am tempted :D

Admittedly, why not final?

vampcat · 2017-10-15T14:37:09Z

I have no clue how I broke the build system 😓

emanuele3d · 2017-10-17T14:22:48Z

@Cervator? Can you have a look at the outcome of the build? It doesn't look like something this PR should be capable of generating.

emanuele3d

A number of left-over, small issues but we are getting there.

emanuele3d · 2017-10-17T15:03:24Z

engine/src/main/java/org/terasology/config/flexible/settings/StringSetting.java

+import org.terasology.config.flexible.validators.SettingValueValidator;
+import org.terasology.engine.SimpleUri;
+
+public class StringSetting extends SettingImpl<String> {


I see you made String, Float, Integer and Double settings. I would suggest to add BooleanSetting and perhaps LongSetting. Byte, Char and Short would complete the list but I feel they are probably a bit overkill for our needs. I wouldn't say no if you added them though... 😉

emanuele3d · 2017-10-17T15:06:36Z

engine/src/main/java/org/terasology/config/flexible/settings/DoubleSetting.java

+import org.terasology.config.flexible.validators.SettingValueValidator;
+import org.terasology.engine.SimpleUri;
+
+public class DoubleSetting extends SettingImpl<Double> {


I'm guessing there is no way to eliminate the SettingImpl class without a lot of almost identical code (save for the type), right?

As far as I know, that seems to be the case.

emanuele3d · 2017-10-17T15:21:22Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfig.java


-    Map<SimpleUri, Setting> getActiveSettings();
+    void load(Reader reader);


Are the reader/writer passed here because it is not possible to change their path once they have been instantiated?

The configManager is responsible for deciding the path of serialization, creating a reader/writer for it and passing that to the FC.

This way, the FC does not have to concern itself with where the settings are being stored, and can focus only on the how.

I understand that. But could the writer/reader be passed on Construction? If a path change requires re-instantiation, I guess the answer is no.

emanuele3d · 2017-10-17T15:23:48Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigImpl.java

+            if (unusedSettings.containsKey(id)) {
+                setting.setValueFromString(unusedSettings.remove(id));
+            }
+        }


My previous comment wasn't quite concrete enough. I'd say: yes document the behaviour in the javadoc, no do not suggest to call resetValue() if necessary - it shouldn't be necessary unless the user wants it.

emanuele3d · 2017-10-17T15:26:59Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManagerImpl.java

            } catch (IOException e) {
-                logger.error("Failed to load config file {}, falling back on default config");
+                // TODO: Handle exception


Yes please. 🙂

emanuele3d · 2017-10-17T15:28:08Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigImpl.java

+
+            jsonReader.endObject();
+        } catch (Exception e) {
+            // TODO: Handle exception


Yes please. 🙂

emanuele3d · 2017-10-17T15:31:28Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

@@ -18,11 +18,11 @@
 import org.terasology.engine.SimpleUri;

 public interface FlexibleConfigManager {
-    void addFlexibleConfig(SimpleUri flexibleConfigUri, FlexibleConfig flexibleConfig);
+    void addConfig(SimpleUri configUri, FlexibleConfig config) throws RuntimeException;


Sorry if I said otherwise, but this configURIs are really configIDs, so I'd rename all instances of configUri to configId, to stick to what it is rather than its type.

GooeyHub · 2017-10-18T21:46:22Z

Uh oh, something went wrong with the build. Need to check on that

emanuele3d

Looking good. Mostly just a few changes to the javadocs.

emanuele3d · 2017-10-22T20:28:35Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigImpl.java

@@ -138,7 +136,7 @@ public void load(Reader reader) {

            jsonReader.endObject();
        } catch (Exception e) {
-            // TODO: Handle exception
+            throw new RuntimeException("Error parsing config file!");


Hmmm... I imagine this prints out the whole stack trace normally. But will this include any hint of where the parsing went wrong? It would be important to have that information printed out.

And can the catch any more precise? I.e. see:

https://www.javaworld.com/article/2077500/testing-debugging/java-tip-134--when-catching-exceptions--don-t-cast-your-net-too-wide.html

I restructured the code slightly to print the name of the config that failed to parse, and printed the stack trace of that exception as well - that should cover the "where" part.

emanuele3d · 2017-10-22T20:39:00Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfig.java

+     * All the non-default values are loaded and "parked" till the actual setting is added to the config,
+     * and which point they are "unparked" and parsed, replacing the default value of the setting.
+     *
+     * Note that this function should be called before adding any settings to the FlexibleConfig.


I'd expand on this a little and clarify what happens if this isn't the case, i.e. settings are overwritten.

Reminder about this one.

More specifically we could write:

Note that this function should be called before adding any settings to the FlexibleConfig, or any corresponding parked setting will never be unparked.

emanuele3d · 2017-10-22T20:47:10Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfig.java

+    /**
+     * Loads the values of the settings having non-default values, to enable persistence across sessions.
+     *
+     * All the non-default values are loaded and "parked" till the actual setting is added to the config,


* All the non-default values are loaded and "parked", initially remaining inaccessible. * Once a Setting object is added to the config, a corresponding non-default value is sought * among the parked values. If one is found it is parsed and stored in the Setting object.

emanuele3d · 2017-10-22T20:55:40Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

@@ -17,14 +17,46 @@

 import org.terasology.engine.SimpleUri;

+/**
+ * Stores multiple {@link FlexibleConfig} instances that can be retrieved using their id.
+ * Also responsible for overseeing their serialization - to and from - disk.


I'd maybe write "coordinating" instead of "overseeing".

emanuele3d · 2017-10-22T20:57:01Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

 public interface FlexibleConfigManager {
+    /**
+     * Adds the given {@link FlexibleConfig} to this manager.
+     * This method throws a RuntimeException if a FlexibleConfig with the given id already exists.


I think you can move the content of this line and put it besides the @throws line below, isn't it?

emanuele3d · 2017-10-22T20:58:54Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

+     * Removes the config associated with the given id, and returns it.
+     *
+     * @param configId The id of the config to remove.
+     * @return The config associated with the given id, and null if no config is associated with the given id.


The config associated with the given id, or null if no config is associated with the given id.

emanuele3d · 2017-10-22T20:59:43Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

+     * Retrieves the config associated with the given id.
+     *
+     * @param configId The id of the config to retrieve.
+     * @return The config associated with the given id.


We should clarify what happens if there is no config associated with the given id.

Reminder about this one.

Cervator · 2017-10-23T00:00:32Z

Here's the failure from Jenkins:

:modules:Core:processResources UP-TO-DATE
:modules:Core:classes
:modules:Core:checkstyleMain
:modules:Core:compileTestJava/home/jenkins/workspace/TerasologyPRs/modules/Core/src/test/java/org/terasology/world/generator/TreeTests.java:60: error: constructor Config in class Config cannot be applied to given types;
        context.put(Config.class, new Config());
                                  ^
  required: Context
  found: no arguments
  reason: actual and formal argument lists differ in length
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
1 error
 FAILED

So in other words it sounds like a test class is failing to compile due to the changes. Which is a common issue and easy to miss since the test classes may not auto-compile when tinkering with the regular Java source.

Doesn't look like it would be a very difficult fix either. Try it out and force-compile the test source and/or try running all the ~~engine~~ Core tests to see if they're okay :-)

vampcat · 2017-10-25T22:18:16Z

@emanuele3d Passing the Context into Config to avoid the use of CoreRegistry (which is temporary, and will be removed once we're done finding a proper place to put FC.save() instead of inside Config.save()) has caused me to change code in a lot of places. I personally don't mind too much, but I think it's worth reconsidering once.

GooeyHub · 2017-10-25T22:22:51Z

Hooray Jenkins reported success with all tests good!

emanuele3d · 2017-10-27T08:05:38Z

@vampcat wrote:

Passing the Context into Config to avoid the use of CoreRegistry (which is temporary, and will be removed once we're done finding a proper place to put FC.save() instead of inside Config.save()) has caused me to change code in a lot of places. I personally don't mind too much, but I think it's worth reconsidering once.

Are you talking about changes that I haven't seen yet? Because so far the changes related to the context being passed in didn't seem so bad. And what would a proper place for FC.save() be?

vampcat · 2017-10-29T20:32:39Z

No, I just meant the changes I have pushed (not sure if you've viewed them all, when saying "the ones i've viewed"). However, that does mean that a lot of tests have to pass null to the config, which feels weird..

Although now that I think about it, it would have been better to simply add a new constructor that takes no arguments, that automatically sets the variable to null.

Your thoughts?

emanuele3d · 2017-10-29T21:47:37Z

Tests are meant to also provide a hint of how things are used in practice. If in practice we should always pass a Context to the constructor then we should do it that way in the tests too. So, I'd avoid a new constructor in this case.

Indeed passing null when we should pass an object is not ideal though. Ideally we'd pass a MockContext object.

You didn't respond about where else you'd put FC.save() by the way.

vampcat · 2017-10-29T21:50:24Z

For the time being, nowhere. However, since FC will replace Config either way, we'll soon replace all config.save() calls with fcManager.save() calls.

emanuele3d · 2017-10-29T22:06:21Z

I'm skeptical about that "soon". 😉 But ok for the rest.

oniatus · 2017-11-05T09:18:35Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfig.java

+     *
+     * @param reader A reader that will serve as the source of the settings.
+     */
+    void load(Reader reader);


I would separate loading and saving from managing settings. Changing and reading setting values and load/save are two different responsibilities for different clients and the strategy how to save a setting (io.Reader and io.Writer) are implementation details which should not be part of this interface.

Actually, I had the loading/saving login in FCManager before, but @emanuele3d commented the following:

I am thinking that in terms of load/save functionality, it makes sense that the manager orchestrates it, i.e. iterating over the FCs as it happens here and providing the file or even the reader. But I'd want the FCs themselves to handle the actual reading/writing, to separate the concerns and mantain some of the functionality private to the FC class.
Which is why I put this here.

I'm fine with either, so can you two decide which feels better? :D

I'd keep it as it is for the time being. Let's get this PR moving again and merged in. We can always refactor after we have gained some experience with FCs in general.

oniatus · 2017-11-05T09:20:30Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

+
+/**
+ * Stores multiple {@link FlexibleConfig} instances that can be retrieved using their id.
+ * Also responsible for overseeing their serialization - to and from - disk.


In addition to my previos comment about save/load, I'd expect a method like save(FlexibleConfig) and include the reader/writer logic here.

Hmmmm... I'm not sure I agree with this and the related comment above.

Can you do a comparison between the two different ways of doing things and show why the changes you suggest would be better?

oniatus · 2017-11-05T09:23:28Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

+     * Iterates over all the configs registered with this manager, and loads the
+     * settings stored in them.
+     */
+    void load();


Just by reading FlexibleConfigManager.load() -> what will it do?
How about FlexibleConfigManager.loadAllRegisteredConfigs()?

Also naming consistency -> javadoc calls it "registered", the method to register is called "add".

I'd recommend to rename load to loadAllConfigs().

Fair point about consistency. I'd retain the add method and change the javadoc.

oniatus · 2017-11-05T09:23:36Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

+     * Iterates over all the configs registered with this manager, and saves the
+     * settings stored in them.
+     */
+    void save();


same as above

oniatus · 2017-11-05T09:26:30Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManagerImpl.java

+import java.util.Map.Entry;
+
+public class FlexibleConfigManagerImpl implements FlexibleConfigManager {
+    private static final Logger logger = LoggerFactory.getLogger(FlexibleConfigManager.class);


This should be a logger for the -Impl class

oniatus · 2017-11-05T09:29:18Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManagerImpl.java

+            try (Reader reader = Files.newBufferedReader(getPathForFlexibleConfig(flexibleConfigId), TerasologyConstants.CHARSET)) {
+                flexibleConfig.load(reader);
+            } catch (NoSuchFileException e) {
+                // The config does not exist, so the default values will be used.


Exceptions should not be part of the expected controlflow.
Use java.nio.files.Files.exists(Path) to check if the file at the path exists and ignore this case explicitly

Hmmm... I'm not sure about the dogma implied in the first sentence. 😉
But the fix sounds reasonable enough. =)

GooeyHub · 2017-12-04T18:01:54Z

Hooray Jenkins reported success with all tests good!

emanuele3d

A couple of javadoc-related reminders/suggestions, but from my perspective we are pretty much good to go.

emanuele3d · 2017-12-12T21:21:35Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfig.java

+     * All the non-default values are loaded and "parked" till the actual setting is added to the config,
+     * and which point they are "unparked" and parsed, replacing the default value of the setting.
+     *
+     * Note that this function should be called before adding any settings to the FlexibleConfig.


More specifically we could write:

Note that this function should be called before adding any settings to the FlexibleConfig, or any corresponding parked setting will never be unparked.

emanuele3d · 2017-12-12T21:22:39Z

engine/src/main/java/org/terasology/config/flexible/FlexibleConfigManager.java

+     * Retrieves the config associated with the given id.
+     *
+     * @param configId The id of the config to retrieve.
+     * @return The config associated with the given id.


Reminder about this one.

GooeyHub · 2017-12-12T21:43:28Z

Hooray Jenkins reported success with all tests good!

vampcat · 2017-12-13T12:19:54Z

Btw, how do you feel about creating a new MockContext object? I really dislike the Context(null) constructor i've had to use, so I'd be glad to replace it with MockContext in a small PR.

GooeyHub · 2017-12-13T12:27:51Z

Hooray Jenkins reported success with all tests good!

emanuele3d · 2017-12-13T12:38:50Z

Yes, to use a MockContext would be better, as I suggested in an earlier comment. I'd do it in this PR though: it is in this PR that a context is introduced as input to the constructor, therefore it is in this PR that ideally the tests should be modified accordingly. It's not a big deal though: if you feel strongly about doing it in a separate PR, ok.

GooeyHub · 2017-12-13T13:01:23Z

Hooray Jenkins reported success with all tests good!

oniatus · 2017-12-13T19:50:02Z

Approved, next change should use the new system for at least one of our config concepts to put some usage on the new Api 👍

emanuele3d · 2017-12-13T23:10:30Z

@oniatus: indeed the idea is to start using FCs in the context of the Renderer, at least. As the two Rendering configs have a lot of parameters we are unlikely to migrate everything to FCs in one step, but that's the direction we are aiming for.

emanuele3d · 2017-12-13T23:10:57Z

@Cervator: this PR is good to go!

emanuele3d · 2017-12-20T14:54:28Z

Poking @Cervator for merge. Unless there is something in this PR that doesn't quite fully convince him?

vampcat · 2017-12-20T15:28:57Z

Cervator is currently flying to Europe or something, so I don't think he'll be available a whole lot for a few days. That being said, I'll try poking him on slack (since he tends to ignore all github pings).

oniatus · 2017-12-20T20:00:54Z

Went for a second review and gave it another playtest to make sure everything works.
Good work, I am looking forward to upcoming changes in order to improve the unflexible configs 👍

emanuele3d · 2017-12-21T11:18:04Z

@oniatus, I know you have merge privileges (so do I) and I appreciate the good intention. However, please do not merge PRs in which I am involved unless explicitly requested. There are some type of PR that @Cervator prefers to handle himself.

oniatus · 2017-12-21T11:51:00Z

@emanuele3d Sorry for that, we should clarify the review/merge process then because I did not know this till now.

emanuele3d · 2017-12-21T14:03:56Z

No worries, I do realize this has never been thoroughly discussed and responsibilities within the merge-privileged have not been clearly defined.

There is no doubt Cervator could use some help. But perhaps we can help with the least controversial, least consequential PRs first, leaving to him the more significant ones, especially in light of the fact that he tests them against his "mega-workspace", an IJ project he has that includes all existing modules, to spot conflicts that normally would go unnoticed.

That been said, it is certainly good we have this conversation so that we can clarify things. Maybe he'd be happy to relax the workflow and let a number of other people deal with any PR.

Cervator · 2018-02-02T20:02:21Z

Belated response to this here. Interesting situation with the merging - I really would love more help merging, and honestly with this amount of PRs some bumps and bruises might be worth it. So I'm thrilled @oniatus stepped in :-)

At the same time @emanuele3d couldn't have picked better timing with this PR as an example for the usefulness of a mega-workspace as it did in fact break something super subtle, a test class in PolyWorld that had Config config = new Config();

I only found it from Alpha 9 prep after sorting out an API violation that slipped through and is documented at #3184. That was keeping my mega-workspace from doing full builds, so I might not have caught this anyway. To make things more perfect-storm'ish I had built every single module as part of release prep just 3 days before this got merged, making me puzzled as Jenkins was still happy ... :D

Had me confused for a bit until I finally realized commit ee0d74a#diff-dbff4de92b8b73995ac93deb86683778 from here added a new constructor for Config meaning the default one went away, which is in fact also an API violation, woo! :-) That is a tricky one to catch

Both API violations seemingly only caused one tiny module break each, fixing both right now. A fancier setup in Jenkins would have caught both and done away with the need for mega workspaces. In the meantime with the new Groovy utility making such a workspace is easier so maybe a few beyond me can keep such a workspace around for testing tricky PRs (but nothing else - keep a separate dir for it and don't do any work there, aiiee can they get tricky to keep clean)

Thanks for everybody's effort and initiative here, please keep at it, even if the road may get bumpy from time to time. So long as we test before doing an actual release we can still fix issues like this and stay in compliance with SemVer, so even if we get a few PR merges with slight issues that still keeps us moving and OK.

Onward and upwards!

emanuele3d requested review from Cervator, emanuele3d and oniatus October 4, 2017 20:06

emanuele3d assigned vampcat Oct 4, 2017

emanuele3d added the Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. label Oct 4, 2017

oniatus suggested changes Oct 5, 2017

View reviewed changes

emanuele3d suggested changes Oct 5, 2017

View reviewed changes

vampcat commented Oct 15, 2017

View reviewed changes

emanuele3d suggested changes Oct 17, 2017

View reviewed changes

emanuele3d suggested changes Oct 22, 2017

View reviewed changes

oniatus suggested changes Nov 5, 2017

View reviewed changes

emanuele3d suggested changes Dec 12, 2017

View reviewed changes

emanuele3d and others added 4 commits December 13, 2017 15:22

Fixed minor issue in javadoc.

7f94e76

Minor improvements to FlexibleConfigManager's javadoc.

59b5274

Merge conflicts

e169c60

Moar merge conflicts!

adbd098

vampcat force-pushed the fcSerialization branch from 7ca067d to adbd098 Compare December 13, 2017 12:18

Added new MockContext class

86ae30f

emanuele3d approved these changes Dec 13, 2017

View reviewed changes

oniatus approved these changes Dec 13, 2017

View reviewed changes

oniatus merged commit 6eaf991 into MovingBlocks:develop Dec 20, 2017

eviltak mentioned this pull request Jan 15, 2018

FlexibleConfig improvements #3227

Merged

4 tasks

Cervator added the API label Feb 2, 2018

Cervator added a commit to Terasology/PolyWorld that referenced this pull request Feb 2, 2018

API fix for MovingBlocks/Terasology#3118 and tiny code quality tweaks

120e882

Cervator mentioned this pull request Feb 2, 2018

API fix for #3184 and minor code quality tweaks #3247

Merged

Cervator added this to the Alpha 9 milestone Feb 7, 2018

vampcat deleted the fcSerialization branch February 18, 2018 13:46

skaldarnar removed the Api label May 15, 2021


		Map<SimpleUri, String> getUnusedSettings();

		Map<SimpleUri, Setting> getActiveSettings();


		Map<SimpleUri, Setting> getActiveSettings();
		void load(Reader reader);

Added FCManager and support for FC serialization #3118

Added FCManager and support for FC serialization #3118

Conversation

vampcat commented Oct 4, 2017

GooeyHub commented Oct 4, 2017

GooeyHub commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vampcat Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vampcat Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vampcat Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vampcat Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emanuele3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vampcat Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vampcat Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emanuele3d Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooeyHub commented Oct 6, 2017

emanuele3d commented Oct 12, 2017

GooeyHub commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vampcat commented Oct 15, 2017

emanuele3d commented Oct 17, 2017

emanuele3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooeyHub commented Oct 18, 2017

emanuele3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vampcat Oct 5, 2017 •

edited

Loading

vampcat Oct 5, 2017 •

edited

Loading

vampcat Oct 5, 2017 •

edited

Loading

vampcat Oct 5, 2017 •

edited

Loading

vampcat Oct 6, 2017 •

edited

Loading

vampcat Oct 17, 2017 •

edited

Loading

emanuele3d Oct 17, 2017 •

edited

Loading