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

Implement reparenting ScopedContextKeyServices #104694

Merged
merged 3 commits into from Sep 16, 2020

Conversation

roblourens
Copy link
Member

Background: #99318

The notebook editor has to be reused when dragging it between editor groups because we can't lose the state of code running in outputs in the webview. This has been implemented since a few months ago but there is an issue - the ScopedContextKeyServices under the notebook editor (like for the ListView) are created as children of the context key service for the editor group. This causes issues unless they can be reparented to the context key service for the correct group.

I wanted to fix this by persisting only the webview and reinstantiating the notebook editor itself so that this would work more like other editors (where we always reinstantiate the edit when moving it between groups). We can't do that because the webview has to be in the DOM in the middle of the notebook editor DOM structure. If you move it in the DOM, its contents will reload.

I believe the only solution is to allow moving ScopedContextKeyServices to a new parent. This is a little awkward but I think it's unavoidable. So this PR does the following

Implement reparenting ScopedContextKeyServices

updateParent has to do the following

  • Update this._parent
  • Get the Context, and the parent's Context, update the Context's this._parent. This requires exposing the ID which I don't really like
  • Collect all context values before and after, diff them, fire updates for any changes

Give NotebookEditorWidget its own ScopedContextKeyService

Because we need one root for the editor to move to the new parent.

Give the title control a way to get the context key service inside any editor, not just code editors

We had a backdoor in place just for the code editor, but we can extend this to any editor. We could do this in a nicer and less ad-hoc way than what I've implemented here @bpasero, like with a method on BaseEditor to override, let me know what you think.

@roblourens roblourens self-assigned this Aug 14, 2020
@jrieken jrieken requested a review from alexdima August 17, 2020 06:20
@jrieken
Copy link
Member

jrieken commented Aug 17, 2020

adding @alexdima who did the context key services

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I don't really like the new indirection via getEditorWithContext and I also do not like our current use of getCodeEditor which makes this code work only with ICodeEditor or IDiffEditor. Rather I think every workbench editor should have a chance to implement this properly and then do the right thing internally.

As such I would suggest to expose a method from all base workbench editors that subclasses can override to control this and we keep the current behaviour inside the text editor.

We also carefully need to check other places where we call invokeWithinContext. For example, there is IEditorService#invokeWithinEditorContext which probably needs the same tweak.

@roblourens
Copy link
Member Author

Good point. Should this be on IEditorPane or IEditorControl?

@bpasero
Copy link
Member

bpasero commented Aug 18, 2020

@roblourens I feel like it should be on IEditorPane because that is the workbench concept for editors. The control is mainly a way to get at the ICodeEditor in most cases but I think in this case is not needed (the implementation can delegate as needed).

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

👍 The changes in the context key service look good!

@jrieken jrieken removed their request for review August 31, 2020 07:15
roblourens added a commit that referenced this pull request Aug 31, 2020
@roblourens roblourens added this to the September 2020 milestone Aug 31, 2020
- Give NotebookEditorWidget its own ScopedContextKeyService
- Give the title control a way to get the context key service inside any editor
Fix #99318
@roblourens roblourens force-pushed the roblou/reparentContextKeyServices branch from f88e77a to 1406eab Compare September 15, 2020 21:29
@roblourens roblourens merged commit d25064b into master Sep 16, 2020
@roblourens roblourens deleted the roblou/reparentContextKeyServices branch September 16, 2020 02:29
roblourens added a commit that referenced this pull request Sep 16, 2020
roblourens added a commit that referenced this pull request Sep 16, 2020
roblourens added a commit that referenced this pull request Sep 16, 2020
bpasero added a commit that referenced this pull request Sep 18, 2020
* Implement generic invokeWithinContext for all editor panes
Continues PR #104694

* - Remove "invokeWithinContext" from groups and panes,
only expose context key service
- Implement this correctly for composite editors
- Fix tests

* Rename and simplify things

* Fix build error

* Fix EditorService unit test

* Update src/vs/workbench/browser/parts/editor/textDiffEditor.ts

Co-authored-by: Benjamin Pasero <benjpas@microsoft.com>

* 💄
PR #104947

* 💄

Co-authored-by: Benjamin Pasero <benjpas@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants