`WorkspacePreferenceProvider` Events Inconsistent

[colin-grant-work]

Bug Description:

The WorkspacePreferenceProvider only fires change events when a saved workspace (workspace file) is opened. It does not fire change events when a single-root workspace is open, when the WorkspacePreferenceProvider delegates to the FolderPreferencseProvider. That means that the WorkspacePreferenceProvider breaks its contract much of the time - it cannot be trusted to provide the user information about changes to preferences in the workspace.

Reproduction

1. Create a service that subscribes to onDidPreferencesChanged events from the WorkspacePreferenceProvider (in my use case, a TaskConfigurationModel using the WorkspacePreferenceProvider)
2. Open a single-root workspace
3. Set a preference in PreferenceScope.Workspace (using the UI, the WorkspacePreferenceProvider directly, or the PreferenceService)
4. Observe that the preference change is successful (e.g. check the preferences UI or subscribe to an event from the PreferencesService)
5. Observe that no onDidPreferencesChanged event is fired by the WorkspacePreferenceProvider even though a workspace-scoped preference has changed.

Additional Information

https://github.com/eclipse-theia/theia/blob/39887bf9d05c29fedc9466db76ca0a58706a1a2a/packages/preferences/src/browser/workspace-preference-provider.ts#L58-L73

The intention of the instanceof check is to reduce duplicate events - especially important given the number of events the preference system fires - but it means that any user of the WorkspacePreferenceProvider has to be aware of its implementation and duplicate its logic. Any user must know that if the workspace is unsaved, they can only subscribe to events from the FolderPreferencesProvider rather than the WorkspacePreferenceProvider. In effect, any user of the WorkspacePreferenceProvider has to do the same check and delegation that the WorkspacePreferenceProvider does.

• Operating System: All
• Theia Version: 1.9.0

[colin-grant-work]

I see a couple of potential solutions:

1. Make Emitters smart enough to prevent double subscription (normal JS emitters have this feature - a second subscription with the same handler is a no-op), have the WorkspacePreferenceProvider track its subscriptions and pass them to the FolderPreferenceProvider when it delegates to that, or handle them itself when it is using a WorkspaceFilePreferenceProvider. That way, any user of the services who applies the same handler to events from both the FolderPreferenceProvider and the WorkspacePreferenceProvider would only 'hear' the event once.
2. Since the FolderPreferencesProvider will always exist and be active in both single-root and multi-root workspaces, assign it the responsibilities currently handled by the WorkspacePreferenceProvider - in a multi-root workspace, it would create a WorkspaceFilePreferenceProvider and pass events from that. Since it would always be responsible for firing events for both scopes, it could prevent duplicates without suppressing any event completely. This would still benefit from making Emitters ignore double subscriptions, since the FolderPreferencesProvider would be bound to both PreferenceScope.Folder and PreferenceScope.Workspace and might be injected and have its events subscribed to under both names.

[vince-fugnitto]

I would have thought the following:

Provider	Responsibility	Overrides
UserPreferenceProvider	responsible for user preferences	overrides default
WorkspaceFilePreferenceProvider	responsible for workspace file preferences (multi-root workspaces)	overrides user
WorkspacePreferenceProvider	responsible for workspace preferences (non multi-root workspaces)	overrides user
FolderPreferencesProvider	responsible for folder preferences (only multi-root workspaces)	overrides workspace

It seems that this is not the behavior you noticed.

I see that FolderPreferencesProvider can be used as a delegate for WorkspacePreferenceProvider (added in the following commit https://github.com/eclipse-theia/theia/commit/d43112331461e7bad5ad7049594e5010ad0192c3):

https://github.com/eclipse-theia/theia/blob/39887bf9d05c29fedc9466db76ca0a58706a1a2a/packages/preferences/src/browser/folder-preference-provider.ts#L49-L55

[colin-grant-work]

The precedence you describe is how the PreferenceService determines how to apply preferences, but there are quirks that are internal to the WorkspacePreferenceProvider. As you observe, it delegates to the FolderPreferencesProvider in a single-root (unsaved) workspace. In that case, folder scope and workspace scope are the same, so the WorkspacePreferenceProvider just piggy backs on the functionality of the FolderPreferencesProvider - which works perfectly well for setting and retrieving preferences - but it also suppresses its own events, and that means that it no longer really satisfies its own API: it is silent in cases when subscribers expect an event to fire. So subscribers have to know when they can and can't rely on events from that provider, and they have to ensure that they have direct access to the FolderPreferencesProvider to get access to the events the WorkspacePreferenceProvider suppresses.

[vince-fugnitto]

@colin-grant-work the only thing I'm not sure about is using the specific providers directly, in theory we should not care about who was ultimately delegated to provide the event. Should we not instead use the generic preferenceService.onPreferenceChanged event at a requested scope to achieve what we want?

https://github.com/eclipse-theia/theia/blob/39887bf9d05c29fedc9466db76ca0a58706a1a2a/packages/core/src/browser/preferences/preference-service.ts#L159-L166

https://github.com/eclipse-theia/theia/blob/39887bf9d05c29fedc9466db76ca0a58706a1a2a/packages/core/src/browser/preferences/preference-service.ts#L34-L56

[colin-grant-work]

In general, I think that should work, but it isn't how the TaskConfigurationManager has been designed - it currently accesses the UserPreferenceProvider and FolderPreferencesProvider directly, and I stumbled on this unexpected behavior when I tried to use the WorkspacePreferenceProvider in a similar way. If it's intended that the providers should only be accessed through the PreferenceService, then perhaps the individual providers should be bound such that they aren't accessible outside of the service at all, so that no one is tempted to use them in ways they aren't designed for. At the moment it looks like only the PreferenceService and TaskConfigurationManager are trying to access them directly, so it might not be impossible to refactor the TaskConfigurationManager (and its subsidiaries) not to rely on particular providers, if that's more consistent with the intended architecture - I'd have to take a closer look at exactly how the task configurations are accessed and managed. My impression is that there are parts of the task system that are accessing the files directly (using information obtained from the individual providers) in ways that might better be left to the preference system instead.

[vince-fugnitto]

@colin-grant-work I'm not sure what the intention is about using providers directly, but from the looks of it anyone that does (ex: tasks) will not work correctly due to the bug you mentioned.

For example, when a TaskConfigurationModel is initialized with a specific provider, it will not be notified of all possible events it should be interested in from what you described:

https://github.com/eclipse-theia/theia/blob/39887bf9d05c29fedc9466db76ca0a58706a1a2a/packages/task/src/browser/task-configuration-model.ts#L38-L49

[colin-grant-work]

Exactly the case I'm working with 😄