Do not access/update the storage state directly

[sandy081]

Storage state is changed directly by the layout service while applying the default layout. Eg:

vscode/src/vs/workbench/browser/layout.ts

Lines 636 to 642 in 6d70b4f

	storageService.store(`${viewletId}.state.hidden`, JSON.stringify(viewsState), StorageScope.GLOBAL);
	storageService.store(`${viewletId}.state`, JSON.stringify(viewsWorkspaceState), StorageScope.WORKSPACE);
	}
	}
	if (sidebarState.length) {
	storageService.store(ActivitybarPart.PINNED_VIEWLETS, JSON.stringify(sidebarState), StorageScope.GLOBAL);

I do not think this is the right way to access/update the state that is internal to a specific component/service. This will break if the corresponding component/service changes its semantics. Worst that corresponding component/service owner will not be aware that something is broken. It seems keys are also hardcoded 😞

Right way to do this is to add APIs to corresponding services and let the services change their state.

I would strongly recommend to change this at the earliest.

@bpasero @sbatten FYI

[bpasero]

Agree. Wouldn't a better approach be to propagate this default layout to the related components and let them take care without writing to storage service at all? I feel this should be taken as a default value when no other storage value is present but not go through the storage service at all. In other words, I would not expect any storage service involvement to get this default layout working ideally.

[sandy081]

Yes, that's what I have in my mind too.

Layout service can have an API to expose default layout and corresponding components (ActivityBarPart, ViewContainerModel) can read this and add to their state.

I feel this should be taken as a default value when no other storage value is present but not go through the storage service at all.

💯 It should be similar to our configuration model - Default -> User (Storage).

[bpasero]

@eamodio as discussed here is a list of related debt I would like to report:

• WORKSPACE_FIRST_OPEN should move out of layout.ts into a place that a) works in web and desktop and b) is more central to access (e.g. could be literally a concept of the storage service that people can ask for since the storage service introduces the notion of workspace scope)
• I do not understand the name property in PanelActivityState that is not localized and not used it seems?
• IDefaultSideBarLayout and IDefaultPanelLayout repeat the same types, wouldn't it make sense to extract something like IView for reuse?

[eamodio]

Per conversations with @bpasero & @sandy081 were are simplifying/reducing the features of the default layout api and expanding on the default editors api.

Here is the new api:

interface IDefaultLayout {
	readonly views?: IDefaultView[];
	readonly editors?: IDefaultEditor[];
}

interface IDefaultView {
	readonly id: 'explorer' | 'run' | 'scm' | 'search' | 'extensions' | 'remote' | 'terminal' | 'debug' | 'problems' | 'output' | 'comments' | string;
}

interface IDefaultEditor {
	readonly uri: UriComponents;
	readonly openOnlyIfExists?: boolean;
	readonly viewType?: string;
}

IDefaultLayout.sidebar & IDefaultLayout.panel have been deprecated and replaced by IDefaultLayout.views. IDefaultLayout.views is an array of views that are desired to be active (regardless of current layout location). The first view in the array wins, so if you pass in an array with 2 views, and both views are in the sidebar, then the first view will be the active one.

IDefaultEditor now has a new viewType property which is the type of custom editor that should be opened in place of the default editor (if it exists).

Internally we will be replacing the current tweaking of storage keys from the default layout code, into specific apis exposed by the view service.

[sandy081]

@eamodio What is the id of IDefaultView representing? I see some ids are listed and they seem to be view container ids. Why not id is just a string and callers can just provide any view id?

[eamodio]

Current the old code paths still exist, but once Codespaces migrates to the new api I can remove all of that.

@sandy081

What is the id of IDefaultView representing? I see some ids are listed and they seem to be view container ids. Why not id is just a string and callers can just provide any view id?

I made it an object rather than just the string in case we need to add more information to it after (e.g. extension id -- so that we could possibly improve performance)

[bpasero]

@eamodio I feel I could have contributed to this work or participate in discussion. Can we turn this into a PR and work on it together? I feel this more efficient compared to me opening issues or even making the changes myself on top.

In particular in this case I find it weird that the workbench is responsible for opening views as instructed by the default layout, I feel this code should live inside the view or panel service? Now we seem to be restoring views first, and then viewlets?

[sandy081]

@bpasero Sorry for not updating you.

On Wednesday We (@eamodio , @sbatten & Myself) discussed about implementing default layout for views and we came to following conclusions:

• While applying default views, do not restore layout until default views are available (ie., views are registered) because views might not be available and also to prevent flickering. We also discussed other options like having some intermediate UI while extensions are being registered but not sure if it is worth. We can improve it but it’s a good start.

• If views are available show them, otherwise wait until extensions are registered and fallback to non default approach.

Re: PR - Yeah, agreed that if there is a PR that one of us could give the feedback.

[sandy081]

I feel this code should live inside the view or panel service? Now we seem to be restoring views first, and then viewlets?

Restoring is the state owned by layout and hence I think it makes sense the layout service restore views/viewlets based on its previous/default state. I do not think we need to restore viewlets anymore as restoring/opening views restore/open viewlets.

[sandy081]

I do not think we need to restore viewlets anymore as restoring/opening views restore/open viewlets.

I see why we need to restore viewlets, I take it back. This is needed for restoring previous state.

[bpasero]

Thanks Sandy for clarification, I suggest we can take this offline, will follow up with Eric.

I opened #99972 to remove the restore logic from workbench.ts and move it into layout.ts where imho it belongs.