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

Add custom preference key #54

Closed
ArvidNy opened this issue Jan 14, 2019 · 4 comments
Closed

Add custom preference key #54

ArvidNy opened this issue Jan 14, 2019 · 4 comments
Labels
enhancement New feature or request

Comments

@ArvidNy
Copy link
Contributor

ArvidNy commented Jan 14, 2019

In our current setup, we have chosen not to use the Java Preferences API for various reasons. For instance, we want our advance users to be able to manually edit/add settings and we want users to be able to send their settings file to us in case we have problems recreating an issue. I found out you have the option to add a custom StorageHandler which is great for us, since it allows us to store the settings wherever we want. However, we already have a key for each setting that we would prefer to keep using instead of the breadcrumb hash.

I know the breadcrumb approach has been discussed before and that it is a question of the maximum amount of characters, but I would like to propose an option to make the breadcrumb optional instead of replacing it. First I was thinking about adding it as another variable to the constructors, but I realized it would mean a lot of additional lines and it might risk confusing anyone. So my suggestion would be to add a method in the Settings class called setCustomPreferenceKey(String key) or something like that and in the method saveSettingsValue change the line to look like this: storageHandler.saveObject(key.isEmpty() ? getBreadcrumb() : key, value.getValue()); That should prevent users from accidentally setting it and it would use the breadcrumb approach unless something else is deliberately set.

For our project it would mean that we could reuse our existing preference file quite easily, but I'm sure more users would appreciate the option to set humanly readable keys. I just wanted to check with you if you would be willing to consider something like that if I created a PR for it and if you have anything else you think I need to take into account first in that case?

@martinfrancois
Copy link
Contributor

Hi @ArvidNy, thanks for your question!
It seems to me that there is a misunderstanding. You mention the "breadcrumb hash", this is an implementation detail that is specific to the default storage handler. In fact, if you're implementing your own StorageHandler, the breadcrumb you get passed in the methods is reflecting the path to the setting including the setting name, delimited with #. So for example My Category#My Group#My Setting, which makes the breadcrumb in fact human readable. Should you want to use a different delimiter, you could simply replace it or split the breadcrumb by it to do any kinds of custom processing you want, before storing it. The hashing is necessary when using the Preferences API, but if you're saving to a file, you can simply use the breadcrumb and not hash it.
Would that be enough to satisfy your use case or do you still feel like you would be missing some part I didn't mention?

@ArvidNy
Copy link
Contributor Author

ArvidNy commented Jan 15, 2019

Thanks for the reply! Well, the main idea behind my suggestion is for us to keep using the keys we already have, instead of relying on the breadcrumb (hashed or not). You are right about the hash approach taking place in the StorageHandler, so sorry for leading you off with that. We haven't named our settings after a predictable pattern, so being able to pass it directly to the Setting and from there to the custom StorageHandler would be ideal for us. This is an example of the keys we have in our settings file.

stage.mode=screen
item.theme.override=true
small.song.text.v.position=bottom
max.font.size=139.3625498007968

The usage I'm looking for is in other words something like this:
Setting.of("Allow custom item themes to override global theme", new SimpleBooleanProperty(true)).setCustomKey("item.theme.override")

That would lead to the following additions in the Setting class:

private String key = "";
  public Setting setCustomKey(String key) {
    this.key = key;
    return this;
  }

And changing the following method (and similar changes to the loadSettingValue method):

public void saveSettingValue(StorageHandler storageHandler) {
    storageHandler.saveObject(key.isEmpty() ? getBreadcrumb() : key, value.getValue());
  }

That would help us moving over to this library a lot! Otherwise we would have to map each key to its corresponding breadcrumb and that would mean we would have to maintain that as well in case we move a setting to another category or add it to a group. As for the humanly readable argument, what I meant was that in case anyone else want to use the Preference API without the hash approach, they also might want to set their own shorter keys that does not rely on the breadcrumb.

@martinfrancois
Copy link
Contributor

Nevermind about that, thanks for the perfect explanation, now I fully understand what you mean!

I see the case now and I agree that it would also present a good addition, especially to help people migrate to PreferencesFX!

The only thing I would suggest would be to name the method customKey instead of setCustomKey, to fit the fluent API a bit better.

Feel free to implement it and submit a PR!

@martinfrancois martinfrancois added the enhancement New feature or request label Jan 21, 2019
@martinfrancois
Copy link
Contributor

Implemented in #60

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

No branches or pull requests

2 participants