Multi window support for web views #11048

lucas-koehler · 2022-04-20T16:01:13Z

What it does

This implements the multi window MVP proposal desribed in #10526 (comment)

Support for moving webview-based views into a secondary window or tab.
For webview-based views a new button becomes available in the toolbar of the view to move the view to a secondary window.
This is only supported for webview-based views and only one view can be moved into the secondary window.
There can be multiple secondary windows though.

Primary code changes:

Add concept of extractable widgets. Only widgets implementing the interface can be extracted.
Add SecondaryWindowHandler that encapsulates logic to move widgets to new windows.
Only webviews can be extracted
Configure opened secondary windows in electron
- Hide electron menu
- Always use the native window frame for secondary windows to get window controls
- Do not show secondary window icon if main window uses a custom title bar
Contribute widget extraction button in a separate new extension widget-extraction-ui
Extend application shell areas with a secondaryWindow area that contains all extracted widgets
Extend frontend and webpack generators to generate the base html for secondary windows and copy it to the lib folder
Bridge plugin communication securely via secure messaging between external webview and Theia:
Webviews only accept messages from its direct parent window. This is necessary to avoid Cross Site Scripting (XSS).
To make the messaging work in secondary windows, messages are sent to the external widget and then delegated to the webview's iframe.
Thereby, the secondary window only accepts messages from its opener which is the theia main window.
To achieve this, a webview knows the secondary window it is in (if any). It then sends messages to this window instead of directly to the webview iframe.
Patch phosphor library during install via a postinstall hook. Remove check that widget attachment target must be in same DOM.

Contributed on behalf of ST Microelectronics and Ericsson and by ARM and EclipseSource.

Co-authored-by: Stefan Dirix sdirix@eclipsesource.com
Co-authored-by: robmor01 Rob.Moran@arm.com
Signed-off-by: Lucas Koehler lkoehler@eclipsesource.com

pr-ubuntu.mp4

Limitations

Electron is unsupported. Generally, the functinality is existent but there are some issues with close handling of secondary windows.
Only web views are supported. Plugin views don’t use webviews/iframes and thus cannot be extracted as part of this MVP.
Widgets/editors that do not support moving from one area to another (e.g. main to bottom) also do not support moving to secondary windows
If the last widget of an area is moved to a secondary window, it is still tracked as the former area’s current widget. This is caused by Dock panel's currentTitle is not reset to undefined when the last widget is moved elsewhere #10895 and also happens when moving widgets inside the main window
When a webview in a secondary window opens a file picker - or possibly another dialog - it is opened in the main window instead of the secondary one.
While an extracted webview resides in a secondary window, no contributed toolbar items contributed by the plugin are shown above the webview. This is caused by the absence of subsidiary UI (e.g. tabs, toolbar) in secondary windows. Fortunately, that isn't a very common use case since webviews can integrate any additional UI necessary.
External views cannot be restored to the main window, yet. To get a view back to the main window it needs to be closed and re-opened
Windows are not restored on reload because they are not part of the stored application layout.
extracted views do not respect "autoSave" (ex: for custom editors)

Screenshots

Ubuntu (click to expand)

Windows 11 (click to expand)

Mac (click to expand)

How to test

Get one or more VS Code extensions that contribute web views and drop them in the plugins folder. For instance:
- Protein viewer, you can use protein 2btf as example input
- Draw.io integration
- Mermaid Editor
- Build the webview sample providing a cat coding view from here
If you use other extensions, make sure they support being moved around in the shell (e.g. from main to bottom) because webviews that don't support that won't support being moved to a new window either
Open a web view and click the window button in its tool bar:

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

colin-grant-work

With the specified limitations, this appears to be working as advertised. There are some jarring bits, depending on the behavior of different webviews. For example, the Java plugin opens a webview to allow the user to configure their JDK settings, and that webview can open a file picker, but the file picker appears in the main window rather than secondary window. That's expected at the moment, but isn't optimal UI, by any means.

Otherwise, minor comments only.

colin-grant-work · 2022-04-20T18:38:59Z

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

+        }
+
+        // secondary-window.html is part of Theia's generated code. It is generated by dev-packages/application-manager/src/generator/frontend-generator.ts
+        const newWindow = window.open('secondary-window.html', `${this.prefix}-subwindow${this.secondaryWindows.length}`, 'popup');


Is this ID supposed to be unique? From the comment on the unUnload callback above, it sounds like closing a secondary window might remove it from the secondaryWindows array, and if that's the case, using length will not guarantee unique identifiers.

colin-grant-work · 2022-04-20T18:43:18Z

packages/core/src/electron-main/electron-main-application.ts

@@ -315,6 +316,41 @@ export class ElectronMainApplication {
        return electronWindow;
    }

+    /** Configures native window creation, i.e. using window.open or links with target "_blank" in the frontend. */
+    protected configureNativeSecondaryWindowCreation(electronWindow: BrowserWindow): void {


Would it be appropriate to implement this as part of the startup routine of the TheiaElectronWindow class?

I would rather keep it as is if that is okay with you:

On the one hand it makes sense to configure this as inside the TheiaElectronWindow because it only affects its child windows.
On the other hand, we use protected properties (useNativeWindowFrame) and methods (e.g. getDefaultOptions and getDefaultTheiaWindowBounds which was extracted from getDefaultTheiaWindowOptions) to create the secondary windows. We would need to make this publicly available and depend from TheiaElectronWindow on ElectronMainApplication.

colin-grant-work · 2022-04-20T18:48:00Z

packages/secondary-window-ui/src/browser/secondary-window-ui-frontend-contribution.ts

+export const EXTRACT_WIDGET = Command.toDefaultLocalizedCommand({
+    id: 'extract-widget',
+    label: 'Move View to Secondary Window'
+});


The nls methods containing default should only be used if the translated strings exist in VSCode. In this case, the label field should be translated using nls.localize with an appropriate key.

We have another method for normal localized commands, usage:

export const EXTRACT_WIDGET = Command.toLocalizedCommand({ id: 'extract-widget', label: 'Move View to Secondary Window' }, 'theia/secondary-window-ui/extract-widget');

colin-grant-work · 2022-04-20T18:49:01Z

packages/secondary-window-ui/src/browser/secondary-window-ui-frontend-contribution.ts

+                // sanity check
+                if (!ExtractableWidget.is(widget)) {
+                    // command executed with a non-extractable widget
+                    console.error('Invalid command execution');


More detail should be provided here:

Suggested change

console.error('Invalid command execution');

console.error('Invalid attempt to move non-extractable widget to secondary window.');

colin-grant-work · 2022-04-20T18:54:38Z

scripts/patch-libraries.js

+const patches = [
+    {
+        file: 'node_modules/@phosphor/widgets/lib/widget.js',
+        find: /^.*?'Host is not attached.'.*?$/gm,


Could use a comment explaining what this modifies:

Suggested change

find: /^.*?'Host is not attached.'.*?$/gm,

// removes an error thrown by PhosphorJS when a widget's node is not a child of `document.body` in order to accommodate secondary windows.

find: /^.*?'Host is not attached.'.*?$/gm,

lucas-koehler · 2022-04-21T15:32:50Z

@colin-grant-work Thank you very much for the quick review. I pushed two commits to address the issues and add machine translations for the new command.
I added the file picker issue to the limitations so we remember it and can open a follow up issue. Is that alright with you?

colin-grant-work · 2022-04-27T18:47:15Z

Another UI limitation of the current iteration - also related to the absence of subsidiary UI - is the absence of any toolbar items contributed by the plugin to be shown above its views. That isn't a very common use case since the webview can integrate any additional UI necessary, but it's worth noting along with the file-picker.

lucas-koehler · 2022-04-28T07:17:11Z

@colin-grant-work That is a good point, thanks. I added the following limitation to the PR description:

While an extracted webview resides in a secondary window, no contributed toolbar items contributed by the plugin are shown above the webview. This is caused by the absence of subsidiary UI (e.g. tabs, toolbar) in secondary windows. Fortunately, that isn't a very common use case since webviews can integrate any additional UI necessary.

koegel · 2022-05-04T20:55:41Z

@colin-grant-work Do you feel comfortable in approving this change or is there open issues to be fixed in your opinion or should we get an additional pair of eyes? Thanks!

colin-grant-work · 2022-05-04T23:02:37Z

@koegel, I'll give it another run through tomorrow to make sure there aren't any regressions that I've missed, and if there aren't then I'll approve. In the meantime, it looks like it needs to be rebased.

koegel · 2022-05-04T23:04:18Z

Perfect, thanks!

vince-fugnitto · 2022-05-05T13:43:40Z

CHANGELOG.md

@@ -12,6 +12,7 @@
 - [plugin] added support for `SnippetString.appendChoice` [#10969](https://github.com/eclipse-theia/theia/pull/10969) - Contributed on behalf of STMicroelectronics
 - [plugin] added support for `AccessibilityInformation` [#10961](https://github.com/eclipse-theia/theia/pull/10961) - Contributed on behalf of STMicroelectronics
 - [plugin] added missing properties `id`, `name` and `backgroundColor` to `StatusBarItem` [#11026](https://github.com/eclipse-theia/theia/pull/11026) - Contributed on behalf of STMicroelectronics
+- [core] Added support for moving webview-based views into a secondary window/tab. Added new extension `secondary-window-ui` that contributes the UI integration to use this. [#11048](https://github.com/eclipse-theia/theia/pull/11048)- Contributed on behalf of ST Microelectronics and Ericsson and by ARM and EclipseSource


After rebasing please ensure that this entry is added under v1.26.0.

vince-fugnitto · 2022-05-05T13:44:19Z

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

+</head>
+
+<body>
+    <div id="pwidget"></div>


Can we find a better name for this id than pwidget?

I renamed this to widget-host as PhoshporJS describes the element to attach a widget to as host. I hope that's alright with you. Otherwise, I can rename it again.

vince-fugnitto · 2022-05-05T13:56:33Z

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

+        const newWindow = window.open('secondary-window.html', this.nextWindowId(), 'popup');
+
+        if (!newWindow) {
+            this.messageService.error('The widget could not be moved to a secondary window because the window creation failed. Please make sure to allow popus.');


Suggested change

this.messageService.error('The widget could not be moved to a secondary window because the window creation failed. Please make sure to allow popus.');

this.messageService.error('The widget could not be moved to a secondary window because the window creation failed. Please make sure to allow popups.');

vince-fugnitto · 2022-05-05T14:16:07Z

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

+    constructor(
+        @inject(MessageService) protected readonly messageService: MessageService,
+        @inject(WindowService) protected readonly windowService: WindowService
+    ) { }


I believe we should use property injection here?

https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#property-injection

vince-fugnitto · 2022-05-05T14:38:34Z

packages/core/src/browser/shell/application-shell.ts

@@ -219,6 +220,7 @@ export class ApplicationShell extends Widget {
        @inject(ApplicationShellOptions) @optional() options: RecursivePartial<ApplicationShell.Options> = {},
        @inject(CorePreferences) protected readonly corePreferences: CorePreferences,
        @inject(SaveResourceService) protected readonly saveResourceService: SaveResourceService,
+        @inject(SecondaryWindowHandler) protected readonly secondaryWindowHandler: SecondaryWindowHandler,


I believe this is a breaking change that should be documented:

https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#property-injection

lucas-koehler · 2022-05-05T16:18:19Z

Hi @vince-fugnitto , thank you for your review :)
I rebased the changes, applied your comments and added a new 1.26.0 section to the changelog

vince-fugnitto

@lucas-koehler I haven't looked at the code extensively or the general approach taken to achieve multi-window yet, but I do see some improvements/features we likely want in the first iteration?

Restore View

The ability to restore an extracted view from a secondary window back to the main window is missing. I believe that this is a basic feature that should be part of the initial implementation and apply for all secondary windows (webviews and for future use-cases). At the moment the only way I can see that users are able to restore views is to close them and re-open them in the main app which is not great for useability. Sometimes we only want to extract views temporarily.

SecondaryWindowHandler Improvement

The secondary-window-handler does not seem to know much about secondary windows, or perhaps the use-case is not handled, but I feel it should be handled? Take for example the following use-case:

open a .drawio file - the drawio integration will open the drawio editor
extract the view to secondary window
minimize the secondary window
re-select the same .drawio file from the explorer
the secondary window is not focused or brought to the forefrtont as a user would expect

I believe the handler should know about this use-case, it should be aware of all secondary windows and know how to show them given if they can handle a particular resource.

Restore Window on Reload

Part of the initial implementation is likely to support window restoration for all secondary views. We should probably think of adding the secondary windows to the application's layout we store, and properly restore such windows on reload. If the user sets up a specific "scene" for his development we probably want to restore that on reload for the best user-experience.

koegel · 2022-05-06T09:53:39Z

@vince-fugnitto Thank you for taking the time to review this. I think the three missing features you point out are all valid and important.

We have aimed to rather release an MVP of the MDI earlier with less features than a feature-complete version of it later. In addition the functionality is opt-in, meaning you will have to add the extension to your product to make use of it, so you will be aware of its limitations. Therefore we have stripped down this PR to the bare minimum. In this sense "Restore View" and "Restore Window on Reload" are imho features that do not have to be part of the MVP. In my opinion the functionality is still valuable without them and is far more than you can achieve with VS Code today.

Some more reasons why I believe we should rather release this earlier with less features than later with more features:

Might drive adoption of Theia: This is a textbook case of where Theia is more powerful than VS Code. This feature is ranked high by users of VS Code but there seems to be no intent to get it implemented for VS Code.
Might attract contributions: Once this features becomes visible there is a good chance we might see contributions by others that improve and enhance it.
Prioritization of additional features might change once this feature is in use: For example I could imagine that users would rather like to have support for the terminal in secondary windows than being able to restore view on reload and both might have similar effort.

I would therefore propose the following:

We will take a look whether "SecondaryWindowHandler Improvement" can be fixed without additional changes to Phosphor (which is to be avoided) and with reasonable additional effort.
For the other two features we would also do a ball-park estimation and open up follow-up issues to extract them from this PR.
We amend the information about limitations of this feature with your comments so adopters know exactly what they get.

What do you think?

vince-fugnitto · 2022-05-06T12:27:40Z

I would therefore propose the following:

We will take a look whether "SecondaryWindowHandler Improvement" can be fixed without additional changes to Phosphor (which is to be avoided) and with reasonable additional effort.

For the other two features we would also do a ball-park estimation and open up follow-up issues to extract them from this PR.

We amend the information about limitations of this feature with your comments so adopters know exactly what they get.

What do you think?

@lucas-koehler I don't mean to hold back the pull-request because the basic features are missing, just wanted to highlight them and give my first impressions as a user which others might experience as well.

I'm fine with the way forward, we still have the month to polish the mvp and possibly implement these features which I do think are generic and basic and will benefit all future views. The mvp already only handles a subset of all use-cases (webviews), which others may not find all that useful, so I thought that making sure the features are present will only help adoption.

lucas-koehler · 2022-05-06T13:17:02Z

Hi @vince-fugnitto , thank you very much for your input.

One question regarding "SecondaryWindowHandler Improvement": Did you observe this in electron or the browser? For electron this is already known but in the browser bringing up the minimized window works for me. If in the browser, which browser did you use?

For the other two points, I added the following limitations to the PR description:

External views cannot be restored to the main window, yet. To get a view back to the main window it needs to be closed and re-opened

Windows are not restored on reload because they are not part of the stored application layout.

vince-fugnitto · 2022-05-06T13:29:13Z

One question regarding "SecondaryWindowHandler Improvement": Did you observe this in electron or the browser? For electron this is already known but in the browser bringing up the minimized window works for me. If in the browser, which browser did you use?

@lucas-koehler I observed it for electron, have you investigated as to why it does not work in electron but does in the browser?

lucas-koehler · 2022-05-06T13:56:30Z

@lucas-koehler I observed it for electron, have you investigated as to why it does not work in electron but does in the browser?

@vince-fugnitto When an external widget is revealed by the application shell, the secondary window handler calls focus on the widget's external Window. In the browser this is sufficient to bring the window to the front. However, I do not know why this does not work for electron.

vince-fugnitto · 2022-05-06T15:47:57Z

@vince-fugnitto When an external widget is revealed by the application shell, the secondary window handler calls focus on the widget's external Window. In the browser this is sufficient to bring the window to the front. However, I do not know why this does not work for electron.

@lucas-koehler I believe the bug should be fixed for the mvp, it is likely due to a design problem of how we deal with windows in browser vs electron. For instance, we have the window-service today which properly handles each target separately that we may want to enhance on how to create new windows, and focus them.

lucas-koehler · 2022-05-10T12:34:19Z

@lucas-koehler I believe the bug should be fixed for the mvp, it is likely due to a design problem of how we deal with windows in browser vs electron. For instance, we have the window-service today which properly handles each target separately that we may want to enhance on how to create new windows, and focus them.

@vince-fugnitto Seems reasonable. I will investigate this later this week.

lucas-koehler · 2022-05-16T11:16:29Z

Hello @vince-fugnitto ,
I solved the electron focus issue by adding a new service SecondaryWindowService that handles creating and focussing the secondary windows. I preferred a separate service because the secondary window creation has not much in common with opening other windows. But if you prefer, I can also integrate this in the existing WindowService.

I squashed the existing commits from the older review comments and added a separate commit for the new service to hopefully make it easier to review :)

For the other two issues, I'd stick with opening follow up issues along with the other limitations after this was merged.

vince-fugnitto

The changes work well for me, we can open up the follow-up issues.
I have some minor comments, and I would like others to take a look at the changes as well.

package.json

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

packages/core/src/electron-browser/window/electron-secondary-window-service.ts

packages/secondary-window-ui/src/browser/secondary-window-ui-frontend-contribution.ts

lucas-koehler · 2022-05-17T09:24:19Z

@vince-fugnitto I fixed the minor comments. I'll address the phosphor comment here for better visibility.

I believe this change will actually not work downstream, and means that the feature is unusable for them?
We are only patching for ourselves, what do we expect downstream to do?

That is a very good catch, thanks! Besides actually forking PhosphorJS - which was determined not to be viable for this MVP (see here for reasons) - I see two possibilities to solve this:

Provide a quick guide for users to apply the given patch in their Theia instance (adding the script and adding the postinstall to their root package.json)
Adapt the webpack file generated in webpack-generator.ts to remove the problematic line from PhosphorJS during the build.

Initially we went with the separate script because it is easier to extend and see what exactly is happening. However, requiring users to manually adapt their package.json might not be the best way to go.

IMO, option number 2 is the way to go. Downstream users don't need to do anything and no files are altered on the user's file system. This is also safe against postinstall scripts potentially not being run by default in the future. As PhosphorJS is archived, the webpack adaption should also be stable because Phoshpor does not change.

Does that sound like an acceptable solution to you?

vince-fugnitto · 2022-05-17T12:08:10Z

Does that sound like an acceptable solution to you?

@lucas-koehler if the second option works then I'd be fine with it :) Initially I had thought the best solution would be to fork the repo, even if in the long run we want to move away from it. Alternatively, like 1 we could have the script under cli but I also find that undesirable for downstream apps.

Let's go with your second proposal 👍

lucas-koehler · 2022-08-26T10:23:47Z

Rebased changes onto 1.29.0

koegel · 2022-08-31T15:00:10Z

Gentlemen: are we good to go ahead with this PR? @colin-grant-work, @vince-fugnitto Do you want to re-review? @msujew Can you please do a final review of the changes made in response to your findings.
Thank you very much for all your effort to get this up and running!

Just a heads-up: we have already scheduled resources for September and October to make the next steps with MDI (electron support and support for the terminal). The earlier we get this merged the better...:smiley:

vince-fugnitto

@lucas-koehler @koegel given that the electron use-case is not yet supported, should we remove the electron-specific changes from this pull-request and add them when it is supported? It feels odd to me to include electron-specific changes when it does not work.

I do wonder if @theia/secondary-window-ui should be made more generic (possibly @theia/secondary-window as we might add non-UI functionality in that package later on) I also wonder if a lot of the secondary window changes can actually be moved to that package instead of in core given that all the code is actually dormant unless the package is consumed.

Am I correct that these are the latest limitations (we may want to update the pr description with the latest state):

electron is unsupported
only webviews (contributed by plugins) are supported
only extraction is supported, does not allow views to return to the main window
does not support possible tabbar or menu support
secondary windows does not restore on startup
possible actions from the webviews appear in the wrong window (ex: dialogs)
secondary windows are incorrectly tracked if the last widget (ex: "close all tabs in the main area" can potentially close secondary windows)
extracted views do not respect "autoSave" (ex: for custom editors)

I also noticed that the open editors tree-view should be updated with a better name than secondarywindow (all one word) for end-users:

I've also noticed that secondary windows do not display the workspace and application name the same way the main window does. If it is intended it unfortunately makes it harder to distinguish which secondary window belongs to which workspace.

lucas-koehler · 2022-09-02T08:57:24Z

Hello @vince-fugnitto ,
thank you for your extensive feedback :)
I updated the limitations section in the PR. I also squashed the commits before your review and rebased onto master.

As I believe you discussed yesterday with @koegel, I incorporated parts of your suggestions:

The extension was renamed to secondary-window
The open editors tree view now shows IN SECONDARY WINDOW instead of SECONDARYWINDOW (see screenshot below)
The external windows now append the workspace and application name (i.e. the main window's title) to their title (see screenshot below)

I also wonder if a lot of the secondary window changes can actually be moved to that package instead of in core given that all the code is actually dormant unless the package is consumed.

This was omitted because the application shell uses the secondary window handler in multiple places. If we move it to the other extension and make it optional we need to check and handle its absence in various places.

It feels odd to me to include electron-specific changes when it does not work.

They were left in to save the effort of extracting them just to re-add them again soon. However, the moveWidgetToSecondaryWindow method was removed from the application-shell and the secondary window handler was marked as experimental. This should avoid adopters using this functionality "by accident".

I hope this works for you and thanks again for your thoughts :)

Note: I did not remove all the other reviewers on purpose (I cannot even do this), it happend automatically by re-requesting your review.

UI update:

vince-fugnitto

The changes look good to me given the known limitations. Once merged we should open issues to track them.

vince-fugnitto · 2022-09-02T13:02:51Z

packages/secondary-window/package.json

+{
+  "name": "@theia/secondary-window",
+  "version": "1.29.0",
+  "description": "Theia - Secondary window UI contributions",


minor:

Suggested change

"description": "Theia - Secondary window UI contributions",

"description": "Theia - Secondary Window Extension",

Support for moving webview-based views into a secondary window or tab. For webview-based views a new button becomes available in the toolbar of the view to move the view to a secondary window. This is only supported for webview-based views and only one view can be moved into the secondary window. The window extraction is not added to the electron app for now as there are issues with close handling of secondary windows on Electron. There can be multiple secondary windows though. Primary code changes: - Add concept of extractable widgets. Only widgets implementing the interface can be extracted. - Add `SecondaryWindowHandler` that encapsulates logic to move widgets to new windows. - Add service `SecondaryWindowService` to handle creating and focussing external windows based on the platform. - Only webviews can be extracted - Configure opened secondary windows in electron - Hide electron menu - Always use the native window frame for secondary windows to get window controls - Do not show secondary window icon if main window uses a custom title bar - Contribute widget extraction button in a separate new extension `widget-extraction-ui` - Extend application shell areas with a `secondaryWindow` area that contains all extracted widgets - Extend frontend and webpack generators to generate the base html for secondary windows and copy it to the lib folder - Bridge plugin communication securely via secure messaging between external webview and Theia: Webviews only accept messages from its direct parent window. This is necessary to avoid Cross Site Scripting (XSS). To make the messaging work in secondary windows, messages are sent to the external widget and then delegated to the webview's iframe. Thereby, the secondary window only accepts messages from its opener which is the theia main window. To achieve this, a webview knows the secondary window it is in (if any). It then sends messages to this window instead of directly to the webview iframe. - Patch PhosphorJS during application webpack build - Use string-replace-loader to remove the critical line from PhosphorJS during application bundleing - Extend `webpack generator.ts` to add this to applications' `gen-webpack.config.js` - Add extension `secondary-window` which contributes the UI integration (and possibly later more) Contributed on behalf of ST Microelectronics and Ericsson and by ARM and EclipseSource. Co-authored-by: Stefan Dirix <sdirix@eclipsesource.com> Co-authored-by: robmor01 <Rob.Moran@arm.com> Signed-off-by: Lucas Koehler <lkoehler@eclipsesource.com>

lucas-koehler · 2022-09-02T13:12:37Z

@vince-fugnitto Thank you for the fast review. I squashed the commits and added your suggestion for the extension title

vince-fugnitto · 2022-09-08T11:49:26Z

@lucas-koehler I've opened a couple of follow-up issues to track the known limitations and bugs. If you want to do the same that would be appreciated.

lucas-koehler · 2022-09-08T12:33:10Z

@vince-fugnitto Thank you for your effort and the reminder :)
I believe I added the remaining issues based on the limitations listed in the PR.

lucas-koehler force-pushed the multi-window-mvp branch from 93d63d6 to 5eddd35 Compare April 20, 2022 16:02

colin-grant-work reviewed Apr 20, 2022

View reviewed changes

lucas-koehler requested a review from colin-grant-work April 21, 2022 15:32

vince-fugnitto added the webviews issues related to webviews label Apr 22, 2022

vince-fugnitto reviewed May 5, 2022

View reviewed changes

vince-fugnitto requested a review from paul-marechal May 5, 2022 14:45

lucas-koehler force-pushed the multi-window-mvp branch from 9ac7d87 to 87cf80a Compare May 5, 2022 16:16

lucas-koehler requested a review from vince-fugnitto May 5, 2022 16:19

vince-fugnitto reviewed May 5, 2022

View reviewed changes

lucas-koehler force-pushed the multi-window-mvp branch from 87cf80a to ad86315 Compare May 16, 2022 11:11

lucas-koehler requested a review from vince-fugnitto May 16, 2022 11:16

vince-fugnitto reviewed May 16, 2022

View reviewed changes

lucas-koehler force-pushed the multi-window-mvp branch from d87ec25 to c13f4b8 Compare May 20, 2022 11:59

vince-fugnitto reviewed Aug 31, 2022

View reviewed changes

vince-fugnitto requested review from alvsan09 and paul-marechal August 31, 2022 16:52

lucas-koehler force-pushed the multi-window-mvp branch from 14a7b07 to 853e9db Compare September 2, 2022 08:48

lucas-koehler requested review from vince-fugnitto and removed request for msujew, alvsan09 and paul-marechal September 2, 2022 08:57

vince-fugnitto approved these changes Sep 2, 2022

View reviewed changes

lucas-koehler force-pushed the multi-window-mvp branch from 853e9db to 97f6360 Compare September 2, 2022 13:11

JonasHelming merged commit a07b746 into eclipse-theia:master Sep 5, 2022

JonasHelming added this to the 1.30.0 milestone Sep 5, 2022

vince-fugnitto added the secondary-window issues related to multi or secondary window support label Sep 8, 2022

This was referenced Sep 8, 2022

secondary-window: add electron support #11642

Closed

secondary-window: add support for additional views besides webviews #11643

Open

secondary-window: add ability to retract a view #11644

Open

This was referenced Sep 8, 2022

secondary-window: Restore secondary windows on reload #11645

Open

secondary-window: Respect autoSave #11646

Closed

secondary-window: Open dialogs in the secondary window #11647

Open

secondary-window: Support tabbar and menu integrations #11648

Open

tsmaeder mentioned this pull request Sep 14, 2022

Extend multi-window support to Electron (#11642) eclipsesource/theia#49

Open

1 task

tsmaeder mentioned this pull request May 3, 2023

Implement "secondary window" support for Electron #12481

Merged

1 task

rschnekenbu mentioned this pull request Apr 11, 2024

Open external links in user browser for Theia electron app #13592

Closed

	console.error('Invalid command execution');
	console.error('Invalid attempt to move non-extractable widget to secondary window.');

	find: /^.?'Host is not attached.'.?$/gm,
	// removes an error thrown by PhosphorJS when a widget's node is not a child of `document.body` in order to accommodate secondary windows.
	find: /^.?'Host is not attached.'.?$/gm,

	this.messageService.error('The widget could not be moved to a secondary window because the window creation failed. Please make sure to allow popus.');
	this.messageService.error('The widget could not be moved to a secondary window because the window creation failed. Please make sure to allow popups.');

	"description": "Theia - Secondary window UI contributions",
	"description": "Theia - Secondary Window Extension",

Multi window support for web views #11048

Multi window support for web views #11048

Conversation

lucas-koehler commented Apr 20, 2022 • edited Loading

What it does

Limitations

Screenshots

How to test

Review checklist

Reminder for reviewers

colin-grant-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-koehler commented Apr 21, 2022

colin-grant-work commented Apr 27, 2022

lucas-koehler commented Apr 28, 2022

koegel commented May 4, 2022

colin-grant-work commented May 4, 2022 • edited Loading

koegel commented May 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-koehler commented May 5, 2022 • edited Loading

vince-fugnitto left a comment

Choose a reason for hiding this comment

koegel commented May 6, 2022

vince-fugnitto commented May 6, 2022

lucas-koehler commented May 6, 2022 • edited Loading

vince-fugnitto commented May 6, 2022

lucas-koehler commented May 6, 2022

vince-fugnitto commented May 6, 2022

lucas-koehler commented May 10, 2022

lucas-koehler commented May 16, 2022

vince-fugnitto left a comment

Choose a reason for hiding this comment

lucas-koehler commented May 17, 2022

vince-fugnitto commented May 17, 2022

lucas-koehler commented Aug 26, 2022

koegel commented Aug 31, 2022

vince-fugnitto left a comment

Choose a reason for hiding this comment

lucas-koehler commented Sep 2, 2022 • edited Loading

vince-fugnitto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-koehler commented Sep 2, 2022

vince-fugnitto commented Sep 8, 2022

lucas-koehler commented Sep 8, 2022

lucas-koehler commented Apr 20, 2022 •

edited

Loading

colin-grant-work commented May 4, 2022 •

edited

Loading

lucas-koehler commented May 5, 2022 •

edited

Loading

lucas-koehler commented May 6, 2022 •

edited

Loading

lucas-koehler commented Sep 2, 2022 •

edited

Loading