Add secondary window support for text editors #13493

tsmaeder · 2024-03-14T13:13:23Z

What it does

Adds secondary windows support for monaco text editors.

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

contributed on behalf of STMicroelectronics

Note that this PR introduces patch-package as a way to manage PhosphorJS patches. This choice is controversial, as it introduces the burden to do the same when building a theia-based product. Discussion of alternatives can and should be happening on this PR, IMO. I'm opening the PR now in order to allow for testing and code-review, because the mechanism of patching PhosphorJS is irrelevant to those concerns.

Note: #13214 is not part of this PR.

How to test

Extract monaco editors to secondary windows an make sure the normal editor functionality works in the secondary window. This should include the browser and electron case and navigation among editors.

Follow-ups

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

.theia/settings.json

sdirix

I had a rough look over the code but not in detail. I also tested the changes in the Electron example and Browser example.

Extracting editors seems to work fine in both Browser and Electron! Nice!

I found one issue in Electron. The "minimap" overview in the top right does not appear initially. Only when resizing the window horizontally, the "minimap" appears.

I also tested web view VS code extensions and extracting them still works 👍

We must think about the adopter story too. To get this feature in, adopters will also need to execute the patching themselves. This means they will need to:

add patch-package as a devDependency in their root package.json
specify postinstall: patch-package --patch-dir node_modules/@theia/core/patches in their root package.json

This must be mentioned in the breaking changes, however still many users will at first fail to do so. We will also need to adapt documentation and projects like the yeoman theia-generator.

It would be great if the patching was optional, i.e. if the user did not patch then Theia still works as intended, just that the secondary window feature does not work. Bonus points if the cause can be detected and an error is logged.

Looking at the patches, they mostly seem to introduce additional optional parameters. I quickly tested the Electron example application without applying the patches. I had to fix a typing error in browser-menu-plugin.ts, however besides that everything built and worked fine! Of course the secondary window feature then stopped working, but that is expected.

In the long run we are hopefully able to switch from PhosphorJS to Lumino and contribute these changes in a compatible way. Then we can get rid of the patching again.

sdirix · 2024-03-18T14:44:21Z

dev-packages/application-manager/src/generator/webpack-generator.ts

+            },
+            {
+                test: /\.wasm$/,
+                type: 'asset/resource'


Why is this change required?

If I recall correctly, the monaco editor uses wasm for tokenizing and the wasm files were not included in the bundle.

patches/@phosphor+widgets+1.9.3.patch

sdirix · 2024-03-18T14:46:16Z

patches/@theia+monaco-editor-core+1.83.101.patch

Must this be solved via patch? Can't we modify and publish a new @theia/monaco-editor-core ourselves?

This is already fixed in the current version of VS Code. Maintaining patches in our rebuilt version of VS Code is not something I want to start. The alternative, IMO is to patch it here or to leave the resizing of peek-views broken until we update monaco.

packages/core/src/browser/menu/browser-context-menu-renderer.ts

sdirix · 2024-03-18T14:47:59Z

packages/editor/package.json

@@ -45,4 +46,4 @@
  "nyc": {
    "extends": "../../configs/nyc.json"
  }
-}
+}


nitpick: keep the newline?

It's the formatter that does this: only turning off "format on save" allows to keep the newline, see above.

rschnekenbu

The current support is already great! I tested on browser and electron.
The current limitation I could see along the ones already documented was the copy and paste not working properly on text editors. I tested on a 2nd window with the drawio extension, and that seems to work in case of diagram editors. I tried in text editors both with context menu and shortcuts, they don't work in my case. It can still paste if there was some string in the clipboard, but copy and cut do not update the clipboard.
Otherwise, theme is supported in new window, navigation works as expected, most actions I could test were working also as expected, as for example rename symbol and its shortcut.

packages/core/src/browser/secondary-window-handler.ts

tsmaeder · 2024-03-22T14:30:13Z

The copy action actually works, but it copies from the main window's document (document.execCommand('copy');).

tsmaeder · 2024-03-22T15:01:56Z

I found one issue in Electron. The "minimap" overview in the top right does not appear initially. Only when resizing the window horizontally, the "minimap" appears.

@sdirix this works just fine for me on Electron/Windows. What's different?

- 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 <[email protected]>

Signed-off-by: Thomas Mäder <[email protected]>

rschnekenbu

That looks good to me on code and behavior aspect (both electron and browser).
I don't have any constructive comment on patch-package usage and if there are better alternatives here.

tsmaeder · 2024-03-26T12:20:04Z

@rschnekenbu what's missing for an approval?

rschnekenbu

@tsmaeder, looking good to me. I am of course leaving space for other committers to discuss on patch-package, but for what I can review, this is approved ;)

tsmaeder requested review from rschnekenbu and msujew March 14, 2024 13:13

tsmaeder force-pushed the 13163_extractable_editors branch 2 times, most recently from 0b4d0e7 to c2234d1 Compare March 18, 2024 13:18

sdirix reviewed Mar 18, 2024

View reviewed changes

.theia/settings.json Outdated Show resolved Hide resolved

sdirix reviewed Mar 18, 2024

View reviewed changes

rschnekenbu reviewed Mar 21, 2024

View reviewed changes

packages/core/src/browser/secondary-window-handler.ts Show resolved Hide resolved

tsmaeder force-pushed the 13163_extractable_editors branch from ac70f38 to 06dda74 Compare March 22, 2024 15:22

tsmaeder requested review from rschnekenbu and sdirix March 22, 2024 15:22

tsmaeder added 9 commits March 25, 2024 10:56

Patch monaco to make sashes work in secondary windows.

7ba1fcc

Signed-off-by: Thomas Mäder <[email protected]>

Reworked patching mechanism

001d5b5

Signed-off-by: Thomas Mäder <[email protected]>

Use full path of patch-package script

bbed807

Signed-off-by: Thomas Mäder <[email protected]>

Add executable flags to theia-patch.js

b7947e3

Signed-off-by: Thomas Mäder <[email protected]>

Make the linter happy.

c104dc6

Signed-off-by: Thomas Mäder <[email protected]>

Fix bogus context menu patch

1606a23

Signed-off-by: Thomas Mäder <[email protected]>

Address review comments

3ca8d00

Signed-off-by: Thomas Mäder <[email protected]>

Addressing more review comments.

cbfc698

Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder force-pushed the 13163_extractable_editors branch from 06dda74 to cbfc698 Compare March 25, 2024 09:56

Add changelog entry

21ded4e

Signed-off-by: Thomas Mäder <[email protected]>

rschnekenbu reviewed Mar 26, 2024

View reviewed changes

rschnekenbu approved these changes Mar 26, 2024

View reviewed changes

tsmaeder merged commit a47ddeb into eclipse-theia:master Mar 27, 2024
14 checks passed

jfaltermeier added this to the 1.48.0 milestone Mar 28, 2024

sdirix mentioned this pull request Apr 16, 2024

Multi Window support (Floating windows) #10526

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add secondary window support for text editors #13493

Add secondary window support for text editors #13493

tsmaeder commented Mar 14, 2024 •

edited

Loading

sdirix left a comment •

edited

Loading

sdirix Mar 18, 2024

tsmaeder Mar 21, 2024

sdirix Mar 18, 2024

tsmaeder Mar 20, 2024

sdirix Mar 18, 2024

tsmaeder Mar 20, 2024

rschnekenbu left a comment

tsmaeder commented Mar 22, 2024

tsmaeder commented Mar 22, 2024

rschnekenbu left a comment

tsmaeder commented Mar 26, 2024

rschnekenbu left a comment

Add secondary window support for text editors #13493

Add secondary window support for text editors #13493

Conversation

tsmaeder commented Mar 14, 2024 • edited Loading

What it does

How to test

Follow-ups

Review checklist

Reminder for reviewers

sdirix left a comment • edited Loading

Choose a reason for hiding this comment

sdirix Mar 18, 2024

Choose a reason for hiding this comment

tsmaeder Mar 21, 2024

Choose a reason for hiding this comment

sdirix Mar 18, 2024

Choose a reason for hiding this comment

tsmaeder Mar 20, 2024

Choose a reason for hiding this comment

sdirix Mar 18, 2024

Choose a reason for hiding this comment

tsmaeder Mar 20, 2024

Choose a reason for hiding this comment

rschnekenbu left a comment

Choose a reason for hiding this comment

tsmaeder commented Mar 22, 2024

tsmaeder commented Mar 22, 2024

rschnekenbu left a comment

Choose a reason for hiding this comment

tsmaeder commented Mar 26, 2024

rschnekenbu left a comment

Choose a reason for hiding this comment

tsmaeder commented Mar 14, 2024 •

edited

Loading

sdirix left a comment •

edited

Loading