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

External changes of preference values not possible #8

Open
finanzer opened this issue May 17, 2018 · 2 comments
Open

External changes of preference values not possible #8

finanzer opened this issue May 17, 2018 · 2 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@finanzer
Copy link
Contributor

Now the preference values will be read from storage everytime the method show() is called. That makes it impossible to change a value somewhere else in the application, because a new value will b overriden by storageHandler. Is it really usefull to read the values everytime when the dialog is shown? I think the values are already in memory and could be used directly.

@martinfrancois
Copy link
Contributor

Thanks for your comment! During the implementation, we thought of PreferencesFX being the central and only place where these values would be influenced, hence why we didn't think of this. But of course you are absolutely right, we should introduce some kind of check and apply externally changed values without overriding them with the saved settings upon opening.
Also concerning reading the values every time the dialog is shown, this for sure isn't something that is necessary. Feel free to submit a PR to fix these issues!

@martinfrancois martinfrancois added enhancement New feature or request help wanted Extra attention is needed labels Jun 18, 2018
@ghost
Copy link

ghost commented Dec 18, 2020

A related item is StorageHandler imposes that implementations provide Preferences getPreferences(). This makes sense for a PreferencesBasedStorageHandler but does not make sense for an XmlStorageHandler where the backing store is Apache Commons XMLConfiguration, rather than a Preferences object.

Recommend updating the API to remove the following accessor method from the StorageHandler interface:

 Preferences getPreferences();

If needed, another interface, such as PreferencesStorageHandler, could extend from StorageHandler to offer any Preferences-specific definitions. Then PreferencesBasedStorageHandler could implement PreferencesStorageHandler, leaving StorageHandler with an API that doesn't "force" a particular storage mechanism implementation. (Likewise, clearPreferences could be renamed to clear to decouple the concrete concept of Preferences from the interface altogether.)

On that note, StorageHandler should be agnostic to UI settings as well. Currently, methods such as saveDividerPosition, loadDividerPosition, saveWindowWidth, loadWindowWidth, and so forth, all require the implementation to provide functionality for these methods. It would be more flexible to extract these into their own interface so that developers who don't care whether the PreferencesFX UI dialog persists its dimensions and layout need not implement such methods when creating their own implementation(s) of StorageHandler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants