Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

preferences for multi-root workspace #3247

Merged
merged 1 commit into from
Feb 4, 2019

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Oct 22, 2018

With changes in 543b119, URIs of root folders in a multi-root workspace are stored in a file with the extension of "theia-workspace", while the workspace preferences are in the ".theia/settings.json" under the first root.

In this pull request, workspace preferences are stored

  • in the workspace file under the "settings" property, if a workspace file exists, or
  • in the ".theia/settings.json" if the workspace data is not saved in a workspace file.

Also, this change supports

  • having 4 levels of preferences (from highest priority to the lowest): FolderPreference, WorkspacePreference, UserPreference, and DefaultPreference, with
  • an updated preference editor that supports viewing & editing the FolderPreferences, WorkspacePreferences, and UserPreferneces.

Signed-off-by: elaihau liang.huang@ericsson.com

@paul-marechal
Copy link
Member

It seems like one of your components that is getting tested with mocha relies on some frontend package, somehow: https://travis-ci.org/theia-ide/theia/jobs/444752934#L2333

The tests run in node, so if some nested import from a chain of "require" happen to import something from phosphor indirectly, it will fail the tests, because phosphor seems to have side effects (code executed on importation) targeting a browser environment.

@akurinnoy
Copy link
Contributor

Hello, @elaihau

I am working on the issue for plug-ins to implement inspect for Configuration API (#2289), so I'm really looking forward to this PR to be merged.

What is the status of this PR? Is it going to be merged soon?

@akosyakov
Copy link
Member

akosyakov commented Nov 5, 2018

What is the status of this PR? Is it going to be merged soon?

Someone should test this PR, and review that there are no breaking changes in API and behavior.

@elaihau
Copy link
Contributor Author

elaihau commented Nov 5, 2018

@akurinnoy this pr is being reviewed by @lmcbout and others from the Ericsson team. You are also welcomed to give us any suggestions :)

hi @akosyakov, this pr is tested by @lmcbout and myself from the functionality standpoint, but i am not sure if we covered all the cases. could someone from typefox do a quick test in near future?

@akosyakov
Copy link
Member

akosyakov commented Jan 28, 2019

browser console full of errors if try to edit setting files:
screen shot 2019-01-28 at 09 53 34

Reproducible by adding files.watcherExclude into workspace preferences.

@elaihau
Copy link
Contributor Author

elaihau commented Jan 28, 2019

browser console full of errors if try to edit setting files:
Reproducible by adding files.watcherExclude into workspace preferences.

Thank you for the review. The bug was fixed.

@elaihau elaihau force-pushed the pref-editor branch 2 times, most recently from 0099db4 to d97cd0c Compare January 28, 2019 19:12
@elaihau elaihau force-pushed the pref-editor branch 2 times, most recently from 986662f to 08c1281 Compare January 30, 2019 20:02
@elaihau
Copy link
Contributor Author

elaihau commented Jan 30, 2019

resolved conflicts between this PR and 6b1a35c

@elaihau
Copy link
Contributor Author

elaihau commented Jan 31, 2019

@akosyakov I noticed that you bumped the version to 0.4.x. Can I also take advantage of the version change and merge this PR in near future ?

@akosyakov
Copy link
Member

akosyakov commented Jan 31, 2019

@elaihau i've looked at the code, thank you for the effort, now it has much less incompatible changes than an original version. It is easier to follow without being afraid that something broken.

Unfortunately, they are still new mandatory arguments, so yes the major version is required. Could you add a record in Change Log under Breaking Changes? I would test tomorrow again and hopefully we can merge afterwards.

@elaihau
Copy link
Contributor Author

elaihau commented Jan 31, 2019

Could you add a record in Change Log under Breaking Changes?

Yes. Done.

I would test tomorrow again and hopefully we can merge afterwards.

Thank you !

CHANGELOG.md Outdated
@@ -15,6 +15,10 @@ Breaking changes:
- `3_move` and `7_actions` replaced by `navigation` group
- editor context menu group changes:
- `2_cut_copy_paste` renamed to `9_cutcopypaste` group
- [core] `PreferenceProvider` [#3247](https://github.com/theia-ide/theia/pull/3247)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would group them under multi-root workspace support for preferences. There are more incompatible changes to what is listed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i updated the list. could you check if there are items missing?

@akosyakov
Copy link
Member

It seems to behave properly. @svenefftinge Can you test it as well?

Copy link
Contributor

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@elaihau
Copy link
Contributor Author

elaihau commented Feb 1, 2019

@benoitf @evidolob could you please check if this PR is ok for plugin developers ? Thank you !

With changes in 543b119, URIs of root folders in a multi-root workspace are stored in a file with the extension of "theia-workspace", while the workspace preferences are in the ".theia/settings.json" under the first root.

In this pull request, workspace preferences are stored
- in the workspace file under the "settings" property, if a workspace file exists, or
- in the ".theia/settings.json" if the workspace data is not saved in a workspace file.

Also, this change supports
- having 4 levels of preferences (from highest priority to the lowest): FolderPreference, WorkspacePreference, UserPreference, and DefaultPreference, with
- an updated preference editor that supports viewing & editing the FolderPreferences, WorkspacePreferences, and UserPreferneces.

- Internally, vsCode uses an enum to define the preference scope. External extensions, however, make contributions to preference schemas by having schemas defined in the package.json, where the preference scope is a string enum of "window", "application", and "resource". This change updates the PreferenceSchema interface defined in theia, to allow contribution points to define scopes with strings.

- Added affects() function to PreferenceChange,  to help client decide whether or not the preference change matters.

Signed-off-by: elaihau <liang.huang@ericsson.com>
@elaihau
Copy link
Contributor Author

elaihau commented Feb 4, 2019

ping @evidolob and @benoitf one more time for approval :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants