Multi window support for web views #11048
Conversation
lucas-koehler
force-pushed
the
multi-window-mvp
branch
from
93d63d6
to
5eddd35
Compare
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // 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'); |
There was a problem hiding this comment.
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.
| @@ -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 { | |||
There was a problem hiding this comment.
Would it be appropriate to implement this as part of the startup routine of the TheiaElectronWindow class?
There was a problem hiding this comment.
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.
| export const EXTRACT_WIDGET = Command.toDefaultLocalizedCommand({ | ||
| id: 'extract-widget', | ||
| label: 'Move View to Secondary Window' | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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');| // sanity check | ||
| if (!ExtractableWidget.is(widget)) { | ||
| // command executed with a non-extractable widget | ||
| console.error('Invalid command execution'); |
There was a problem hiding this comment.
More detail should be provided here:
| console.error('Invalid command execution'); | |
| console.error('Invalid attempt to move non-extractable widget to secondary window.'); |
scripts/patch-libraries.js
Outdated
| const patches = [ | ||
| { | ||
| file: 'node_modules/@phosphor/widgets/lib/widget.js', | ||
| find: /^.*?'Host is not attached.'.*?$/gm, |
There was a problem hiding this comment.
Could use a comment explaining what this modifies:
| 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, |
|
@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. |
|
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. |
|
@colin-grant-work That is a good point, thanks. I added the following limitation to the PR description:
|
|
@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! |
|
@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. |
|
Perfect, thanks! |
CHANGELOG.md
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
After rebasing please ensure that this entry is added under v1.26.0.
| </head> | ||
|
|
||
| <body> | ||
| <div id="pwidget"></div> |
There was a problem hiding this comment.
Can we find a better name for this id than pwidget?
There was a problem hiding this comment.
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.
| 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.'); |
There was a problem hiding this comment.
| 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.'); |
| constructor( | ||
| @inject(MessageService) protected readonly messageService: MessageService, | ||
| @inject(WindowService) protected readonly windowService: WindowService | ||
| ) { } |
There was a problem hiding this comment.
I believe we should use property injection here?
| @@ -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, | |||
There was a problem hiding this comment.
I believe this is a breaking change that should be documented:
lucas-koehler
force-pushed
the
multi-window-mvp
branch
from
9ac7d87
to
87cf80a
Compare
|
Hi @vince-fugnitto , thank you for your review :) |
There was a problem hiding this comment.
@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
.drawiofile - the drawio integration will open the drawio editor - extract the view to secondary window
- minimize the secondary window
- re-select the same
.drawiofile 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.
|
@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:
I would therefore propose the following:
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. |
|
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:
|
@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 |
@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 |
@vince-fugnitto Seems reasonable. I will investigate this later this week. |
lucas-koehler
force-pushed
the
multi-window-mvp
branch
from
87cf80a
to
ad86315
Compare
|
Hello @vince-fugnitto , 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. |
packages/core/src/electron-browser/window/electron-secondary-window-service.ts
Outdated
Show resolved
Hide resolved
packages/secondary-window-ui/src/browser/secondary-window-ui-frontend-contribution.ts
Outdated
Show resolved
Hide resolved
|
@vince-fugnitto I fixed the minor comments. I'll address the phosphor comment here for better visibility.
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:
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 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 Let's go with your second proposal 👍 |
lucas-koehler
force-pushed
the
multi-window-mvp
branch
from
d87ec25
to
c13f4b8
Compare
|
Rebased changes onto 1.29.0 |
|
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. 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: |
There was a problem hiding this comment.
@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
force-pushed
the
multi-window-mvp
branch
from
14a7b07
to
853e9db
Compare
|
Hello @vince-fugnitto , As I believe you discussed yesterday with @koegel, I incorporated parts of your suggestions:
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.
They were left in to save the effort of extracting them just to re-add them again soon. However, the 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: |
lucas-koehler
requested review from
vince-fugnitto
and removed request for
msujew,
alvsan09 and
paul-marechal
| { | ||
| "name": "@theia/secondary-window", | ||
| "version": "1.29.0", | ||
| "description": "Theia - Secondary window UI contributions", |
There was a problem hiding this comment.
minor:
| "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 <[email protected]> Co-authored-by: robmor01 <[email protected]> Signed-off-by: Lucas Koehler <[email protected]>
lucas-koehler
force-pushed
the
multi-window-mvp
branch
from
853e9db
to
97f6360
Compare
|
@vince-fugnitto Thank you for the fast review. I squashed the commits and added your suggestion for the extension title |
vince-fugnitto
added
the
secondary-window
|
@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. |
|
@vince-fugnitto Thank you for your effort and the reminder :) |
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:
widget-extraction-uisecondaryWindowarea that contains all extracted widgetsWebviews 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.
Contributed on behalf of ST Microelectronics and Ericsson and by ARM and EclipseSource.
Co-authored-by: Stefan Dirix [email protected]
Co-authored-by: robmor01 [email protected]
Signed-off-by: Lucas Koehler [email protected]
pr-ubuntu.mp4
Limitations
currentTitleis not reset toundefinedwhen the last widget is moved elsewhere #10895 and also happens when moving widgets inside the main windowScreenshots
Ubuntu (click to expand)
Windows 11 (click to expand)
Mac (click to expand)
How to test
pluginsfolder. For instance:2btfas example inputReview checklist
Reminder for reviewers