feat: explore perspective mechanisms - #320
Conversation
| protected applyChrome(descriptor: PerspectiveDescriptor): void { | ||
| const perspectiveHidesMenu = descriptor.chromeOptions?.hideMenuBar ?? false; | ||
| const prefHidesMenu = ['compact', 'hidden'].includes( | ||
| this.corePreferences['window.menuBarVisibility'] | ||
| ); | ||
| this.shell.topPanel.setHidden(perspectiveHidesMenu || prefHidesMenu); | ||
|
|
||
| const perspectiveHidesStatus = descriptor.chromeOptions?.hideStatusBar ?? false; | ||
| const prefHidesStatus = !this.preferenceService.get<boolean>( | ||
| 'workbench.statusBar.visible', true | ||
| ); | ||
| this.statusBar.setHidden(perspectiveHidesStatus || prefHidesStatus); |
There was a problem hiding this comment.
This reimplements chrome handling that already lives in the shell and status bar, and both keep managing the same targets. The menu-bar branch duplicates ApplicationShell.setTopPanelVisibility (
theia/packages/core/src/browser/shell/application-shell.ts
Lines 417 to 419 in a332678
StatusBarImpl's own preference listener (theia/packages/core/src/browser/status-bar/status-bar.tsx
Lines 57 to 61 in a332678
window.menuBarVisibility (theia/packages/core/src/browser/shell/application-shell.ts
Lines 342 to 346 in a332678
topPanel hidden and the result depends on listener order. Better to route through the shell rather than set topPanel/statusBar directly here.
|
|
||
| setMenuBarHiddenByPerspective(hidden: boolean): void { | ||
| this.perspectiveHidesTopPanel = hidden; | ||
| this.setTopPanelVisibility(this.corePreferences['window.menuBarVisibility']); |
There was a problem hiding this comment.
On Electron this likely has no effect: the shell only wires top-panel visibility for non-Electron (init guards these setTopPanelVisibility calls with !environment.electron.is(),
theia/packages/core/src/browser/shell/application-shell.ts
Lines 340 to 346 in a332678
AI First's hideMenuBar won't hide the menu there.
| import { EXPLORER_VIEW_CONTAINER_ID } from '@theia/navigator/lib/browser'; | ||
| import { SCM_VIEW_CONTAINER_ID } from '@theia/scm/lib/browser/scm-contribution'; | ||
|
|
||
| const CHAT_VIEW_WIDGET_ID = 'chat-view-widget'; |
There was a problem hiding this comment.
ai-ide already depends on @theia/ai-chat-ui, so this could import ChatViewWidget.ID (
|
Thanks for the review! I updated the PR to address the comments. I removed menu bar visibility option as it's a bit tedious to consistently support, due to the several different options. Also, we likely want to "filter" menus rather than "hide" them entirely. |
sdirix
left a comment
There was a problem hiding this comment.
One minor comment. COuld be adressed in a follow up
| if (this.switchInProgress) { | ||
| await this.switchInProgress; | ||
| } | ||
| this.switchInProgress = this.doSwitchPerspective(id); |
There was a problem hiding this comment.
The added guard serializes two switches but not three or more: a third concurrent caller resumes from the same await this.switchInProgress and then reassigns switchInProgress to its own doSwitchPerspective, so it runs concurrently with the second one and their savedLayouts writes can still interleave. Chaining instead would fully serialize, e.g. this.switchInProgress = (this.switchInProgress ?? Promise.resolve()).then(() => this.doSwitchPerspective(id)).
|
|
||
| const savedLayout = this.savedLayouts.get(id); | ||
| if (savedLayout) { | ||
| await this.shell.setLayoutData(savedLayout); |
There was a problem hiding this comment.
activePerspectiveId is already advanced at L148, but setLayoutData here (and collapsePanel below) aren't guarded like the widget loop is. If either rejects, the service reports the new perspective as active while its layout was never applied, and since switches are chained the rejection also drops any switch queued behind it. A try/catch around the layout application would keep the state consistent. Fine as a follow-up.
What it does
menu bars, status bar, collapse areas, ...How to test
Follow-ups
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers