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

added Center Mode (#15684) #40757

Merged
merged 17 commits into from
Feb 21, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions src/vs/code/electron-main/menus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ export class CodeMenu {

const fullscreen = new MenuItem(this.withKeybinding('workbench.action.toggleFullScreen', { label: this.mnemonicLabel(nls.localize({ key: 'miToggleFullScreen', comment: ['&& denotes a mnemonic'] }, "Toggle &&Full Screen")), click: () => this.windowsMainService.getLastActiveWindow().toggleFullScreen(), enabled: this.windowsMainService.getWindowCount() > 0 }));
const toggleZenMode = this.createMenuItem(nls.localize('miToggleZenMode', "Toggle Zen Mode"), 'workbench.action.toggleZenMode');
const toggleCenteredLayout = this.createMenuItem(nls.localize('miToggleCenteredLayout', "Toggle centered Layout"), 'workbench.action.toggleCenteredLayout');
Copy link
Contributor

Choose a reason for hiding this comment

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

Use upper case to be consistent with other menu entries, so "Toggle Centered Layout"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

const toggleMenuBar = this.createMenuItem(nls.localize({ key: 'miToggleMenuBar', comment: ['&& denotes a mnemonic'] }, "Toggle Menu &&Bar"), 'workbench.action.toggleMenuBar');
const splitEditor = this.createMenuItem(nls.localize({ key: 'miSplitEditor', comment: ['&& denotes a mnemonic'] }, "Split &&Editor"), 'workbench.action.splitEditor');
const toggleEditorLayout = this.createMenuItem(nls.localize({ key: 'miToggleEditorLayout', comment: ['&& denotes a mnemonic'] }, "Toggle Editor Group &&Layout"), 'workbench.action.toggleEditorGroupLayout');
Expand Down Expand Up @@ -747,6 +748,7 @@ export class CodeMenu {
__separator__(),
fullscreen,
toggleZenMode,
toggleCenteredLayout,
isWindows || isLinux ? toggleMenuBar : void 0,
__separator__(),
splitEditor,
Expand Down
36 changes: 36 additions & 0 deletions src/vs/workbench/browser/actions/toggleCenteredLayout.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { TPromise } from 'vs/base/common/winjs.base';
import nls = require('vs/nls');
import { Action } from 'vs/base/common/actions';
import { KeyCode, KeyMod, KeyChord } from 'vs/base/common/keyCodes';
import { Registry } from 'vs/platform/registry/common/platform';
import { SyncActionDescriptor } from 'vs/platform/actions/common/actions';
import { IWorkbenchActionRegistry, Extensions } from 'vs/workbench/common/actions';
import { IPartService } from 'vs/workbench/services/part/common/partService';

class ToggleCenteredLayout extends Action {

public static readonly ID = 'workbench.action.toggleCenteredLayout';
public static readonly LABEL = nls.localize('toggleCenteredLayout', "Toggle Centered Layout");

constructor(
id: string,
label: string,
@IPartService private partService: IPartService
) {
super(id, label);
this.enabled = !!this.partService;
}

public run(): TPromise<any> {
this.partService.toggleCenteredLayout();
return TPromise.as(null);
}
}

const registry = Registry.as<IWorkbenchActionRegistry>(Extensions.WorkbenchActions);
registry.registerWorkbenchAction(new SyncActionDescriptor(ToggleCenteredLayout, ToggleCenteredLayout.ID, ToggleCenteredLayout.LABEL, { primary: KeyChord(KeyMod.CtrlCmd | KeyCode.KEY_K, KeyCode.KEY_T) }), 'View: Toggle Centered Layout', nls.localize('view', "View"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not use a default shortcut for this. So just leave undefined.
The issue with this one is that it is very similar to the CMD + K, CMD + T, which is taken by show theme
We can easily add a default keybinding later if people ask

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

165 changes: 162 additions & 3 deletions src/vs/workbench/browser/parts/editor/editorGroupsControl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import { attachProgressBarStyler } from 'vs/platform/theme/common/styler';
import { IDisposable, combinedDisposable } from 'vs/base/common/lifecycle';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only file I did not yet review. Will first try out the feature a bit.

import { ResourcesDropHandler, LocalSelectionTransfer, DraggedEditorIdentifier } from 'vs/workbench/browser/dnd';
import { ITelemetryService } from 'vs/platform/telemetry/common/telemetry';
import { IPartService } from 'vs/workbench/services/part/common/partService';
import { TextResourceEditor } from 'vs/workbench/browser/parts/editor/textResourceEditor';

export enum Rochade {
NONE,
Expand Down Expand Up @@ -101,6 +103,8 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro

private static readonly EDITOR_TITLE_HEIGHT = 35;

private static readonly CENTERED_EDITOR_MIN_MARGIN = 10;

private static readonly SNAP_TO_MINIMIZED_THRESHOLD_WIDTH = 50;
private static readonly SNAP_TO_MINIMIZED_THRESHOLD_HEIGHT = 20;

Expand All @@ -125,6 +129,22 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro
private sashTwo: Sash;
private startSiloThreeSize: number;

// if the centered layout is activated, the editor inside of silo ONE is centered
// the silo will then contain:
// [left margin]|[editor]|[right margin]
// - The size of the editor is defined by centeredEditorSize
// - The position is defined by the ratio centeredEditorLeftMarginRatio = left-margin/(left-margin + editor + right-margin).
// - The two sashes can be used to control the size and position of the editor inside of the silo.
// - In order to seperate the two sashes from the sashes that control the size of bordering widgets
// CENTERED_EDITOR_MIN_MARGIN is forced as a minimum size for the two margins.
private centeredEditorActive: boolean;
private centeredEditorSashLeft: Sash;
private centeredEditorSashRight: Sash;
private centeredEditorPreferedSize: number;
private centeredEditorLeftMarginRatio: number;
private centeredEditorDragStartPosition: number;
private centeredEditorDragStartSize: number;

private visibleEditors: BaseEditor[];

private lastActiveEditor: BaseEditor;
Expand All @@ -144,6 +164,7 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro
groupOrientation: GroupOrientation,
@IWorkbenchEditorService private editorService: IWorkbenchEditorService,
@IEditorGroupService private editorGroupService: IEditorGroupService,
@IPartService private partService: IPartService,
@IContextKeyService private contextKeyService: IContextKeyService,
@IExtensionService private extensionService: IExtensionService,
@IInstantiationService private instantiationService: IInstantiationService,
Expand Down Expand Up @@ -193,6 +214,10 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro
return this.layoutVertically ? EditorGroupsControl.MIN_EDITOR_WIDTH : EditorGroupsControl.MIN_EDITOR_HEIGHT;
}

private get visibleSilos(): number {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed, use this.editorGroupService.getStacksModel().groups.length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

return this.visibleEditors.reduce((acc, min) => min ? acc + 1 : acc, 0);
}

private isSiloMinimized(position: number): boolean {
return this.silosSize[position] === this.minSize && this.silosMinimized[position];
}
Expand All @@ -209,6 +234,22 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro
});
}

private get centeredEditorAvailableSize(): number {
return this.silosSize[0] - EditorGroupsControl.CENTERED_EDITOR_MIN_MARGIN * 2;
}

private get centeredEditorSize(): number {
return Math.min(this.centeredEditorAvailableSize, this.centeredEditorPreferedSize);
}

private get centeredEditorPosition(): number {
return EditorGroupsControl.CENTERED_EDITOR_MIN_MARGIN + this.centeredEditorLeftMarginRatio * (this.centeredEditorAvailableSize - this.centeredEditorSize);
}

private get centeredEditorEndPosition(): number {
return this.centeredEditorPosition + this.centeredEditorSize;
}

private get snapToMinimizeThresholdSize(): number {
return this.layoutVertically ? EditorGroupsControl.SNAP_TO_MINIMIZED_THRESHOLD_WIDTH : EditorGroupsControl.SNAP_TO_MINIMIZED_THRESHOLD_HEIGHT;
}
Expand Down Expand Up @@ -969,6 +1010,23 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro
// Silo Three
this.silos[Position.THREE] = $(this.parent).div({ class: 'one-editor-silo editor-three' });

// Center Layout stuff
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor. Feels like code duplication, we do the creation and listeners twice. Can't we have a helper method that just takes a centered sash and nicely initilizes it. For that the DragStart and Drag methods would have to take sashes as parameters and based on Sash do different things.
Which might not be too bad, currently I feel like there are too many methods.

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 don't think that's a good idea... that would make the individual method body larger... and break the coding style for sashes... I mean the class is pretty big but that would require an additional PR that really breaks the class apart. I think it's much more clean the way it is... what we could do is outfactor the initialization for the sashes. I did that in this commit SrTobi-Forks@abccb77 (it's not part of this PR at the moment) but I am not really sure if that makes the code more structured (in fact, it adds one more method 😄 ).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense

this.centeredEditorSashLeft = new Sash(this.parent.getHTMLElement(), this, { baseSize: 5, orientation: Orientation.VERTICAL });
this.toUnbind.push(this.centeredEditorSashLeft.onDidStart(() => this.onCenterSashLeftDragStart()));
this.toUnbind.push(this.centeredEditorSashLeft.onDidChange((e: ISashEvent) => this.onCenterSashLeftDrag(e)));
this.toUnbind.push(this.centeredEditorSashLeft.onDidEnd(() => this.onCenterSashLeftDragEnd()));
this.toUnbind.push(this.centeredEditorSashLeft.onDidReset(() => this.resetCenteredEditor()));

this.centeredEditorSashRight = new Sash(this.parent.getHTMLElement(), this, { baseSize: 5, orientation: Orientation.VERTICAL });
this.toUnbind.push(this.centeredEditorSashRight.onDidStart(() => this.onCenterSashRightDragStart()));
this.toUnbind.push(this.centeredEditorSashRight.onDidChange((e: ISashEvent) => this.onCenterSashRightDrag(e)));
this.toUnbind.push(this.centeredEditorSashRight.onDidEnd(() => this.onCenterSashRightDragEnd()));
this.toUnbind.push(this.centeredEditorSashRight.onDidReset(() => this.resetCenteredEditor()));

this.centeredEditorActive = false;
this.centeredEditorLeftMarginRatio = 0.5;
this.centeredEditorPreferedSize = -1; // set this later if we know the container size

// For each position
POSITIONS.forEach(position => {
const silo = this.silos[position];
Expand Down Expand Up @@ -1858,12 +1916,70 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro
this.sashTwo.layout();
}

private setCenteredEditorPositionAndSize(pos: number, size: number): void {
this.centeredEditorPreferedSize = Math.max(this.minSize, size);
pos -= EditorGroupsControl.CENTERED_EDITOR_MIN_MARGIN;
pos = Math.min(pos, this.centeredEditorAvailableSize - this.centeredEditorSize);
pos = Math.max(0, pos);
this.centeredEditorLeftMarginRatio = pos / (this.centeredEditorAvailableSize - this.centeredEditorSize);

this.layoutContainers();
}

private onCenterSashLeftDragStart(): void {
this.centeredEditorDragStartPosition = this.centeredEditorPosition;
this.centeredEditorDragStartSize = this.centeredEditorSize;
}

private onCenterSashLeftDrag(e: ISashEvent): void {
const minMargin = EditorGroupsControl.CENTERED_EDITOR_MIN_MARGIN;
const diffPos = e.currentX - e.startX;
const diffSize = -diffPos;

const pos = this.centeredEditorDragStartPosition + diffPos;
const size = this.centeredEditorDragStartSize + diffSize;
this.setCenteredEditorPositionAndSize(pos, pos <= minMargin ? size + pos - minMargin : size);
}

private onCenterSashLeftDragEnd(): void {
this.layoutContainers();
}

private onCenterSashRightDragStart(): void {
this.centeredEditorDragStartPosition = this.centeredEditorPosition;
this.centeredEditorDragStartSize = this.centeredEditorSize;
}

private onCenterSashRightDrag(e: ISashEvent): void {
const diffPos = e.currentX - e.startX;
const diffSize = diffPos;

const pos = this.centeredEditorDragStartPosition;
const maxSize = this.centeredEditorAvailableSize - this.centeredEditorDragStartPosition;
const size = Math.min(maxSize, this.centeredEditorDragStartSize + diffSize);
this.setCenteredEditorPositionAndSize(size < this.minSize ? pos + (size - this.minSize) : pos, size);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems funky, that on sashLeftDragEnd you are layouting containers, and on sahRightDragEnd you do no action.
Why is layout needed when the sash event ends, it feels to me like everything is getting nicely layouting continously and an explicit call at the end is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!? I removed the call to this.layoutContainers from onCenterSashLeftDragEnd.

private onCenterSashRightDragEnd(): void {
}

public getVerticalSashTop(sash: Sash): number {
return 0;
}

public getVerticalSashLeft(sash: Sash): number {
return sash === this.sashOne ? this.silosSize[Position.ONE] : this.silosSize[Position.TWO] + this.silosSize[Position.ONE];
switch (sash) {
case this.sashOne:
return this.silosSize[Position.ONE];
case this.sashTwo:
return this.silosSize[Position.TWO] + this.silosSize[Position.ONE];
case this.centeredEditorSashLeft:
return this.centeredEditorPosition;
case this.centeredEditorSashRight:
return this.centeredEditorEndPosition;
default:
return 0;
}
}

public getVerticalSashHeight(sash: Sash): number {
Expand Down Expand Up @@ -2013,6 +2129,32 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro
}
});

// Layout centered Editor
const doCentering =
this.layoutVertically &&
this.visibleSilos === 1 &&
this.partService.isCenteredLayoutActive() &&
this.visibleEditors[Position.ONE] instanceof TextResourceEditor;
Copy link
Contributor

Choose a reason for hiding this comment

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

This instanceof check is definetely not needed, it makes the behavior of this feature unpredictable, and I am not sure what is the reason for this limiation

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah definetely remove this instanceof check, since the feature does not behave like it should with it. @bpasero was unable to try it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


if (doCentering && !this.centeredEditorActive) {
this.centeredEditorSashLeft.show();
this.centeredEditorSashRight.show();

// no size set yet. Calculate a default value
if (this.centeredEditorPreferedSize === -1) {
this.resetCenteredEditor(false);
}
} else if (!doCentering && this.centeredEditorActive) {
this.centeredEditorSashLeft.hide();
this.centeredEditorSashRight.hide();
}
this.centeredEditorActive = doCentering;

if (this.centeredEditorActive) {
this.centeredEditorSashLeft.layout();
this.centeredEditorSashRight.layout();
}

// Layout visible editors
POSITIONS.forEach(position => {
this.layoutEditor(position);
Expand All @@ -2031,10 +2173,18 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro

private layoutEditor(position: Position): void {
const editorSize = this.silosSize[position];
if (editorSize && this.visibleEditors[position]) {
const editor = this.visibleEditors[position];
if (editorSize && editor) {
let editorWidth = this.layoutVertically ? editorSize : this.dimension.width;
let editorHeight = (this.layoutVertically ? this.dimension.height : this.silosSize[position]) - EditorGroupsControl.EDITOR_TITLE_HEIGHT;

let editorPosition = 0;

if (this.centeredEditorActive) {
editorWidth = this.centeredEditorSize;
editorPosition = this.centeredEditorPosition;
}

if (position !== Position.ONE) {
if (this.layoutVertically) {
editorWidth--; // accomodate for 1px left-border in containers TWO, THREE when laying out vertically
Expand All @@ -2043,7 +2193,16 @@ export class EditorGroupsControl extends Themable implements IEditorGroupsContro
}
}

this.visibleEditors[position].layout(new Dimension(editorWidth, editorHeight));
editor.getContainer().style({ 'margin-left': editorPosition + 'px', 'width': editorWidth + 'px' });
editor.layout(new Dimension(editorWidth, editorHeight));
}
}

private resetCenteredEditor(layout: boolean = true) {
this.centeredEditorLeftMarginRatio = 0.5;
this.centeredEditorPreferedSize = Math.floor(this.dimension.width * 0.5);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the golden ratio as the default value.
So this.dimension.width * 0.61

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. nice idea.

if (layout) {
this.layoutContainers();
}
}

Expand Down
26 changes: 26 additions & 0 deletions src/vs/workbench/electron-browser/workbench.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ export class Workbench implements IPartService {
private static readonly sidebarRestoreStorageKey = 'workbench.sidebar.restore';
private static readonly panelHiddenStorageKey = 'workbench.panel.hidden';
private static readonly zenModeActiveStorageKey = 'workbench.zenmode.active';
private static readonly centeredLayoutActiveStorageKey = 'workbench.centeredlayout.active';
private static readonly panelPositionStorageKey = 'workbench.panel.location';
private static readonly defaultPanelPositionStorageKey = 'workbench.panel.defaultLocation';

Expand Down Expand Up @@ -213,6 +214,7 @@ export class Workbench implements IPartService {
wasSideBarVisible: boolean;
wasPanelVisible: boolean;
};
private centeredLayoutActive: boolean;

constructor(
parent: HTMLElement,
Expand Down Expand Up @@ -379,6 +381,11 @@ export class Workbench implements IPartService {
this.toggleZenMode(true);
}

// Restore Forced Center Mode
if (this.storageService.getBoolean(Workbench.centeredLayoutActiveStorageKey, StorageScope.GLOBAL, false)) {
this.centeredLayoutActive = true;
}

const onRestored = (error?: Error): IWorkbenchStartedInfo => {
this.workbenchCreated = true;

Expand Down Expand Up @@ -660,6 +667,9 @@ export class Workbench implements IPartService {
wasSideBarVisible: false,
wasPanelVisible: false
};

// Centered Layout
this.centeredLayoutActive = false;
}

private setPanelPositionFromStorageOrConfig() {
Expand Down Expand Up @@ -1313,6 +1323,22 @@ export class Workbench implements IPartService {
}
}

public isCenteredLayoutActive(): boolean {
return this.centeredLayoutActive;
}

public toggleCenteredLayout(): void {
this.centeredLayoutActive = !this.centeredLayoutActive;

if (this.centeredLayoutActive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would replace this if statement with
this.storageService.store(Workbench.centeredLayoutActiveStorageKey, this.centeredLayoutActive, StorageScope.GLOBAL);

Seems more straighforwared to me.
It seems like we were doing this clunky business in zen mode so you took it from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually you doing this 'clunky' business all the time for stored boolean values.

this.storageService.store(Workbench.centeredLayoutActiveStorageKey, 'true', StorageScope.GLOBAL);
} else {
this.storageService.remove(Workbench.centeredLayoutActiveStorageKey, StorageScope.GLOBAL);
}

this.layout();
}

// Resize requested part along the main axis
// layout will do all the math for us and adjusts the other Parts
public resizePart(part: Parts, sizeChange: number): void {
Expand Down
10 changes: 10 additions & 0 deletions src/vs/workbench/services/part/common/partService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,16 @@ export interface IPartService {
*/
toggleZenMode(): void;

/**
* Returns whether the centered layout is active.
*/
isCenteredLayoutActive(): boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this to isLayoutCentered
Notice how we do not use "active" for similar methods in the same service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


/**
* Toggles the workbench in and out of centered layout.
*/
toggleCenteredLayout(): void;

/**
* Resizes currently focused part on main access
*/
Expand Down
4 changes: 4 additions & 0 deletions src/vs/workbench/test/workbenchTestServices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,10 @@ export class TestPartService implements IPartService {

public toggleZenMode(): void { }

public isCenteredLayoutActive(): boolean { return false; }
public toggleCenteredLayout(): void { }


public resizePart(part: Parts, sizeChange: number): void { }
}

Expand Down
Loading