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.

Sign up for GitHub

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

Jump to bottom

Fix workspace initialization #248

Merged
merged 18 commits into from
Nov 17, 2023
Merged

Fix workspace initialization #248

merged 18 commits into from
Nov 17, 2023

Conversation

CGNonofr
Copy link
Contributor

@CGNonofr CGNonofr commented Nov 15, 2023 •
edited
Loading

What this PR does:

  • It changes the way services overrides are configured, using the VSCode IWorkbenchConstructionOptions type to configure them all at once. It allows to configure things that are now hardcoded (with a wrong value sometimes).
  • It removes most service-override parameters that are an issue because some service-override includes others and it may override previous given parameters, leading to unpredictable behaviors
  • It fixes the json intellisense not working anymore on settings/keybindings after recent path changes (to match those in VSCode)
  • it fixes editor serialization (2 bugs: 57573db and 0cf5130 and a way to register a custom one 7565dc3) (@CompuIves)
  • it allows to easily use IndexedDB fs provider for user files

It changes the recommended api but there is no breaking changes, only some method parameters marked as deprecated

@CGNonofr CGNonofr requested a review from kaisalmen November 15, 2023 22:58
@CGNonofr CGNonofr mentioned this pull request Nov 15, 2023
Merged
@CGNonofr CGNonofr force-pushed the fix-workspace-initialization branch from e66200e to cc28aa5 Compare November 15, 2023 23:48
@CompuIves
Copy link
Collaborator

Yeah this cleans up a lot! Excited about this change 😄

@CGNonofr
Copy link
Contributor Author

@CompuIves Are you interested in approval rights here?

@kaisalmen I'd like your opinion on those changes as I will impact monaco-languageclient due to the usage of IWorkbenchConstructionOptions

@CompuIves
Copy link
Collaborator

@CompuIves Are you interested in approval rights here?

I am! Happy to help out with reviews.

@CGNonofr CGNonofr force-pushed the fix-workspace-initialization branch from e4104c7 to b8716c6 Compare November 16, 2023 14:45
@CGNonofr CGNonofr requested a review from CompuIves November 16, 2023 16:46
@CGNonofr
Copy link
Contributor Author

@CompuIves feel free to start with that one then ;)

CompuIves
CompuIves approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@CompuIves CompuIves left a comment

Choose a reason for hiding this comment

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

Looks clear to me! I really like that we're not using stubs for workspace anymore. I've a couple of questions, but generally everything looks a-okay!

src/service-override/files.ts Show resolved Hide resolved
src/service-override/files.ts Show resolved Hide resolved
src/service-override/storage.ts Show resolved Hide resolved
src/workbench.ts Show resolved Hide resolved
Loïc Mangeonjean added 18 commits November 17, 2023 10:11
@CGNonofr
fix: start editor factory
e039f3f
@CGNonofr
fix: no need to replace APP_SETTINGS_HOME anymore
463d31c
@CGNonofr
feat: provide workbench contruction option when initializing services
35fcde2
@CGNonofr
feat: use workbench contruction option when initializing services
f20bf79
@CGNonofr
feat: flush storage service on focus change
0ea4e7a 
like in vscode
@CGNonofr
feat: add a way to register IndexedDB providers like in for user data…
bdbeccc 
… and logs
@CGNonofr
feat: do not overwrite file when initializing it
27cedfb
@CGNonofr
fix: create view parts later so that editor serializer registry is ready
4989be9
@CGNonofr
clean: remove debug code
dc9e96e
@CGNonofr
feat: add a method to registe reditor serializer
4b43ecc
@CGNonofr
fix(demo): fix eslint error
23b61a2
@CGNonofr
refactor: put default settings/keybinding in decidated files
e234b78
@CGNonofr
fix(demo): update service initialization according to api changes
61ec898
@CGNonofr
feat(demo): use IndexedDB fs providers
89dce94
@CGNonofr
feat(demo): add reset settings/keybindings buttons
5a46cbf
@CGNonofr
feat(demo): register custom editor serializer
d651097
@CGNonofr
feat: add ability to clean user data
cc5beca
@CGNonofr
fix: properly update keybindings
7a82720
@CGNonofr CGNonofr force-pushed the fix-workspace-initialization branch from b8716c6 to 7a82720 Compare November 17, 2023 09:11
@CGNonofr CGNonofr merged commit 278fa18 into main Nov 17, 2023
1 check passed
@CGNonofr CGNonofr deleted the fix-workspace-initialization branch November 17, 2023 10:23
@kaisalmen
Copy link
Collaborator

@kaisalmen I'd like your opinion on those changes as I will impact monaco-languageclient due to the usage of IWorkbenchConstructionOptions

@CGNonofr can you give me a hint (link to the demo maybe) why this impacts monaco-languageclient (I haven't done anything yet, just going through what happend the last couple of days). Thank you.

@CGNonofr
Copy link
Contributor Author

CGNonofr commented Nov 20, 2023 •
edited
Loading

@kaisalmen I'd like your opinion on those changes as I will impact monaco-languageclient due to the usage of IWorkbenchConstructionOptions

@CGNonofr can you give me a hint (link to the demo maybe) why this impacts monaco-languageclient (I haven't done anything yet, just going through what happend the last couple of days). Thank you.

It impacts the way services are initialized, see 61ec898

It moves initialization parameter to the last parameter of the initialize method instead of getServiceOverride methods, especially the workspace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CompuIves CompuIves CompuIves approved these changes

@kaisalmen kaisalmen Awaiting requested review from kaisalmen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CGNonofr @CompuIves @kaisalmen