Inline the toolbar for macOS #12628

Closed
wants to merge 45 commits into
from

Projects

None yet

8 participants

@orta
Contributor
orta commented Sep 26, 2016 edited

Moves the traffic buttons on the window into the space above the activity bar on macOS - for full context see #12377

screen shot 2016-09-26 at 13 20 52

screen shot 2016-09-26 at 13 20 44


TODO:

  • Wrap up the config setting
  • migrate all CSS to be scoped by .use-inline-toolbar

fixes #12377

@msftclas

Hi @orta, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@coveralls

Coverage Status

Coverage increased (+0.002%) to 61.217% when pulling 764ecbd on orta:mac-titlebar-inline into a1e908b on Microsoft:master.

@coveralls

Coverage Status

Coverage increased (+0.002%) to 61.217% when pulling 764ecbd on orta:mac-titlebar-inline into a1e908b on Microsoft:master.

@orta orta WIP on moving the inline toolbar behind a feature flag
13e6e69
@orta
Contributor
orta commented Sep 26, 2016 edited

When False
screen shot 2016-09-26 at 13 48 26

When true
screen shot 2016-09-26 at 13 48 46

Note: The setup script is async, so the activitybar kinda pop into the correct position, this would not happen once the system defaults to inline

@orta
Contributor
orta commented Sep 26, 2016

I've also signed the CLA just now

@bpasero
Member
bpasero commented Sep 26, 2016

@orta can you share how it would behave with sidebar hidden?

@bpasero bpasero self-assigned this Sep 26, 2016
@orta
Contributor
orta commented Sep 26, 2016

With the sidebar hidden, there shouldn't be any behaviour changes

  • you can still drag it back out of the edge by the activity bar
  • Layout works fine, as it the workspace looks at the width of the elements at runtime

screen shot 2016-09-26 at 14 50 05

@orta
Contributor
orta commented Sep 26, 2016

Hrm, it does look like my method of triggering the re-layout adds the sidebar back when you go to full screen though. Will look at that now.

@orta orta Support using the inline titlebar via an option
2da5767
@bpasero
Member
bpasero commented Sep 26, 2016

@orta right. I see you are pushing down the tabs to make room for moving the window above?

Btw please keep in mind that there is also a mode where tabs are disabled and we in the team are actually mostly running with tabs === off.

@orta orta Relayout by triggering the sidebar to the same setting as before
57ae230
@orta
Contributor
orta commented Sep 26, 2016

Oh, perfect, in that mode the entire thing can be draggable too πŸ‘ -

Assuming you mean workbench.editor.showTabs, I've re-aligned the vertical spacing on text for them and allowed the entire tab to become a drag target, meaning you get the entire top bar as being a drag target

screen shot 2016-09-26 at 15 18 48

@orta
Contributor
orta commented Sep 26, 2016

I'm assuming the CLA bot isn't happy ( I used my personal email address - not my gh one ) - DocuSign Envelope ID: 6F1C856A-21F3-4532-9F03-F7252777488B

I can also put up the PDF too, if needs be

@bpasero
Member
bpasero commented Sep 26, 2016

@orta sucks about the CLA. @chrisdias @seanmcbreen can you help how to get the green badge?

@orta
Contributor
orta commented Sep 26, 2016

No worries, test fails are legit, will take a look

@coveralls

Coverage Status

Coverage remained the same at 61.22% when pulling 9a1eb27 on orta:mac-titlebar-inline into c844c65 on Microsoft:master.

@bpasero bpasero closed this Sep 27, 2016
@bpasero bpasero reopened this Sep 27, 2016
@msftclas

Hi @orta, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@bpasero
Member
bpasero commented Sep 27, 2016

The CLA issue is fixed now πŸ‘

@bpasero bpasero removed the cla-required label Sep 27, 2016
orta added some commits Sep 26, 2016
@orta orta Handle notabs elegantly in inline-toolbars, valign the text and allow…
… dragging on the entire thing
cd023db
@orta orta Merge Master
fc0face
@orta
Contributor
orta commented Sep 27, 2016

Awesome, I've pulled in some breaking changes, the last travis succeeded, but appveyor didn't run, so hopefully this should green

@coveralls

Coverage Status

Coverage remained the same at 61.203% when pulling fc0face on orta:mac-titlebar-inline into 5d057dd on Microsoft:master.

@joaomoreno
Member

omg

@joaomoreno
Member

There are a few jumpy issues at startup: the activity bar starts narrow and grows larger after a second or so.

Else, looks epic and I wanna use it.

@orta
Contributor
orta commented Sep 27, 2016

Yeah, that comes from the cross process communication here

Potentially somewhere in the renderer it could look at window.macOSUseInlineToolbar and set the class synchronously?

@bpasero
Member
bpasero commented Sep 27, 2016

@orta you can read the same settings on the renderer side without the need for IPC communication. The same IConfigurationService is accessible on the renderer side and should enable this nicely πŸ‘

@orta orta Make the renderer set the css style for inline titles when needed
0ccaeb4
@orta
Contributor
orta commented Sep 27, 2016

Alright, had to write some relatively janky code to get that working, but it is - happy for advice for refactoring, but also content to leave - given that it's temporary.

@coveralls

Coverage Status

Coverage remained the same at 61.203% when pulling 0ccaeb4 on orta:mac-titlebar-inline into 5d057dd on Microsoft:master.

@bpasero
Member
bpasero commented Oct 5, 2016

@orta can you bring this PR up to speed with master?

@orta orta Merge branch 'master' of https://github.com/Microsoft/vscode into mac…
…-titlebar-inline
0d36e94
@orta
Contributor
orta commented Oct 5, 2016

Merged master, and given it a run through with window.macOSUseInlineToolbaras both true and false - looks πŸ‘

@coveralls

Coverage Status

Coverage remained the same at 61.238% when pulling 0d36e94 on orta:mac-titlebar-inline into 23517f0 on Microsoft:master.

@coveralls

Coverage Status

Coverage remained the same at 61.238% when pulling 0d36e94 on orta:mac-titlebar-inline into 23517f0 on Microsoft:master.

@bpasero
Member
bpasero commented Oct 5, 2016 edited

@orta thanks, some feedback in no particular order:

  • I am not happy with the vertical space above the tabs, it looks like a UI bug to me especially because the labels inside the tabs are no longer vertically centered

image

  • I am loosing the ability to reorder editors left and right because the editor area is now a drag zone for the window and thus is no longer the drag zone for reordering (try it out with your setting disabled: have 2 editors open side by side and click somewhere to the right of a tab in the empty space and start dragging it to the right)
  • I am always working with zoom level 1 and now the buttons are not centered anymore nicely:

image

Maybe some responsive CSS would help here to make a nice look even when zooming in

  • resizing the output panel now drags the entire window, it looks like the drag zone also applies down there and it should not
  • additional feedback as code review
@bpasero

Initial round of feedback

src/vs/code/electron-main/window.ts
@@ -168,13 +170,17 @@ export class VSCodeWindow {
title: this.envService.product.nameLong,
webPreferences: {
'backgroundThrottling': false // by default if Code is in the background, intervals and timeouts get throttled
- }
+ },
@bpasero
bpasero Oct 5, 2016 Member

extra comma

@@ -134,12 +134,24 @@ export class ElectronIntegration {
ipc.on('vscode:enterFullScreen', (event) => {
this.partService.joinCreation().then(() => {
this.partService.addClass('fullscreen');
+ // We need to re-layout the sidebar as the activitybar's width can change between
+ // fullscreen and window'd modes on macOS.
+ this.partService.setSideBarHidden(this.partService.isSideBarHidden());
@bpasero
bpasero Oct 5, 2016 Member

this is ugly, we should rather call this.partService.layout() and make it work there if it does not

});
});
ipc.on('vscode:leaveFullScreen', (event) => {
this.partService.joinCreation().then(() => {
this.partService.removeClass('fullscreen');
+ this.partService.setSideBarHidden(this.partService.isSideBarHidden());
@bpasero
bpasero Oct 5, 2016 Member

this is ugly, we should rather call this.partService.layout() and make it work there if it does not

+ });
+ });
+
+ ipc.on('vscode:macOSUseInlineToolbar', (event) => {
@bpasero
bpasero Oct 5, 2016 Member

this looks like dead code to me

@@ -74,6 +74,11 @@ import {IEnvironmentService} from 'vs/platform/environment/common/environment';
import {ITelemetryService} from 'vs/platform/telemetry/common/telemetry';
import * as watermark from 'vs/workbench/parts/watermark/watermark';
+// These three are used for the macOS inline toolbars option, and can be removed once default
+import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
+import { ConfigurationService } from 'vs/platform/configuration/node/configurationService';
@bpasero
bpasero Oct 5, 2016 Member

Can you make sure the formatting of imports matches the other imports? also I suggest to remove the comment for now, we will likely not make this default soon nor get rid of the setting if people don't like it. We still have to figure out how to run this experiment, maybe settings is not the best in the end.

+// These three are used for the macOS inline toolbars option, and can be removed once default
+import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
+import { ConfigurationService } from 'vs/platform/configuration/node/configurationService';
+import { IWindowSettings } from 'vs/code/electron-main/window';
@bpasero
bpasero Oct 5, 2016 Member

this is a layer break: we cannot depend from a VSCode window into the main process, please see https://github.com/Microsoft/vscode/wiki/Code-Organization

@@ -98,6 +103,10 @@ const Identifiers = {
STATUSBAR_PART: 'workbench.parts.statusbar'
};
+const ExposedWindowOptions = {
@bpasero
bpasero Oct 5, 2016 Member

Just inline this one and avoid the const

@@ -709,6 +718,17 @@ export class Workbench implements IPartService {
// Create Workbench DIV Off-DOM
this.workbenchContainer = $('.monaco-workbench-container');
this.workbench = $().div({ 'class': 'monaco-workbench ' + (isWindows ? 'windows' : isLinux ? 'linux' : 'mac'), id: Identifiers.WORKBENCH_CONTAINER }).appendTo(this.workbenchContainer);
+
+ // Only needed while the option `windowConfig.macOSUseInlineToolbar` exists
+ if (isMacintosh) {
@bpasero
bpasero Oct 5, 2016 Member

Please move all of this code into renderWorkbench where we already adjust some CSS classes based on things like sidebar visible or not. It is also much easier to get hold of the IConfigurationService by adding the following to the constructor of the workbench:

@IConfigurationService private configurationService: IConfigurationService

All the other code is not needed then.

To find out if the setting is set, I suggest this code:

this.configurationService.lookup('window.macOSUseInlineToolbar').value

@orta
Contributor
orta commented Oct 5, 2016 edited

I am not happy with the vertical space above the tabs, it looks like a UI bug to me especially because the labels inside the tabs are no longer vertically centered

Yeah, that is, looks like different CSS classes are applied if you have different icons sets - I didn't notice this, totally fixable πŸ‘

I am loosing the ability to reorder editors left and right because the editor area is now a drag zone

Yeah, I had wondered about adding a drag indicator to the tab groups to get that back:

screen shot 2016-10-05 at 08 48 06

Which could keep that functionality without losing discoverability.

I am always working with zoom level 1 and now the buttons are not centered anymore nicely:

I've done some preliminary checking, and I can make the activity-bar exempt from zooming, however, that creates a weird gap ( as the sidebar/editor expects width * zoom level as it's left - this is visible in the above screenshot) so I'd need to get access to the window.devicePixelRatio inside layout.ts to undo the implicit scaling. Or use the ZoomManager


WRT to the code stuff πŸ‘ - should be able to get around to this later in week

@bpasero
Member
bpasero commented Oct 5, 2016

@orta if you can share how to exclude a HTML Element from zooming that would be great. We should not zoom the activity bar in any case but I could not find a way to prevent it from happening. If you have a solution let me know.

@orta
Contributor
orta commented Oct 5, 2016

Sure thing: if you apply zoom: reset; to the CSS element, it ignores the zoom πŸ‘

@joaomoreno
Member

Btw, as to the activity bar width: I noticed that Slack beta already has a similar style than what we're trying to achieve here. They succeed in keeping their sidebar slim by snugging the traffic lights a bit more in the corner:

image

We could do the same, pushing it left and up a few more pixels and keeping the width slim.

@orta
Contributor
orta commented Oct 5, 2016

I covered that in here: #12377 (comment)

It's possible, but it's breaking the standard of inline toolbars, and it loses the vertical alignment in exchange for 4px of width

@bpasero
Member
bpasero commented Oct 5, 2016

zoom:reset does not seem to work nicely though:

image

@joaomoreno
Member

I'd hardly call it a standard, given all the different styles we've seen. Slack simply ignores vertical alignment because to them, their app is a 3 vertical strip layout: picker, sidebar, content. We are too (activity bar, side bar, editor), so I think it would work as well.

@orta
Contributor
orta commented Oct 5, 2016

@bpasero I covered that above in #12628 (comment)

I've done some preliminary checking, and I can make the activity-bar exempt from zooming, however, that creates a weird gap ( as the sidebar/editor expects width * zoom level as it's left - this is visible in the above screenshot) so I'd need to get access to the window.devicePixelRatio inside layout.ts to undo the implicit scaling. Or use the ZoomManager

@joaomoreno - I couldn't find a reference in the macOS HIG which is the standard, but the position that you want it to be in is not supposed to be for inline toolbars ( or I guess "full size contentview" windows ) IMO: Slack are doing it wrong. However, I can definitely change it if that's what it takes to get merged πŸ‘

orta added some commits Oct 11, 2016
@orta orta [macInline] Address feedback from #12628
de25adb
@orta orta Merge Master
74fb622
@orta orta Ensure sashes don't trigger app dragging
78f0711
@orta
Contributor
orta commented Oct 11, 2016

I've merged this back up to master, and cleaned up the abstractions ( thanks @bpasero )

I've not made the changes to the traffic lights, @joaomoreno if you're sure on wanting Slack style ones - I can do that.

I've started looking, but not pushed anything about making the sidebar zoom level agnostic, will look at that again soon.

Does anyone have any feedback WRT adding drag handlers for the tab gutter actions?
screen shot 2016-10-11 at 09 38 44

  • Zoom
  • Traffic Lights?
  • Drag handles?
@bpasero
Member
bpasero commented Oct 13, 2016

Btw Atom seems to now ship with a custom title bar and I wonder if we should also look into providing a custom title area color when you run in a dark theme: atom/atom#11790

image

@orta
Contributor
orta commented Oct 14, 2016

I spent some time this evening looking at zooming, and handling a scale independent sidebar - I think it's doable, but I couldn't find a way to do it that doesn't break the layers - will see if I can find some time this weekend to take another look.

It was harder than I expected to actually ensure a consistently placed sidebar leading edge, which felt odd - even when using the electron API directly.

WRT the custom title bar colour, for this version it wouldn't change much, as the titlebar is removed, the colour theme for that space is represented by whatever the sidebar and tab-gutter are:

2016-10-14 23_28_45

@bpasero bpasero Merge branch 'master' into mac-titlebar-inline
dfad1f6
@bpasero

Another set of feedback

src/vs/base/browser/ui/sash/sash.css
@@ -35,6 +35,10 @@
/** Custom Mac Cursor */
+.monaco-workbench.mac.use-inline-toolbar > .monaco-sash.mac {
@bpasero
bpasero Oct 15, 2016 Member

We should not add workbench dependencies here, instead if we want to override something for widgets it should go into workbench.css

@@ -7,13 +7,19 @@
transform: rotate(180deg);
}
-.monaco-workbench > .part.activitybar {
+.monaco-workbench > .part.activitybar, .monaco-workbench.mac.fullscreen.use-inline-toolbar > .part.activitybar {
@bpasero
bpasero Oct 15, 2016 Member

I do not understand why this rule is duplicated here?

@orta
orta Oct 21, 2016 Contributor

This is so that it reverts back to the original behaviour during in full screen πŸ‘ - I should add a comment, I agree it's unintuitive

@orta
orta Oct 21, 2016 Contributor

Though, from retrospect, maybe I can just remove .use-inline-toolbar on switching

+ -webkit-app-region: drag;
+}
+
+.monaco-workbench.mac.use-inline-toolbar > .part.editor > .content > .one-editor-silo > .container > .title .title-label {
@bpasero
bpasero Oct 15, 2016 Member

Why are we adding 1px (from 35px) here?

.monaco-workbench > .part.editor > .content > .one-editor-silo > .container > .title .monaco-icon-label::before {
height: 35px; /* tweak the icon size of the editor labels when icons are enabled */
}
+.monaco-workbench.mac.use-inline-toolbar > .part.editor > .content > .one-editor-silo > .container > .title .monaco-icon-label::before {
@bpasero
bpasero Oct 15, 2016 Member

Why are we adding 1px (from 35px) here?

@@ -11,6 +11,10 @@
display: flex;
}
+.monaco-workbench.mac.use-inline-toolbar > .part > .composite.title {
@bpasero
bpasero Oct 15, 2016 Member

This is bad, it is causing me to drag the window when I drag the title of the output panel for example.

@orta
orta Oct 23, 2016 Contributor

Thanks for this note - this is what was causing me to make changes to src/vs/base/browser/ui/sash/sash.css - fixing this fixes both

@@ -134,12 +134,14 @@ export class ElectronIntegration {
ipc.on('vscode:enterFullScreen', (event) => {
this.partService.joinCreation().then(() => {
this.partService.addClass('fullscreen');
+ this.partService.layout();
@bpasero
bpasero Oct 15, 2016 Member

This should only be called if the setting for inline title is enabled, no?

@orta
orta Oct 21, 2016 Contributor

I thought it wouldn't do any harm on both context's, but I'll wrap it, makes it easier to see what is cause by the inline titlebar too πŸ‘

});
});
ipc.on('vscode:leaveFullScreen', (event) => {
this.partService.joinCreation().then(() => {
this.partService.removeClass('fullscreen');
+ this.partService.layout();
@bpasero
bpasero Oct 15, 2016 Member

This should only be called if the setting for inline title is enabled, no?

+
+ // Mac specific UI changes
+ if (isMacintosh) {
+ const {serviceCollection} = this.workbenchParams;
@bpasero
bpasero Oct 15, 2016 Member

Unused serviceCollection?

+ // Mac specific UI changes
+ if (isMacintosh) {
+ const {serviceCollection} = this.workbenchParams;
+ const windowConfig = this.configurationService.getConfiguration<any>('window');
@bpasero
bpasero Oct 15, 2016 Member

Instead of any, you should add the setting to https://github.com/Microsoft/vscode/blob/mac-titlebar-inline/src/vs/workbench/electron-browser/common.ts#L24 and use IWindowConfiguration properly.

+ if (isMacintosh) {
+ const {serviceCollection} = this.workbenchParams;
+ const windowConfig = this.configurationService.getConfiguration<any>('window');
+ if (windowConfig.macOSUseInlineToolbar) {
@bpasero
bpasero Oct 15, 2016 Member

windowConfig can be null if the user has a weird config

@orta
orta Oct 21, 2016 Contributor

Thanks!

@bpasero
Member
bpasero commented Oct 15, 2016 edited

@orta another round of feedback. I am having the following issues still:

  • zooming is problematic (as discussed)
  • I do not like the padding on top of the tabs. I think it does not work well with our lightweight tabs. if you compare with how tabs look in Chrome I think it is obvious where the tab ends and where the window title starts:

image

Us:

image

Maybe we should just not add any drag affordance for the window on the editor area and keep it to the activity bar and viewlet header. We could even allow it on the status bar too.

orta added some commits Oct 21, 2016
@orta orta [Mac Inline] Address a lot of the config based feedback 3bdacbc
@orta orta Merge branch 'master' of https://github.com/Microsoft/vscode into mac…
…-titlebar-inline
a246915
@orta orta Merge branch 'mac-titlebar-inline' of https://github.com/orta/vscode …
…into mac-titlebar-inline
423ee38
@orta orta [Inline] Add comments to the css style applied during titlebar inline
53b651b
@orta
Contributor
orta commented Oct 22, 2016 edited

Cool, so I've reset the tab changes, they now all should be back to before I added some indentation.

I've "handled" the zoom scaling, but it breaks a bunch of layers, see the commit 70a94b3 - any ideas on how I can make this work correctly with the architecture boundaries?

There is a zoom level inside the window settings, I could add the zoomScale to that maybe?

2016-10-22 22_58_12

The trick was to make the width of the activity bar the window.zoomScale / width - then set the contents of it to have zoom: reset - this means the layout works as expected for the rest of the parts, and lets the icons / hit zones stay correct across zooms

@bpasero
Member
bpasero commented Oct 23, 2016

@orta a more elegant solution would be to set the zoom scale to the browser.ts instance (https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/electron-browser/integration.ts#L170) which we use so that other layers can access this. We already store the zoom level in there so we could also add the scaling.

src/vs/workbench/browser/layout.ts
+ const windowConfig = this.configurationService.getConfiguration<IWindowConfiguration>();
+ if (windowConfig && windowConfig.window.macOSUseInlineToolbar) {
+ const zoom = webFrame.getZoomFactor();
+ activityBarMinWidth = 76/zoom;
@bpasero
bpasero Oct 23, 2016 Member

@orta the 76px should be read from the CSS file if possible and not hardcoded (compare with https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/layout.ts#L307)

@bpasero
Member
bpasero commented Oct 23, 2016

Also please merge with latest on master.

orta added some commits Oct 22, 2016
@orta orta [Inline] Make the activitybar zoom agnostic in style 5f9efca
@orta orta Merge branch 'master' of https://github.com/Microsoft/vscode into mac…
…-titlebar-inline
9fcab0e
@orta
Contributor
orta commented Oct 23, 2016

OK, so I've got that working pretty πŸ‘ in a way which I think works with all the abstractions correctly.
To use the CSS values, I had to cache the original width in order to use it as a reference later. I've added inline comments about why to layout.ts.

I'm seeing a bug with the activitybar icons resizing when I don't have the developer tools open, will look into that when I get back to a computer in a few hours.

I realise my builds are red-ing, I'm getting the same gulp file crashes locally too, not quite sure what's going on there, will also take a look at those.

@bpasero
Member
bpasero commented Oct 23, 2016

@orta looks like you pushed changes that are not formatted (layout.ts). We fail our builds for files that are not formatted.

orta added some commits Oct 23, 2016
@orta orta [Inline] Keep a reference to the original part width from CSS in orde…
…r to multiply that when zooming
1bd2d2f
@orta orta [Inline] CSS drag fixes
de4aeab
@orta orta [Inline] CSS Zooming fixes for activitybar icons
b5718f3
@orta
Contributor
orta commented Oct 23, 2016

Alright, that should be the zooming completely ignoring the activitybar πŸ‘

@bpasero bpasero Merge remote-tracking branch 'upstream/master' into mac-titlebar-inline
0aa9a45
@bpasero
Member
bpasero commented Oct 24, 2016

@orta using Seti Icon theme I can see the icons not aligned good anymore:

image

Also I think the status bar should not be a region that is sensitive to window movement: When you try to drag the hidden panel from above the status bar up, you end up resizing the window:

editors

Finally I am seeing some bad interaction with the fact that editor titles are for window movement and also for editor moving left/right. Trying to capture it:

oct-24-2016 16-12-56

src/vs/code/electron-main/window.ts
@@ -29,6 +29,7 @@ export interface IWindowCreationOptions {
state: IWindowState;
extensionDevelopmentPath?: string;
allowFullscreen?: boolean;
+ macOSUseInlineToolbar: boolean;
@bpasero
bpasero Oct 24, 2016 Member

I am starting to think that we should make this more open ended because I also would like to have the option to have a custom dark title bar as a choice. This should be an enumeration with open ended values (e.g. default, inline, dark) so that we can continue with experiments.

@orta
orta Oct 24, 2016 edited Contributor

Yeah, that makes sense to me - yeah!

Any thoughts on what the name should be then? titlebarStyle?

@bpasero
bpasero Oct 24, 2016 Member

Yes, I guess we need to keep the macOS prefix though.

@@ -136,17 +136,26 @@ export class ElectronIntegration {
ipc.on('vscode:enterFullScreen', (event) => {
this.partService.joinCreation().then(() => {
this.partService.addClass('fullscreen');
+ const windowConfig = this.configurationService.getConfiguration<IWindowConfiguration>();
+ if (windowConfig && windowConfig.window.macOSUseInlineToolbar) {
+ this.partService.layout();
@bpasero
bpasero Oct 24, 2016 Member

This deserves a comment why we call layout under this condition.

});
});
ipc.on('vscode:leaveFullScreen', (event) => {
this.partService.joinCreation().then(() => {
this.partService.removeClass('fullscreen');
+ const windowConfig = this.configurationService.getConfiguration<IWindowConfiguration>();
+ if (windowConfig && windowConfig.window.macOSUseInlineToolbar) {
+ this.partService.layout();
@bpasero
bpasero Oct 24, 2016 Member

This deserves a comment why we call layout under this condition.

src/vs/workbench/browser/layout.ts
@@ -470,7 +492,11 @@ export class WorkbenchLayout implements IVerticalSashLayoutProvider, IHorizontal
}
// Activity Bar Part
- this.activitybar.getContainer().size(null, activityBarSize.height);
+ if (windowConfig && windowConfig.window.macOSUseInlineToolbar) {
@bpasero
bpasero Oct 24, 2016 Member

Instead of duplicating the same code, the activityBarSize should be used in both cases. I know it was not used before but I do not think it would cause any harm.

@@ -15,6 +15,8 @@
.monaco-workbench > .part.editor > .content > .one-editor-silo > .container > .title .tabs-container > .tab .tab-label a {
text-decoration: none;
font-size: 13px;
+ height:35px;
+ line-height: 35px;
@bpasero
bpasero Oct 24, 2016 Member

Are these two still needed?

+.monaco-workbench.mac.use-inline-toolbar > .part.editor > .content > .one-editor-silo > .container > .title .title-label {
+ line-height: 36px;
+}
+
@bpasero
bpasero Oct 24, 2016 Member

Still needed?

+.monaco-workbench.mac.use-inline-toolbar > .part.editor > .content > .one-editor-silo > .container > .title .monaco-icon-label::before {
+ height: 36px;
+}
+
@bpasero
bpasero Oct 24, 2016 Member

Still needed?

@bpasero
Member
bpasero commented Oct 25, 2016

@orta I also like a more narrow size for the bar, see Slack as example (seems to be 65px):

screen shot 2016-10-25 at 10 28 21

orta added some commits Oct 25, 2016
@orta orta [Inline] Use a smaller width activitybar, changes name of the option 23058ac
@orta orta Merge branch 'mac-titlebar-inline' of https://github.com/orta/vscode …
…into mac-titlebar-inline
409df22
@orta orta Merge
4d18e7e
@orta orta Update to handle DOM changes from merge
34c0574
@orta
Contributor
orta commented Oct 25, 2016

Cool cool, updated:

  • switches the option to be a string enum called macOSTitlebarStyle e.g. "window.macOSTitlebarStyle": "inline",
  • Uses the thinner activity bar
  • all the tab height changes can were removed ( not needed for the alignment changes ) - this fixes the seti theme too
  • can't drag from tab wells, not tab container with workbench.editor.showTabs as false
  • removes drag from the status bar

screen shot 2016-10-25 at 12 01 43

@bpasero
Member
bpasero commented Oct 25, 2016

Thanks, we are currently closing for endgame so I would look at this again early next week.

bpasero and others added some commits Nov 5, 2016
@bpasero bpasero Merge remote-tracking branch 'upstream/master' into mac-titlebar-inline
8575acf
@orta orta Merge Master 4f64e2a
@orta orta Support only showing the inline window when the sidebar is on the left
194b39d
@orta
Contributor
orta commented Nov 7, 2016

I've updated this to master, and I've taken into account @dilijev's feedback in #12377 (comment)

The inline title only works if you are in left hand sidebar more

screen shot 2016-11-07 at 10 51 32

screen shot 2016-11-07 at 10 51 45

@bpasero
Member
bpasero commented Nov 7, 2016 edited

@orta I wonder if there is a better solution if the sidebar is moved to the right: just show the buttons to the left of the tab bar. In other words, push the tab bar to the right by 70px but leave everything else as it is. Very much like Chrome:

image

I believe we need this option anyways as soon as we allow to hide the activity bar (which is coming I believe rather soon than late).

The solution would need to work both for having tabs enabled and disabled.

@bpasero
Member
bpasero commented Nov 7, 2016

@orta fyi I merged #15098 to master and plan to adopt the merge conflicts in this PR. My idea is still that your contribution would be another option in the newly introduced window.titleBarStyle setting.

bpasero added some commits Nov 8, 2016
@bpasero bpasero Merge remote-tracking branch 'upstream/master' into mac-titlebar-inline
e639a53
@bpasero bpasero clean up from merging
209cf9b
@bpasero bpasero Merge remote-tracking branch 'upstream/master' into mac-titlebar-inline
980cfc5
@bpasero
Member
bpasero commented Nov 8, 2016 edited

I merged master into this PR and changed the setting to "window.titleBarStyle": "hidden" to play well with the other setting I introduced ("window.titleBarStyle": "custom").

Outstanding issues:

  • handle the case of activity bar right (as mentioned imho it should work in that case too)
  • consider shrinking the activity bar when in full screen mode (I might have broken this)

Will continue to look into the issues asap.

bpasero added some commits Nov 8, 2016
@bpasero bpasero Merge remote-tracking branch 'upstream/master' into mac-titlebar-inline
85ca948
@bpasero bpasero add missing
58168d9
@bpasero bpasero fix compile
722cdef
@bpasero bpasero fix custom title hiding
f8ae059
@orta
Contributor
orta commented Nov 8, 2016

Looks like there's been some issues with the Mac builds today: https://www.traviscistatus.com

bpasero added some commits Nov 9, 2016
@bpasero bpasero Merge remote-tracking branch 'upstream/master' into mac-titlebar-inline
a097106
@bpasero bpasero css polish
04d843c
@bpasero
Member
bpasero commented Nov 9, 2016

@orta let me know how you would like to proceed. I do not want to steal your work and there are some things that still need attention:

  • you cannot use -webkit-app-region: drag; on places where actions are embedded, otherwise you end up moving the window when clicking by accident (e.g. check the debug viewlet). see https://github.com/electron/electron/blob/master/docs/api/frameless-window.md#draggable-region for more info
  • the activity bar should shrink to normal width when in fullscreen mode
  • having the activity bar to the right should work in a way that it adds a margin to the tab title (or tab well) to make room for it

If I do not hear anything from you I will continue to work on these items throughout this milestone.

bpasero added some commits Nov 10, 2016
@bpasero bpasero Merge remote-tracking branch 'upstream/master' into mac-titlebar-inline
80eae42
@bpasero bpasero Merge remote-tracking branch 'upstream/master' into mac-titlebar-inline
6d97755
@bpasero bpasero Merge remote-tracking branch 'upstream/master' into mac-titlebar-inline
9c6cc3d
@bpasero bpasero reduce activity bar size when in fullscreen mode
d67824d
@bpasero bpasero use -webkit-app-region: no-drag; for actions to still execute them wi…
…thout dragging
3fb52f6
@bpasero
Member
bpasero commented Nov 10, 2016 edited

Pushed changes to:

  • shrink the activity bar when entering full screen mode
  • use -webkit-app-region: no-drag; for actions to not drag by accident

Outstanding issues:

  • badges look off
    image
  • what to do when activity bar is hidden
    image
  • what to do when sidebar is hidden or activity bar is on the right hand side
    image
@bpasero bpasero added the help wanted label Nov 10, 2016
@octref
Contributor
octref commented Nov 10, 2016

Maybe hide the traffic light too for people who hide activity bar?

If people are keyboard-driven to the extent of not clicking activity bar icons, I guess they never use traffic light anyway.

Still, right side activity bar is a problem.

@bpasero
Member
bpasero commented Nov 11, 2016

@octref as far as i know you cannot hide the window buttons on Mac.

@octref
Contributor
octref commented Nov 11, 2016 edited

@bpasero You can do that with https://github.com/electron/electron/blob/master/docs/api/frameless-window.md#create-a-frameless-window

By flipping this bit on src/vs/code/electron-main:

image

image

@bpasero
Member
bpasero commented Nov 11, 2016

True, good point πŸ‘

@orta
Contributor
orta commented Nov 11, 2016

Sorry, been getting married, honeymoon’d and then at conferences this last
week!

Really happy to have you wrapping this up, you know so much more than me
about all the different contexts. Especially as the changes are new - hack
away :D

--
[Β·/ ] Orta Therox

w/ Artsy http://artsy.net/
CocoaPods http://cocoapods.org/ / CocoaDocs http://cocoadocs.org/
@orta / orta.github.com
Artsy is totally hiring iOS Devs http://artsy.net/job/developer ATM

On Fri, Nov 11, 2016 at 6:24 AM, Benjamin Pasero notifications@github.com
wrote:

True, good point πŸ‘

β€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12628 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC_jtuak-kp-N9dZGMOTKh370m613Dwks5q8_wBgaJpZM4KGdMs
.

@dilijev
Member
dilijev commented Nov 11, 2016

@octref for what it's worth I'm very keyboard-driven and use hotkeys for nearly everything, but I'd be more likely to toggle visibility of the sidebar with a hotkey because the functionality it exposes is for the most part UI-driven or uncommon enough that it's not worth memorizing hotkeys.

Also maybe I'm weird I still use the title bar to close windows most of the time.

@octref
Contributor
octref commented Nov 11, 2016

@orta Still, thank you a lot for your incredible contribution so far. I'll pick it up and hopefully get it into the next release.

@dilijev Input taken. Thanks.

@octref octref self-assigned this Nov 23, 2016
@octref
Contributor
octref commented Dec 5, 2016

Finally settled down. I'm thinking about bringing this forward during endgame when master isn't moving fast.

@bpasero This time I'd like to get a concrete design before starting the work, so I won't waste time and go the wrong direction.
Do you mind discussing with UX team in this week's meeting and let me know how you would like this to be done?
My current thinking is, we have 3 mode for the titleBarStyle.

  • default: what we currently have.
  • inline: what @orta has here. Except we always show sidebar on left side. Toggle sidebar and changing sidebar to right will be disabled.
  • hidden: hide the titlebar and traffic light altogether. Give a bit space on top of tab, similar to inline. User could still toggle/move sidebar.

In the long term, we can evolve inline to make a custom inline titlebar for each OS, aligning to its design guidelines. For example, @be5invis's proposal for Windows #12377 (comment)

@octref octref removed their assignment Dec 5, 2016
@bpasero
Member
bpasero commented Dec 6, 2016

Please ping me again in 1 week, we are in endgame and I will not have time for this issue during the week πŸ‘

@tal
tal commented Dec 14, 2016

@bpasero I'd actually love to see this myself. so I'll ping on their behalf :P

@bpasero
Member
bpasero commented Dec 15, 2016 edited

@octref maybe better to move the discussion out into a new UX issue so that we can have it separate from the implementation issue. if you can add mockups for your proposals, that would probably be most helpful.

@octref
Contributor
octref commented Dec 19, 2016

Hey @orta can you add me as a collaborator to your fork? Let's bring it on par with master first...

@octref
Contributor
octref commented Dec 19, 2016
@orta
Contributor
orta commented Dec 19, 2016

@octref πŸ‘

@bpasero bpasero removed their assignment Jan 18, 2017
@bpasero
Member
bpasero commented Jan 19, 2017

Closing this PR until there is activity again.

@bpasero bpasero closed this Jan 19, 2017
@bpasero bpasero removed the help wanted label Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment