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

Add 'New Window' Command #9519

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/core/src/browser/frontend-application-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ import {
import { QuickAccessContribution } from './quick-input/quick-access';
import { QuickCommandService } from './quick-input/quick-command-service';
import { SidebarBottomMenuWidget } from './shell/sidebar-bottom-menu-widget';
import { WindowContribution } from './window-contribution';

export { bindResourceProvider, bindMessageService, bindPreferenceService };

Expand Down Expand Up @@ -356,4 +357,8 @@ export const frontendApplicationModule = new ContainerModule((bind, unbind, isBo
bind(CredentialsService).to(CredentialsServiceImpl);

bind(ContributionFilterRegistry).to(ContributionFilterRegistryImpl).inSingletonScope();
bind(WindowContribution).toSelf().inSingletonScope();
for (const contribution of [CommandContribution, KeybindingContribution, MenuContribution]) {
bind(contribution).toService(WindowContribution);
}
colin-grant-work marked this conversation as resolved.
Show resolved Hide resolved
});
64 changes: 64 additions & 0 deletions packages/core/src/browser/window-contribution.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/********************************************************************************
* Copyright (C) 2021 Ericsson and others.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0.
*
* This Source Code may also be made available under the following Secondary
* Licenses when the conditions for such availability set forth in the Eclipse
* Public License v. 2.0 are satisfied: GNU General Public License, version 2
* with the GNU Classpath Exception which is available at
* https://www.gnu.org/software/classpath/license.html.
*
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/

import { inject, injectable } from 'inversify';
import { Command, CommandContribution, CommandRegistry, environment } from '../common';
import { WindowService } from './window/window-service';
import { KeybindingContribution, KeybindingRegistry } from './keybinding';
import { MenuContribution, MenuModelRegistry } from '../common/menu';
import { CommonMenus } from '../browser/common-frontend-contribution';

export namespace WindowCommands {

export const NEW_WINDOW: Command = {
id: 'workbench.action.newWindow',
label: 'New Window'
};
}

@injectable()
export class WindowContribution implements CommandContribution, KeybindingContribution, MenuContribution {

@inject(WindowService)
protected windowService: WindowService;

registerCommands(commands: CommandRegistry): void {
commands.registerCommand(WindowCommands.NEW_WINDOW, {
execute: () => {
this.windowService.openNewDefaultWindow();
}
});
}

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

registerMenus(registry: MenuModelRegistry): void {
registry.registerMenuAction(CommonMenus.FILE_NEW, {
commandId: WindowCommands.NEW_WINDOW.id,
order: 'c'
});
}

private isElectron(): boolean {
return environment.electron.is();
}

}
5 changes: 5 additions & 0 deletions packages/core/src/browser/window/default-window-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { CorePreferences } from '../core-preferences';
import { ContributionProvider } from '../../common/contribution-provider';
import { FrontendApplicationContribution, FrontendApplication } from '../frontend-application';
import { WindowService } from './window-service';
import { DEFAULT_WINDOW_HASH } from './window-service';

@injectable()
export class DefaultWindowService implements WindowService, FrontendApplicationContribution {
Expand Down Expand Up @@ -48,6 +49,10 @@ export class DefaultWindowService implements WindowService, FrontendApplicationC
return undefined;
}

openNewDefaultWindow(): void {
this.openNewWindow(DEFAULT_WINDOW_HASH);
}

canUnload(): boolean {
const confirmExit = this.corePreferences['application.confirmExit'];
let preventUnload = confirmExit === 'always';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { WindowService } from '../window-service';
@injectable()
export class MockWindowService implements WindowService {
openNewWindow(): undefined { return undefined; }
openNewDefaultWindow(): void { }
canUnload(): boolean { return true; }
get onUnload(): Event<void> { return Event.None; }
}
12 changes: 12 additions & 0 deletions packages/core/src/browser/window/window-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ export interface NewWindowOptions {
* Service for opening new browser windows.
*/
export const WindowService = Symbol('WindowService');

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

/**
* 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@colin-grant-work colin-grant-work Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@OmarSdt-EC OmarSdt-EC Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :
Capture
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.


export interface WindowService {

/**
Expand All @@ -33,6 +39,12 @@ export interface WindowService {
*/
openNewWindow(url: string, options?: NewWindowOptions): undefined;

/**
* Opens a new default window.
* - In electron and in the browser it will open the default window without a pre-defined content.
*/
openNewDefaultWindow(): void;

/**
* Called when the `window` is about to `unload` its resources.
* At this point, the `document` is still visible and the [`BeforeUnloadEvent`](https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ export class ElectronWindowService extends DefaultWindowService {
return undefined;
}

openNewDefaultWindow(): void {
this.delegate.openNewDefaultWindow();
}

@postConstruct()
protected init(): void {
// Update the default zoom level on startup when the preferences event is fired.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ export const electronMainWindowServicePath = '/services/electron-window';
export const ElectronMainWindowService = Symbol('ElectronMainWindowService');
export interface ElectronMainWindowService {
openNewWindow(url: string, options?: NewWindowOptions): undefined;
openNewDefaultWindow(): void;
}
8 changes: 4 additions & 4 deletions packages/core/src/electron-main/electron-main-application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { ContributionProvider } from '../common/contribution-provider';
import { ElectronSecurityTokenService } from './electron-security-token-service';
import { ElectronSecurityToken } from '../electron-common/electron-token';
import Storage = require('electron-store');
import { DEFAULT_WINDOW_HASH } from '../browser/window/window-service';
const createYargs: (argv?: string[], cwd?: string) => Argv = require('yargs/yargs');

/**
Expand Down Expand Up @@ -268,10 +269,9 @@ export class ElectronMainApplication {
};
}

protected async openDefaultWindow(): Promise<BrowserWindow> {
const options = await this.getLastWindowOptions();
const [uri, electronWindow] = await Promise.all([this.createWindowUri(), this.createWindow(options)]);
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. start the example-electron application
  2. open a workspace folder
  3. close the application
  4. re-start the application - the previous workspace is not restored and instead we fallback to the default state (without a workspace).

return electronWindow;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,8 @@ export class ElectronMainWindowServiceImpl implements ElectronMainWindowService
return undefined;
}

openNewDefaultWindow(): void {
this.app.openDefaultWindow();
}

}
6 changes: 4 additions & 2 deletions packages/workspace/src/browser/workspace-commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,12 @@ export class FileMenuContribution implements MenuContribution {

registerMenus(registry: MenuModelRegistry): void {
registry.registerMenuAction(CommonMenus.FILE_NEW, {
commandId: WorkspaceCommands.NEW_FILE.id
commandId: WorkspaceCommands.NEW_FILE.id,
order: 'a'
});
registry.registerMenuAction(CommonMenus.FILE_NEW, {
commandId: WorkspaceCommands.NEW_FOLDER.id
commandId: WorkspaceCommands.NEW_FOLDER.id,
order: 'b'
});
const downloadUploadMenu = [...CommonMenus.FILE, '4_downloadupload'];
registry.registerMenuAction(downloadUploadMenu, {
Expand Down
9 changes: 8 additions & 1 deletion packages/workspace/src/browser/workspace-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import { injectable, inject, postConstruct } from '@theia/core/shared/inversify';
import URI from '@theia/core/lib/common/uri';
import { WorkspaceServer, THEIA_EXT, VSCODE_EXT, getTemporaryWorkspaceFileUri } from '../common';
import { WindowService } from '@theia/core/lib/browser/window/window-service';
import { DEFAULT_WINDOW_HASH, WindowService } from '@theia/core/lib/browser/window/window-service';
import {
FrontendApplicationContribution, PreferenceServiceImpl, PreferenceScope, PreferenceSchemaProvider, LabelProvider
} from '@theia/core/lib/browser';
Expand Down Expand Up @@ -128,6 +128,13 @@ export class WorkspaceService implements FrontendApplicationContribution {
}

protected async doGetDefaultWorkspaceUri(): Promise<string | undefined> {

// If an empty window is explicitly requested do not restore a previous workspace.
if (window.location.hash === DEFAULT_WINDOW_HASH) {
window.location.hash = '';
return undefined;
}

// Prefer the workspace path specified as the URL fragment, if present.
if (window.location.hash.length > 1) {
// Remove the leading # and decode the URI.
Expand Down