Skip to content
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

WIP: Persistent preferences #76

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

PhilippvK
Copy link
Owner

Tests do not work at the moment.

TODOs:

  • Check how to handle incomplete Preferences (Only Server/Port exists,...)

Partially resolves Issue #75

TODO: Also save language setting after it was added to the Launcher
Dialog
Warning: currently incomplete (TODO: fix)
@@ -49,6 +55,42 @@ public static boolean isUsingCustomServer() {
return true;//instance.serverBox.isSelected();
}

public void loadPreferences() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move all this Preferences logic into a separate class and add getters/setters for individual properties, this way we can handle all the logic for one property being unknown there. It will be also easier to test, since we could be mocking individual methods in that new class instead of the entire properties.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getters could return Optional and the individual getters could work like this:

  • Is the property in loaded properties?
    • Yes, return the property
    • Not, return empty Optional

@PhilippvK
Copy link
Owner Author

Sorry but I have to delay further work to a later point in time. I am currently not able to invest time in the project. Sorry for that!

@pehala
Copy link
Collaborator

pehala commented Dec 3, 2020

Its fine, I am currently in the same situation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants