Create Pref Service #102

hinerm · 2014-07-21T20:10:47Z

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.

hinerm · 2014-07-21T20:11:50Z

@dscho @ctrueden for your reviewing pleasure. I have tested it downstream in Fiji and MATLAB/MIJ.

ctrueden · 2014-07-23T15:54:26Z

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:

Invent our own org.scijava.prefs.Preferences or similar data structure (sort of like how SJC has its own UI-agnostic versions of all the AWT events), and use it in the API contract.
Change those method signatures to not need such a data structure at all.

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!

hinerm · 2014-07-24T17:20:34Z

@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.

hinerm · 2014-07-24T17:38:08Z

@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.

hinerm · 2014-07-24T18:04:45Z

@ctrueden ok, now everything compiles and passes tests as one would expect.. ready for final review

ctrueden · 2014-07-24T19:59:48Z

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.

ctrueden · 2014-07-24T20:01:07Z

Also, this PR isn't auto-mergeable right now. Needs a rebase over latest master.

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`.

This was referenced Jul 23, 2014

Consolidate options, prefs and config logic imagej/imagej2#95

Open

Convert Prefs to PrefService #96

Closed

This was referenced Jul 25, 2014

Convert PrefService to Handler API #106

Closed

Generalize PrefService API #107

Open

hinerm and others added 16 commits July 25, 2014 10:33

Update Prefs usage to PrefService

4ef6f00

All Contextual classes that used the Prefs static utility class should now be using PrefService.

Mark saveValue and loadValue deprecated

cc97d1b

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.

Refactor for ModuleService saving and loading

d503e31

Updated classes that were using ModuleItem#saveValue and ModuleItem#loadValue to use the appropriate ModuleService methods.

Clean up code

9050328

Appled Source > Clean Up in Eclipse, to update newest classes with the IJ2 style template.

DefaultPrefService: add TODO

6fe0553

Added comments detailing planned future for the pref service.

Deprecate Prefs

f857d0f

The Prefs utility class is now deprecated. PrefService should be used instead.

PrefService: remove setStaticBehavior method

a9a109c

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.

Rename preferences to prefs package

7ff590f

We have a short name PrefService, so we should use a short package name instead of a long package name.

DefaultPrefService: fix javadoc

344d6bc

DefaultPrefService: do not reference Prefs class

963d4eb

The Prefs class is deprecated and we should not reference it in any way from non-deprecated code.

Add down-the-middle unit tests for PrefService

cdeb735

AbstractPrefService: suppress warnings

a0c6c95

Added suppress warnings for the Prefs deprecation.

hinerm added a commit that referenced this pull request Jul 25, 2014

Merge pull request #102 from scijava/create_pref_service

606f9f2

Migrates the static utility `Prefs` class to an extensible `PrefService`.

hinerm merged commit 606f9f2 into master Jul 25, 2014

hinerm deleted the create_pref_service branch July 25, 2014 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Create Pref Service #102

Create Pref Service #102

Uh oh!

hinerm commented Jul 21, 2014

Uh oh!

hinerm commented Jul 21, 2014

Uh oh!

ctrueden commented Jul 23, 2014

Uh oh!

hinerm commented Jul 24, 2014

Uh oh!

hinerm commented Jul 24, 2014

Uh oh!

hinerm commented Jul 24, 2014

Uh oh!

ctrueden commented Jul 24, 2014

Uh oh!

ctrueden commented Jul 24, 2014

Uh oh!

Uh oh!

Create Pref Service #102

Create Pref Service #102

Uh oh!

Conversation

hinerm commented Jul 21, 2014

Uh oh!

hinerm commented Jul 21, 2014

Uh oh!

ctrueden commented Jul 23, 2014

Uh oh!

hinerm commented Jul 24, 2014

Uh oh!

hinerm commented Jul 24, 2014

Uh oh!

hinerm commented Jul 24, 2014

Uh oh!

ctrueden commented Jul 24, 2014

Uh oh!

ctrueden commented Jul 24, 2014

Uh oh!

Uh oh!