-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: 1:1 migration from LauncherSettings >>> Settings #667
Conversation
This prepares `Settings` to take over the role of `LauncherSettings` as source of truth for the launcher settings. The information is stored in JavaFX properties wrapping the actual value. The use of properties allows for reactive bindings to simplify automatic updates of the UI. When storig a (legacy) `LauncherSettings` object we also deserialize an equivalent `Settings` object to JSON. Vice versa, we can also read `Settings` from a JSON file. The new `Settings` object is not yet used.
…ad of file Since it is the decision of `Settings` what file types to write, the file path should not be passed in from the outside. Instead, just the folder path can be controlled. As a consequence, the exact file name(s) are an implementation detail and no longer need to be exposed by `Settings`.
This does not (yet) make use of any feature we gain by having JavaFX properties or bindings, but on the upside this should be a feature-equivalent migration to the new settings POJO. I know that the migration is not pretty, but I'd like to improve on the details in separate follow-up PRs to keep the review load bearable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally tests out fine except for a language change not being loaded correctly.
I see it saved in both of the settings files in my ./terasologylauncher
dir, but on start-up the launcher is always using English.
Nitpick: Consider aligning the Settings
variable names to settings
instead of launcherSettings
.
The locale setting indeed brakes with this PR. The reason is that we no longer TerasologyLauncher/src/main/java/org/terasology/launcher/settings/LauncherSettings.java Lines 105 to 115 in 33786f6
I'm currently trying to figure out at which place I'll add the |
@jdrueckert The language issue should be fixed by 286c4d7. I'd appreciate another review 🙃 It would be great to add tests for behavior like this at some point, but I'm not sure whether the current architecture would be a good point to start (a task running on a separate thread updating global state)... |
Changes look fine so I approved it, but there seems to be a test failure? |
There's indeed a failing test in the CI run:
However, tests are running locally for me 😕
I'll re-run the job... |
Contains
This does not (yet) make use of any feature we gain by having JavaFX properties or bindings, but on the upside this should be a feature-equivalent migration to the new settings POJO.
I know that the migration is not pretty, but I'd like to improve on the details in separate follow-up PRs to keep the review load bearable.
Most changes where done with IntelliJ's "Type Migration" and direct migration of getter/setter calls to access of the property value.
How to test
Ensure that all tests are still passing when running
gradlew test
.Start the launcher and play around with the settings.
~/.terasologylauncher/
)Based on #666
Contributes to #522