eclipse-theia / theia

Eclipse Theia is a cloud & desktop IDE framework implemented in TypeScript.
http://theia-ide.org
Eclipse Public License 2.0
20.06k stars 2.5k forks source link

Align Notebook preferences registration with other preferences #13913

Closed cdamus closed 3 months ago

cdamus commented 4 months ago

Feature Description:

The registration of the Notebook preferences, added in this pull request, looks like this:

bind(PreferenceContribution).toConstantValue({ schema: notebookPreferenceSchema });

This is different to the usual pattern of other established preferences, which would have looked more like this:

export const NotebookPreferenceContribution = Symbol('NotebookPreferenceContribution');
bind(NotebookPreferenceContribution).toConstantValue({ schema: notebookPreferenceSchema });
bind(PreferenceContribution).toService(NotebookPreferenceContribution);

In my application, importantly, this previously established pattern of Inversify bindings lets me effectively suppress preferences that I don't want to expose to my users because they are not relevant (the capabilities configured by these preferences aren't manifest in my app at all). This is accomplished by rebinding individual preferences service-identifiers with blanks:

rebind(NotebookPreferenceContribution).toConstantValue({
        schema: <PreferenceSchema>{
            type: 'object',
            properties: {}
        }
    });

This I cannot do with the Notebook preferences. I can only rebind the entire PreferenceContribution service-identifier, which means repeating the contributions of all of the preferences from Theia that my application does need.

So the request in this issue is to rework the Notebook preference contribution to follow the pattern previously established by other Theia preferences, unless there is some specific reason why it wasn't done that way?

cdamus commented 4 months ago

FYI @jonah-iden for any insight you'd like to offer on this question. Thanks!

jonah-iden commented 4 months ago

@cdamus there is no specific reason for the way notebooks does it. I probably just forgot about the ability to overwrite this when I implemented it. So feel free to create a PR to change this or I can create one

cdamus commented 4 months ago

@cdamus there is no specific reason for the way notebooks does it. I probably just forgot about the ability to overwrite this when I implemented it. So feel free to create a PR to change this or I can create one

Thanks! Good to know. I've created a PR, as you can see linked. I would certainly appreciate a review and buddy-testing 😀 .

jonah-iden commented 4 months ago

sure I'll try and take a look later today

jonah-iden commented 4 months ago

@cdamus PR is approved. If you need this fix urgently i can trigger the build of a theia-next release for you after the PR is merged

cdamus commented 4 months ago

Thanks @jonah-iden ! It is not so urgent as to require an interim/test build. I am happy to wait for the next regularly scheduled release.