Move ScopedPreferenceStore to org.eclipse.jface #4262 - #4270
Conversation
…4262 The `IPreferenceStore` interface has two public implementations; A JFace `PreferenceStore` based on _*.properties_ files and an Eclipse 3.x `ScopedPreferenceStore` based on _*.prefs_ files. Consumers who want to use scoped preferences therefore always carry a direct dependency to the 3.x framework, rendering this interface unusable in a pure 4.x application. Consumers currently have to fall back to the `IEclipsePreferences` interface, which is not a drop-in replacement, however, due to a different behavior when it comes to e.g. default values. Because the `ScopedPreferenceStore` doesn't depend on any 3.x-specific functionality, it can be easily moved to the JFace component. The dependency to the Eclipse Runtime bundle can be reduced to the Equinox Preferences bundle by inlining the call to `IPreferenceService.get`. Note that this change has to be split up into two commits; One where the `ScopedPreferenceStore` is moved and one where a dummy implementation is added to the Eclipse Workbench bundle. Otherwise Git is unable to detect the class as being moved to the JFace component and thus doesn't carry over the file history. Contributes to eclipse-platform#4262
…ipse-platform#4262 This adds the `ScopedPreferenceStore` back to the Platform Workbench bundle to restore backwards compatibility. Note that this is done in a separate commit, to make sure that the history of this class now resides with the preference store that has been moved to the JFace component. Closes eclipse-platform#4262
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
There was a problem hiding this comment.
Pull request overview
Moves scoped preference storage into JFace, reducing reliance on the Eclipse 3.x workbench.
Changes:
- Adds the JFace
ScopedPreferenceStoreand Equinox Preferences dependency. - Retains a compatibility wrapper and migrates internal consumers.
- Moves and modernizes the associated tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
PreferencesTestSuite.java |
Removes relocated test. |
ScopedPreferenceStoreTest.java |
Moves test to JFace and JUnit 5. |
AllPrefsTests.java |
Registers relocated test. |
Workbench ScopedPreferenceStore.java |
Adds compatibility wrapper. |
AbstractUIPlugin.java |
Uses JFace implementation. |
PlatformUI.java |
Uses JFace implementation. |
WorkbenchMessages.java |
Removes migrated message field. |
Workbench messages.properties |
Removes migrated message. |
ViewsPreferencePage.java |
Migrates preference-store import. |
WorkbenchChainedTextFontFieldEditor.java |
Migrates preference-store import. |
IDEWorkspacePreferencePage.java |
Migrates preference-store import. |
ChooseWorkspaceData.java |
Migrates preference-store import. |
IDEApplication.java |
Migrates preference-store import. |
GenericEditorPreferencePage.java |
Migrates preference-store type checks. |
Generic Editor MANIFEST.MF |
Increments bundle version. |
TextEditorPreferencePage.java |
Migrates preference-store import. |
JFace ScopedPreferenceStore.java |
Adds scoped preference implementation. |
JFace messages.properties |
Adds validation message. |
JFace MANIFEST.MF |
Adds Equinox Preferences dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * @see org.eclipse.core.runtime.preferences | ||
| * @since 3.40 | ||
| */ | ||
| public class ScopedPreferenceStore extends EventManager implements IPersistentPreferenceStore { |
| * @param includeDefault <code>true</code> if the default context should be | ||
| * included and <code>false</code> otherwise | ||
| * @return IEclipsePreferences[] | ||
| * @since 3.4 public, was added in 3.1 as private method |
| * and {@code true} otherwise | ||
| * @return String or <code>null</code> if the value does not exist. | ||
| */ | ||
| private String internalGet(String key, boolean includeDefault) { |
Test Results 858 files ±0 858 suites ±0 53m 18s ⏱️ + 2m 45s Results for commit f52f4de. ± Comparison against base commit 5b3e6dd. This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both. |
|
Thanks, @ptziegler I would leave 'AbstractUIPlugin.getPreferenceStore()' and 'PlatformUI.getPreferenceStore()' returning the old type for now to avoid class cast exceptions in client code which are casting to it. I also suggest to Deprecate the old type and mark it for deletion. After the deprecation has been around for at least a milestone we could IMHO change 'AbstractUIPlugin.getPreferenceStore()' and 'PlatformUI.getPreferenceStore()' but I would not do this immediately without warning to consumer. |
laeubi
left a comment
There was a problem hiding this comment.
I would fetching the IPreferencesService through org.eclipse.core.internal.preferences.PreferencesService.getDefault() - this is actually internal API but I think in this case acceptable (and we already have other references in platform) and we likely never will change this anyways and would prevent us to duplicate the implementation. Alternatively we could add a method IPreferencesService.get() at Equinox side.
As a nit-pick, the IPreferenceStore javadoc should be updated as it currently mentions only PreferenceStore as the only implementation.
| import org.eclipse.jface.dialogs.IDialogSettings; | ||
| import org.eclipse.jface.dialogs.IDialogSettingsProvider; | ||
| import org.eclipse.jface.preference.IPreferenceStore; | ||
| import org.eclipse.jface.preference.ScopedPreferenceStore; |
There was a problem hiding this comment.
I would also retain the old implementation here.
Instead we should add a static method ScopedPreferenceStore#instance(String id) that return new ScopedPreferenceStore(InstanceScope.INSTANCE, id); and mention it in the javadoc of getPreferenceStore as an alternative for consumer not wanting to use an activator.
To make it even more convenient we could additionally add ScopedPreferenceStore#instance(Class<?> context) and use FrameworkUtil#getBundle to a story from the bundle ID itself (the most common case).
| org.eclipse.equinox.common;bundle-version="[3.18.0,4.0.0)", | ||
| org.eclipse.equinox.bidi;bundle-version="[0.10.0,2.0.0)";resolution:=optional | ||
| org.eclipse.equinox.bidi;bundle-version="[0.10.0,2.0.0)";resolution:=optional, | ||
| org.eclipse.equinox.preferences;bundle-version="[3.12.100,4.0.0)" |
There was a problem hiding this comment.
As ScopedPreferenceStore is the only consumer we can make this optional, even though I generally try to avoid this, it would not introduce any new dependency.
If preferences does not contain a split package, an import-package might be more suitable here.
The
IPreferenceStoreinterface has two public implementations; A JFacePreferenceStorebased on *.properties files and an Eclipse 3.xScopedPreferenceStorebased on *.prefs files. Consumers who want to use scoped preferences therefore always carry a direct dependency to the 3.x framework, rendering this interface unusable in a pure 4.x application.Consumers currently have to fall back to the
IEclipsePreferencesinterface, which is not a drop-in replacement, however, due to a different behavior when it comes to e.g. default values.Because the
ScopedPreferenceStoredoesn't depend on any 3.x-specific functionality, it can be easily moved to the JFace component. The dependency to the Eclipse Runtime bundle can be reduced to the Equinox Preferences bundle by inlining the call toIPreferenceService.get.Note that this change has to be split up into two commits; One where the
ScopedPreferenceStoreis moved and one where a dummy implementation is added to the Eclipse Workbench bundle. Otherwise Git is unable to detect the class as being moved to the JFace component and thus doesn't carry over the file history.Contributes to #4262