Parts splash #53871

jrieken · 2018-07-09T13:07:29Z

With this PR the workbench stores the abstract structure of some workbench parts so that we can show a preview of those very early the next time the editor starts.

bpasero · 2018-07-09T15:18:13Z

@jrieken before going into code review, some general observations:

the titlebar container also needs to be considered, especially now that we support it on Windows and Linux too (possibly with the menu structure so that the menu loads without delay?). On macOS to enable the custom title when running "out of sources" comment this line and this line
I wonder if we should try to get the activity bar icons in as well because that seems like a prominent UI element that is quite stable and it almost feels like a bug that you see the activity bar without these icons in the beginning (same is true for status bar, but I did not see that as being too much of an issue because the status bar is not so much in your face)
currently the data in localStorage is quite verbose, I wonder if we could store just the dynamic pieces and generate the HTML on the consuming side to store less things

Given the first 2 points I wonder if this should get a bit more support from the workbench, e.g. a Part could get asked to return a piece of the splash screen and the workbench simply collects these parts instead of hardcoding them during shutdown phase?

jrieken · 2018-07-10T08:54:02Z

the titlebar container also needs to be considered, especially now that we support

Thanks, that was an oversight. I pushed a fix but it seems that the titlebar changes color on shutdown. When persisting it is rgb(60, 60, 60) and when running it is rgba(60, 60, 60, .6). Not sure why that is. Ideas?

I wonder if we should try to get the activity bar icons in as well because that seems like a prominent UI element that is quite stable and it almost feels like a bug that you see the activity bar without these icons in the beginning

Agreed, but I wanna keep that out of the first stab on this. Ideally, each part knows what of its children can be persisted, like icons of the activity bar, titles of viewlets and panels, and editor tabs. I think that should definitely be explored but bringing in UI elements early also raises the question of how to signal that they cannot be interacted with.

currently the data in localStorage is quite verbose, I wonder if we could store just the dynamic pieces and generate the HTML on the consuming side to store less things

Yeah, it's a tradeoff between storing more and spending precious compute-cycles on startup. I went with the former. It's around 1k which isn't nothing but also just a 1/10 of my color theme data and a 1/50 of my file history.

Given the first 2 points I wonder if this should get a bit more support from the workbench, e.g. a Part could get asked to return a piece of the splash screen and the workbench simply collects these parts instead of hardcoding them during shutdown phase?

Yeah, we should explore but I don't wanna over-engineer this from the start. It's also a tricky question where to put things. Parts only know the inside-world but not how different parts are composted, e.g. the statusbar doesn't know it spreads the whole width of the window but for the parts splash that's important. I think it needs a bit of both, the part should return its inside world with relative sizing and the outside (shell) should persist the overall picture.

bpasero · 2018-07-10T09:33:40Z

Thanks, that was an oversight. I pushed a fix but it seems that the titlebar changes color on shutdown. When persisting it is rgb(60, 60, 60) and when running it is rgba(60, 60, 60, .6). Not sure why that is. Ideas

I think this could be because we change the color of the title area when focus is lost to indicate the window being inactive. Maybe on shutdown this actually gets triggered?

Agreed, but I wanna keep that out of the first stab on this. Ideally, each part knows what of its children can be persisted, like icons of the activity bar, titles of viewlets and panels, and editor tabs. I think that should definitely be explored but bringing in UI elements early also raises the question of how to signal that they cannot be interacted with.

Yeah the fact that you cannot click the view icons makes it ugly to show them. Maybe we could go with an approach where it is clear that things are still loading, similar to what Slack is doing in the channel list:

Yeah, we should explore but I don't wanna over-engineer this from the start. It's also a tricky question where to put things. Parts only know the inside-world but not how different parts are composted, e.g. the statusbar doesn't know it spreads the whole width of the window but for the parts splash that's important. I think it needs a bit of both, the part should return its inside world with relative sizing and the outside (shell) should persist the overall picture.

Yeah once we decide to put more into the part itself we should add it probably.

jrieken · 2018-07-10T10:39:33Z

src/vs/workbench/electron-browser/bootstrap/index.js

+		keep = true;
+	}
+
+	let structure = window.localStorage.getItem(key);


This is now the new first access to local storage. I do recall that we have some special perf-ticks for that but I am unsure if that is still the case and iff so where that is.

bpasero

On Windows I see a scrollbar showing up briefly when I reload the Window with custom title bar.

Settings (Preview) - playground (Workspace) - Code - OSS Dev 10.07.2018 16_16_32.mp4.zip

jrieken · 2018-07-10T15:20:33Z

😱 scrollbar... I wonder how that happens... doesn't look it actually scrolls something. Lets sit on your machine and debug

bpasero · 2018-07-10T15:25:29Z

I am going home now, but in tomorrow. That div must be overflowing somehow (maybe due to the new title div)

bpasero · 2018-07-13T14:40:38Z

@jrieken the overflow reproduces for me also in parallels windows, it seems to:

reproduce independent from the zoom level
only when "window.titleBarStyle": "custom"

Also in the case of "window.titleBarStyle": "custom" the background color seems wrong (but you mentioned this already before I think).

jrieken · 2018-07-18T10:19:49Z

Also in the case of "window.titleBarStyle": "custom" the background color seems wrong (but you mentioned this already before I think).

That was actually just a typo in the template ;-)

jrieken · 2018-07-18T11:56:29Z

the overflow reproduces for me also in parallels windows,

Yeah, the problem was that some parts take height 100% despising being positioned at an top-offset.

jrieken · 2018-07-18T13:45:44Z

cc @isidorn

isidorn · 2018-07-24T08:49:54Z

src/vs/workbench/electron-browser/shell.ts

+	}
+
+	private _removePartsSplash(): void {
+		let element = document.getElementById('monaco-parts-splash');


Can we put this monaco-parts-splash into a variable since it is used twice

isidorn · 2018-07-24T08:50:27Z

src/vs/workbench/electron-browser/shell.ts

+		// activitybar-part
+		let left = this.workbench.getSideBarPosition() === Position.LEFT;
+		let activityPartWidth: number;
+		{


All these parts all already aware of their heigth so I think no height computation is necessery, you can just get their heigth.

Same for width.

If we remove the width and heigth computation maybe we could reuse some of this code to not compute all these 4 times

@isidorn How/where can I access those dimensions?

@jrieken I thought something like

let part = this.workbench.getContainer(Parts.ACTIVITYBAR_PART); part.clientHeight

isidorn · 2018-07-24T08:53:16Z

@jrieken I added some comments directly in the code.
I tried this out on mac and works nicely (even in some corner cases). Now I will try it on Windows.

isidorn · 2018-07-24T09:24:11Z

Tried it out on windows and this works nice.

Some issues I hit (independent of platform):

You keep all these dimensions per Workspace, however most of them we keep globaly and thus a glitch can happen.
Example:

Have a sidebar very small in workspace A
Open workspace B and have a sidebar very large
Open workspace A again, the splash thinks the sidebar is small, even though it is large since we keep it globaly
IMHO: for the most common use case it would work better if we keep the splash globaly. If we take this approach we would take the workspace UX configuraitons as a corner case which I believe they are.

Another issue I hit with the Purple vscode, since we always hide the sidebar when purple vscode is opened it seems, and the splash screen does not respect that but paints the sidebar as it was last time I were purple.

So apart from my suggestion to keep the splash globaly I would also do a special case for purple splash due to the reason above.

jrieken · 2018-07-24T09:35:54Z

You keep all these dimensions per Workspace, however most of them we keep globaly and thus a glitch can happen.

Is there a (nice) way to get that globally stored width?

Another issue I hit with the Purple vscode, since we always hide the sidebar when purple vscode is opened it seems, and the splash screen does not respect that but paints the sidebar as it was last time I were purple.

That's easy to hardcode.

isidorn · 2018-07-24T09:39:59Z

this.storageService.getInteger("workbench.sidebar.width", StorageScope.GLOBAL, 300)

Do you want me to dig out all the values you might need here?

Just wanted to add that I believe that the purple splash is very important since those are the users that open / close vscode all the time for single file editing and for them percieved startup performence is important - will test it out a bit more.

isidorn · 2018-07-24T09:47:06Z

Just tried out purple vscode more and no hardcoding will be needed it seems. Since the most common scenario of just opening files works fine.
What currently fails is the following

Purple vscode, open sidebar
Open folder
Close folder (going back to purple) -> sidebar is hidden but splash shows it

Not sure how to distuingish this case from opening files in purple. So for me the current behavior works for now.

jrieken · 2018-07-24T09:50:04Z

I have now pushed a change that makes sure we ignore the sidebar when the workbench state is the empty one. Doesn't look weirdly hard-coded.

isidorn · 2018-07-24T09:51:44Z

@jrieken checking it out...

isidorn · 2018-07-24T09:54:54Z

@jrieken works fine. Ok I am pretty puzzled when we decide to open the sidebar and when not for the purple vscode...

But overall like the current behavior.
Also to keep in mind is if we are wrong it is better to be wrong and not show it, than be wrong and show it. Since the glitch in the second case is more noticable.

isidorn · 2018-07-24T12:50:10Z

@jrieken I see you pushed should I check it out again?
Though from the last time I tried it it looks good to me for merging to master

jrieken · 2018-07-24T12:55:19Z

Yeah, merging now. The rest are issues ;-)

jens1o · 2018-08-17T10:20:50Z

src/vs/workbench/electron-browser/shell.ts

+		{
+			let part = this.workbench.getContainer(Parts.STATUSBAR_PART);
+			let height = getTotalHeight(part);
+			let bg = part.style.backgroundColor || 'inhert';


jrieken added 4 commits July 6, 2018 15:18

capture & restore dimensions and background colors of our parts

1dadf35

support empty, folder, and workspace case

bedb3b5

special handling for status bar part

af0bb1b

select sidebar, activitypart, and statusbar

fe0ee2b

jrieken added this to the July 2018 milestone Jul 9, 2018

jrieken added the perf-startup label Jul 9, 2018

jrieken self-assigned this Jul 9, 2018

jrieken requested a review from bpasero July 9, 2018 13:10

bpasero self-assigned this Jul 9, 2018

consider title part

2e3fe4b

jrieken commented Jul 10, 2018

View reviewed changes

bpasero approved these changes Jul 10, 2018

View reviewed changes

bpasero removed their assignment Jul 10, 2018

jrieken added 2 commits July 13, 2018 12:47

Merge branch 'master' into joh/splash

a08b9b2

hide overflow

d0ec236

jrieken added 3 commits July 18, 2018 10:14

Merge branch 'master' into joh/splash

c5b2d4c

reliable way of creating workspace key

7881241

fix color for title bar part

8860af0

fix scrollbar issue

8fa044a

jrieken added 2 commits July 18, 2018 16:00

move perf-mark for local storage access to index.js

ad86b59

Merge branch 'master' into joh/splash

71bc904

egamma mentioned this pull request Jul 23, 2018

Iteration Plan for July 2018 #53850

Closed

49 tasks

jrieken added 2 commits July 24, 2018 09:35

Merge branch 'master' into joh/splash

7348d2f

Adopt folderUri changes

5406591

isidorn reviewed Jul 24, 2018

View reviewed changes

more fields

25f44fb

ignore sidebar when workspace is empty

37183ce

replace getDomNodePagePosition with more specific functions

3a7fa5f

jrieken merged commit 73d874f into master Jul 24, 2018

jrieken deleted the joh/splash branch July 24, 2018 12:55

jrieken mentioned this pull request Jul 24, 2018

Test Partssplash #54937

Closed

3 tasks

jens1o reviewed Aug 17, 2018

View reviewed changes

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parts splash #53871

Parts splash #53871

jrieken commented Jul 9, 2018

bpasero commented Jul 9, 2018 •

edited

jrieken commented Jul 10, 2018

bpasero commented Jul 10, 2018 •

edited

jrieken Jul 10, 2018

bpasero left a comment

jrieken commented Jul 10, 2018

bpasero commented Jul 10, 2018 •

edited

bpasero commented Jul 13, 2018

jrieken commented Jul 18, 2018

jrieken commented Jul 18, 2018

jrieken commented Jul 18, 2018

isidorn Jul 24, 2018

jrieken Jul 24, 2018

isidorn Jul 24, 2018

isidorn Jul 24, 2018

jrieken Jul 24, 2018

isidorn Jul 24, 2018

isidorn commented Jul 24, 2018

isidorn commented Jul 24, 2018 •

edited

jrieken commented Jul 24, 2018

isidorn commented Jul 24, 2018

isidorn commented Jul 24, 2018

jrieken commented Jul 24, 2018

isidorn commented Jul 24, 2018

isidorn commented Jul 24, 2018 •

edited

isidorn commented Jul 24, 2018

jrieken commented Jul 24, 2018

jens1o Aug 17, 2018

Parts splash #53871

Parts splash #53871

Conversation

jrieken commented Jul 9, 2018

bpasero commented Jul 9, 2018 • edited

jrieken commented Jul 10, 2018

bpasero commented Jul 10, 2018 • edited

Choose a reason for hiding this comment

bpasero left a comment

Choose a reason for hiding this comment

jrieken commented Jul 10, 2018

bpasero commented Jul 10, 2018 • edited

bpasero commented Jul 13, 2018

jrieken commented Jul 18, 2018

jrieken commented Jul 18, 2018

jrieken commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidorn commented Jul 24, 2018

isidorn commented Jul 24, 2018 • edited

jrieken commented Jul 24, 2018

isidorn commented Jul 24, 2018

isidorn commented Jul 24, 2018

jrieken commented Jul 24, 2018

isidorn commented Jul 24, 2018

isidorn commented Jul 24, 2018 • edited

isidorn commented Jul 24, 2018

jrieken commented Jul 24, 2018

Choose a reason for hiding this comment

bpasero commented Jul 9, 2018 •

edited

bpasero commented Jul 10, 2018 •

edited

bpasero commented Jul 10, 2018 •

edited

isidorn commented Jul 24, 2018 •

edited

isidorn commented Jul 24, 2018 •

edited