Create Pref Service - #102
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
|
Thank you for this carefully considered and long needed update! Please get rid of PrefService#setStaticBehavior. I feel that it "infects" the PrefService with legacy details. This method does not need to be public API, since the deprecated Prefs class itself already has the relevant public API method: Prefs#setDelegateService. Secondly, please purge references to the Java Preferences API from the PrefService interface contract. If we want the SJC PrefService to be implementation-agnostic, we cannot pass along java.util.Preferences objects as method arguments, nor as return values. We have two options here:
If you need further guidance on how to update the PrefService API to accomplish this, just let me know; happy to chat further. All in all, this is a great update. I just want to be extra careful to "get it right" this time! |
Sorry, something went wrong.
|
@ctrueden good points on both. Agreed on the setStaticBehavior method - it was leftover from an early implementation when I thought it was necessary, but is easily folded into the initialize method. Thanks for pointing out the java.util.Preferences imports. The purpose of them in the Prefs class is to get Preferences for specific Classes, as I understand it. So I refactored the PrefService API replacing the Preferences signatures with Class-based equivalents. Let me know if there are any more issues, otherwise I'm happy with the patch. |
Sorry, something went wrong.
|
@ctrueden well I didn't check silly little details like whether this compiles or not (it doesn't). The Prefs class itself is broken with this change.. working on fixing it. |
Sorry, something went wrong.
|
@ctrueden ok, now everything compiles and passes tests as one would expect.. ready for final review |
Sorry, something went wrong.
|
I am loath to point this out, but... there are no unit tests for PrefService. I realize there weren't for Prefs, so in the interest of time, perhaps we should just file an issue for it. Also, regarding the package name: how about org.scijava.prefs? It seems odd to me to use the long form for the package name (preferences) but the short name for the service (PrefService). We have net.imagej.ops with an OpService so I think org.scijava.prefs with a PrefService would be pleasantly consistent. |
Sorry, something went wrong.
|
Also, this PR isn't auto-mergeable right now. Needs a rebase over latest master. |
Sorry, something went wrong.
Using a static utility method for Prefs is limiting - it provides no way to override preference saving/loading behavior. This creates problems when there are errors or negative interactions with the Preferences implementation - for example, in MATLAB there are known bugs regarding the Java Preferences implementation. By converting to a PrefService we can now control this behavior more precisely, as needed. The Prefs utility class will remain for now, but will delegate to the PrefService when it has been created - allowing the utility method behavior to be overridden similarly.
All Contextual classes that used the Prefs static utility class should now be using PrefService.
Added API to save/load ModuleItems. This will allow Contextual classes to move away from ModuleItem#loadValue and ModuleItem#saveValue, which are not Contextual and go directly through the Prefs utility class. Adds a default implementation in DefaultModuleService, using the PrefService if able (which requires an AbstractModuleItem). If necessary, still falls back to ModuleItem's saveValue and loadValue methods.
ModuleItem#saveValue and ModuleItem#loadValue are now deprecated in favor of ModuleService methods, which can be called from a Contextual environment and thus use the new PrefService API.
Updated classes that were using ModuleItem#saveValue and ModuleItem#loadValue to use the appropriate ModuleService methods.
It was discovered that the initialize method of multiple PrefService impls was being invoked - leading to the LOWEST priority overwriting the static service field in the Prefs utility class (since it was the last initialize method to execute). Added a check for priority so that lower priority services don't overwrite higher.
Appled Source > Clean Up in Eclipse, to update newest classes with the IJ2 style template.
Added comments detailing planned future for the pref service.
The Prefs utility class is now deprecated. PrefService should be used instead.
This was an unnecessary method. Originally thought that each service would need a reminder to set itself as the static PrefService... but that's exactly what the initialize method of AbstractPrefService is for.
The whole point of the PrefService is to allow for any preference implementation, while the DefaultPrefService uses java.util.Preferences. Thus the java.util.Preferences imports have been removed from the PrefService, and the necessary Class and String-based indexing methods have been added. Updated the DefaultPrefService implementation and usage of the service accordingly.
We have a short name PrefService, so we should use a short package name instead of a long package name.
The Prefs class is deprecated and we should not reference it in any way from non-deprecated code.
Added suppress warnings for the Prefs deprecation.
Migrates the static utility `Prefs` class to an extensible `PrefService`.
Moves from a static utility Prefs class to a PrefService with default implementation.
All functionality is the same, running through java.lang.prefs.Preferences.
The Prefs class itself is retained but deprecated. It will delegate to a PrefService instead. Note that, because the Prefs class has no Context and lacks a nice way to obtain it, it will default to the DefaultPrefService until another service is set as the static PrefService of choice.
To facilitate this, the AbstractPrefService sets this static field.
As part of this patch, I also had to add ModuleService API to save/load ModuleItem values, as these were Context-less entities using the Prefs class.