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

[feature-request]: Floating editor windows #13163

Closed
kittaakos opened this issue Dec 11, 2023 · 15 comments · Fixed by #13493
Closed

[feature-request]: Floating editor windows #13163

kittaakos opened this issue Dec 11, 2023 · 15 comments · Fixed by #13493
Assignees
Labels
editor issues related to the editor secondary-window issues related to multi or secondary window support
Milestone

Comments

@kittaakos
Copy link
Contributor

Feature Description:

Something like what VS Code (1.85.0) can do: https://code.visualstudio.com/updates/v1_85#_floating-editor-windows

@msujew msujew added editor issues related to the editor secondary-window issues related to multi or secondary window support labels Dec 20, 2023
@tsmaeder
Copy link
Contributor

As far as I can see, VS Code is also using the same approach of just moving the html to a different window, just like we did it for the secondary window support. I believe CSS support, etc. should just work since it's based on webpack finding the right CSS, which is the same mechanism as in the main window.
One difference is that they move the editor group handling with the editors, so you can have multiple editor tabs complete with their toolbars in the secondary window. This would be very welcome for other instances of secondary window support (like terminals) anyway, but looks more complicated and needs analysis.

@tsmaeder
Copy link
Contributor

A little experimenting shows that the CSS does not get loaded properly when an editor is moved to a secondary window. The problem seems to be that we're missing a call to StandaloneThemeService.registerEditorContainer(domElement);. The StandaloneEditor calls this from the constructor, which adds the "monaco-colors" style element the the document header. VS code creates a new instance of the editor in the "AuxiliaryWindow", which registers the style element there.

A second problem I ran into is that beginning with _backgroundTokenizeWithDeadline(), the editor tries to post some messages via $globalThis, which leads to errors in the console. This seems to happen regularly while you edit in the secondary window.

Thirdly, opening other editors, for example with ctrl-click fails silently. I would guess we need to hook up the open event properly.

Other things like hover or content assist seem to work fine, though. Editing and saving seem to work fine, as well.

@tsmaeder tsmaeder self-assigned this Feb 7, 2024
@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 9, 2024

Extending our current "external window" support to text editors would entail (IMO)

  1. Coming up with a contribution point that allows contributors to hook into the process of moving the widget into a secondary window
  2. Use this mechanism to set up the correct host communication for webviews (see acquireVsCodeApi(), etc. Currently, this support is hard-coded in secondary-window.html.
  3. Make the monaco editor widget into an ExtractableWidget and use the hook contribution from above to load the proper css values
  4. Make opening links work.

If all goes well, I think these steps should be doable inside two weeks. Steps 1 and 2 could be done as a first step, in order to allow for smaller PR's. Although what we have now works, I don't think it's architecturally correct with secondary window support knowing about webviews.

@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 9, 2024

Supporting layouts for extracted widgets (having multiple widgets in the same external window) would not change the current architecture as far as css, webview communications, etc. are concerned. However, we would have to

  1. Extend our widget management to allow widgets to live in multiple external windows (ApplicationShell).
  2. Set up the appropriate phosphor widgets when opening a secondary window. That includes toolbars. I assume the main menu would remain on the main window.
  3. Set up d&d support for moving widgets to secondary windows

Once we start using drag and drop, should we support moving widgets between secondary windows and back to the original window?

@tsmaeder
Copy link
Contributor

Reminder: my branch for this is 13163_investigation

@tsmaeder
Copy link
Contributor

Should have a look at #13214 while we're at it. Editors are not much use if you can't reopen them.

@tsmaeder
Copy link
Contributor

tsmaeder commented Mar 1, 2024

Related and still open feature request: #11648

@tsmaeder
Copy link
Contributor

tsmaeder commented Mar 6, 2024

Context menus are broken even for our current extractable Windowy: #12722

Problem lies in our implementation of IContextMenuService, but also the Phosphor menu widget does not work with secondary windows: the menu is attached to the current global document variable, which is always the main menu.

@tsmaeder
Copy link
Contributor

tsmaeder commented Mar 12, 2024

There is another problem related to the "references" zone view: the view uses a ListView which in turn is has a SmoothScrollableElement extending AbstractScrollableElement. When the list tries to send an open event, it checks whether the mouse event target element is inside the scrollable element, but this check always seems to fail.

@tsmaeder
Copy link
Contributor

Turns out target instanceof target.ownerDocument.defaultView.HTMLElement === true, but target instanceof HTMLElement === false. Different windows seems to have different class instances for HTMLElement. Go figure!

tsmaeder added a commit to tsmaeder/theia that referenced this issue Mar 14, 2024
- moves hard-coded support for webviews in secondary
windows into the webviews support
- introduces patch-package as a way to manage Phosphor patches
- fixes the context menu support in secondary windows

fixes eclipse-theia#13163, eclipse-theia#12722

contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder
Copy link
Contributor

On the "bright" side, navigating via the PeekView does not work reliably in VS Code, either.

@tsmaeder
Copy link
Contributor

Found another issue: resizing the Peek View does not work in the secondary windows. It works in VS Code.

tsmaeder added a commit to tsmaeder/theia that referenced this issue Mar 14, 2024
- moves hard-coded support for webviews in secondary
windows into the webviews support
- introduces patch-package as a way to manage Phosphor patches
- fixes the context menu support in secondary windows

fixes eclipse-theia#13163, eclipse-theia#12722

contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder
Copy link
Contributor

Diagnostics don't work when introducing an error in a secondary window with typescript: seems the typescript extensions keeps track of which tabs are shown to the user and only computes diagnostics for open tabs. I suspect the editors in external windows are not part of any tabs and therefore no diagnostics are computed.

@tsmaeder
Copy link
Contributor

tsmaeder commented Mar 18, 2024

The problem with the PeekView not being resizable is a problem in the VS Code code: in sash.ts on line 182, the code now reads:

return this.disposables.add(new DomEmitter(getWindow(this.el), 'mousemove')).event;

however, were using version 1.83.1, where the code still was

return this.disposables.add(new DomEmitter(window, 'mousemove')).event;

The problem here is that the listener is added to the main window (window) instead of the secondary window. You can indeed drag the mouse into the main window and the PeekView will be resized.
My preferred fix for this would be to update to the newest VS Code version.

@tsmaeder
Copy link
Contributor

The relevant PR in VS Code is microsoft/vscode#195778

tsmaeder added a commit to tsmaeder/theia that referenced this issue Mar 18, 2024
- moves hard-coded support for webviews in secondary
windows into the webviews support
- introduces patch-package as a way to manage Phosphor patches
- fixes the context menu support in secondary windows

fixes eclipse-theia#13163, eclipse-theia#12722

contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
tsmaeder added a commit to tsmaeder/theia that referenced this issue Mar 22, 2024
- moves hard-coded support for webviews in secondary
windows into the webviews support
- introduces patch-package as a way to manage Phosphor patches
- fixes the context menu support in secondary windows

fixes eclipse-theia#13163, eclipse-theia#12722

contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
tsmaeder added a commit to tsmaeder/theia that referenced this issue Mar 25, 2024
- moves hard-coded support for webviews in secondary
windows into the webviews support
- introduces patch-package as a way to manage Phosphor patches
- fixes the context menu support in secondary windows

fixes eclipse-theia#13163, eclipse-theia#12722

contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
tsmaeder added a commit that referenced this issue Mar 27, 2024
- moves hard-coded support for webviews in secondary windows into the webviews support
- introduces patch-package as a way to manage Phosphor patches
- fixes the context menu support in secondary windows

fixes #13163, #12722

contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@jfaltermeier jfaltermeier added this to the 1.48.0 milestone Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor issues related to the editor secondary-window issues related to multi or secondary window support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants