Add 'New Window' Command #9519

OmarSdt-EC · 2021-05-26T13:28:06Z

What it does

The pull-request adds the new window command for both the electron and browser targets along with an appropriate keybinding and menu item (file menu). The command is used to spawn a new application window without a workspace.

How to test

Open a project (or a file) in a new workspace
You can choose between these actions to spawn a new window :
- Entering ctrlcmd+shift+n (electron target) or ctrlcmd+alt+n (browser target)
- Clicking on the 'New Window' option in the 'File' menu
- Searching and clicking on the 'New Window' option after entering the command palette (CTRL + P) in the 'View' menu
Check that the new window displays the welcome page and that no workspace has been opened with it

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 · 2021-06-01T21:22:15Z

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

+/**
+ *  The window hash value that is used to spawn a new default window.
+ */
+export const DEFAULT_WINDOW_HASH: string = '#!empty';


I have mixed feelings about using this to solve the problem of opening without reloading the workspace.
Positive feelings:

Opening a new window does feel like the sort of thing that a WindowService ought to do, so locating that functionality in the core package makes sense.

Less positive feelings:

Knowing that the window hash represents the workspace maybe isn't a detail that core should know about, but a detail leaking out of the workspace package, which is clear given that the only function of this value is to stop the workspace service from doing what it would normally do. The workspace already has its own solution to this problem:

theia/packages/workspace/src/browser/workspace-service.ts

Lines 436 to 445 in 011f76e

/**

* Clears current workspace root.

*/

async close(): Promise<void> {

this._workspace = undefined;

this._roots.length = 0;

await this.server.setMostRecentlyUsedWorkspace('');

this.reloadWindow();

}

So I'm not sure exactly which way is best, but I'd prefer to see one solution to the problem of not loading a workspace rather than two.

Hi collin, do you have some ideas to implement this by using the WorkspaceService ?

I think in the end my complaint has more to do with misgivings about how we treat the URL bar (that we don't react to changes unless the user does a hard refresh, for example) than it does with your solution. In reality, core definitely does know what the hash is for (e.g. ElectronMainApplication.prototype.openWindowWithWorkspace() sure knows), so using a specific hash for an empty workspace isn't a problem.

Maybe change the close function in the workspace service to use the empty workspace fragment rather than using setMostRecentlyUsedWorkspace dishonestly, and I'll be thoroughly satisfied 🙂. Sorry for the detour.

Sorry to bother you again, but when you say the empty workspace fragment do you mean the DEFAULT_WINDOW_HASH ?

Yes, indeed, exactly. Sorry for the confusion in terminology. Mixing your HASH term and the withFragment method on URI's.

I think that I misunderstood something about what I have to change to the close method. For me, it is only used to clear the root by setting the most recent workspace uri to an empty string. Also, we use the empty hash value only to know when we are opening a default window and we are modifying the uri just after checking that condition with an empty string like it is shown in this picture :

If we modify the uri fragment with the hash value and let it like this, the root won't be clear anymore and a default window will not appear when we close the workspace.

colin-grant-work · 2021-06-01T21:32:49Z

packages/core/src/browser/window-contribution.ts

+    registerKeybindings(registry: KeybindingRegistry): void {
+        registry.registerKeybindings({
+            command: WindowCommands.NEW_WINDOW.id,
+            keybinding: this.isElectron() ? 'ctrlcmd+shift+n' : 'ctrlcmd+alt+n'


Usually (but not always) when we do alternate sequences for browser and Electron, it involves replacing ctrlcmd with alt, so ctrlcmd+shift+n -> alt+shift+n. Is there a reason not to do that here?

packages/core/src/browser/frontend-application-module.ts

colin-grant-work · 2021-06-01T22:04:10Z

This is working pretty well for me. Just some minor comments on the code.

colin-grant-work · 2021-06-10T22:55:01Z

Please bring this up to date with master and fix existing conflicts, and it should be good to go. If you'd like to make the change to the WorkspaceService.close() method at the same time, that would be great.

msujew · 2021-07-22T09:59:49Z

@OmarSdt-EC Are you still working on this? It looks good to me as well, I think the close method is fine as it is. I would approve it, once you rebased and fixed the conflicts.

OmarSdt-EC · 2021-07-22T10:59:20Z

@msujew Yes, I'm still working on it. I will rebase it and fix the conflicts thank you !

msujew

Alright, Code LGTM and everything works well 👍

The following commit adds support for the 'New Window' command for both electron and browser targets. Also, it prevents a new electron or browser window to be spawned with a workspace. It includes : - The registration of the keybindings used in each target. - The registration of the command in the different menus of each target.

vince-fugnitto · 2021-08-30T14:15:26Z

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

-        electronWindow.loadURL(uri.toString(true));
+    async openDefaultWindow(): Promise<BrowserWindow> {
+        const [uri, electronWindow] = await Promise.all([this.createWindowUri(), this.createWindow()]);
+        electronWindow.loadURL(uri.withFragment(DEFAULT_WINDOW_HASH).toString(true));


This change means that we will not restore the previous workspace if we do the following:

start the example-electron application

open a workspace folder

close the application

re-start the application - the previous workspace is not restored and instead we fallback to the default state (without a workspace).

The following commit adds support for the 'New Window' command for both electron and browser targets. Also, it prevents a new electron or browser window to be spawned with a workspace. It includes : - The registration of the keybindings used in each target. - The registration of the command in the different menus of each target.

vince-fugnitto force-pushed the os/fix-command-NewWindow branch from 68d1e0e to 16bbc48 Compare May 26, 2021 13:59

OmarSdt-EC force-pushed the os/fix-command-NewWindow branch from 16bbc48 to 5d0295e Compare May 26, 2021 14:10

OmarSdt-EC changed the title ~~Os/fix command new window~~ Add 'New Window' Command May 26, 2021

vince-fugnitto added commands issues related to application commands electron issues related to the electron target workspace issues related to the workspace browser issues specific to the browser use-case labels May 26, 2021

vince-fugnitto requested a review from colin-grant-work June 1, 2021 16:47

colin-grant-work reviewed Jun 1, 2021

View reviewed changes

OmarSdt-EC force-pushed the os/fix-command-NewWindow branch from 5d0295e to 3822488 Compare June 10, 2021 13:28

OmarSdt-EC force-pushed the os/fix-command-NewWindow branch from 3822488 to e5e156a Compare July 22, 2021 12:48

msujew approved these changes Jul 26, 2021

View reviewed changes

OmarSdt-EC force-pushed the os/fix-command-NewWindow branch from e5e156a to 1f103bc Compare August 23, 2021 16:21

OmarSdt-EC force-pushed the os/fix-command-NewWindow branch from 1f103bc to c0be505 Compare August 26, 2021 18:04

paul-marechal merged commit fe69e5e into eclipse-theia:master Aug 26, 2021

paul-marechal added this to the 1.17.0 milestone Aug 26, 2021

vince-fugnitto reviewed Aug 30, 2021

View reviewed changes

vince-fugnitto mentioned this pull request Aug 30, 2021

electron: starting application does not restore previous workspace #9997

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'New Window' Command #9519

Add 'New Window' Command #9519

OmarSdt-EC commented May 26, 2021 •

edited by vince-fugnitto

Loading

colin-grant-work Jun 1, 2021

OmarSdt-EC Jun 10, 2021

colin-grant-work Jun 10, 2021 •

edited

Loading

OmarSdt-EC Jun 11, 2021 •

edited

Loading

colin-grant-work Jun 11, 2021

OmarSdt-EC Jun 11, 2021

colin-grant-work Jun 1, 2021 •

edited

Loading

colin-grant-work commented Jun 1, 2021

colin-grant-work commented Jun 10, 2021

msujew commented Jul 22, 2021

OmarSdt-EC commented Jul 22, 2021 •

edited

Loading

msujew left a comment

vince-fugnitto Aug 30, 2021

	/**
	* Clears current workspace root.
	*/
	async close(): Promise<void> {
	this._workspace = undefined;
	this._roots.length = 0;

	await this.server.setMostRecentlyUsedWorkspace('');
	this.reloadWindow();
	}

Add 'New Window' Command #9519

Add 'New Window' Command #9519

Conversation

OmarSdt-EC commented May 26, 2021 • edited by vince-fugnitto Loading

What it does

How to test

Review checklist

Reminder for reviewers

colin-grant-work Jun 1, 2021

Choose a reason for hiding this comment

OmarSdt-EC Jun 10, 2021

Choose a reason for hiding this comment

colin-grant-work Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

OmarSdt-EC Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

colin-grant-work Jun 11, 2021

Choose a reason for hiding this comment

OmarSdt-EC Jun 11, 2021

Choose a reason for hiding this comment

colin-grant-work Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

colin-grant-work commented Jun 1, 2021

colin-grant-work commented Jun 10, 2021

msujew commented Jul 22, 2021

OmarSdt-EC commented Jul 22, 2021 • edited Loading

msujew left a comment

Choose a reason for hiding this comment

vince-fugnitto Aug 30, 2021

Choose a reason for hiding this comment

OmarSdt-EC commented May 26, 2021 •

edited by vince-fugnitto

Loading

colin-grant-work Jun 10, 2021 •

edited

Loading

OmarSdt-EC Jun 11, 2021 •

edited

Loading

colin-grant-work Jun 1, 2021 •

edited

Loading

OmarSdt-EC commented Jul 22, 2021 •

edited

Loading