-
-
Notifications
You must be signed in to change notification settings - Fork 675
fix(layout): persist Browser and Comments panel toggles #1940
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| // The registry subpath rather than the package barrel: this module is a leaf the | ||
| // test suite imports directly, and the barrel pulls in the whole built-in plugin | ||
| // registry (including CSS imports Node cannot load). See CLAUDE.md on testing | ||
| // against a leaf module. | ||
| import { | ||
| collapseRightPanel, | ||
| closeRightPanel, | ||
| getRightPanel, | ||
| isRightPanelVisible, | ||
| openRightPanel, | ||
| registerRightPanel, | ||
| subscribeRightPanels, | ||
| } from "@geolibre/plugins/right-panel-registry"; | ||
| import type { GeoLibreRightPanelRegistration } from "@geolibre/plugins"; | ||
| import { useDesktopSettingsStore, type DesktopLayoutSettings } from "../hooks/useDesktopSettings"; | ||
|
|
||
| /** | ||
| * Layout settings that persist a dockable right panel's visibility. The Browser | ||
| * and Comments panels are the two built-in panels that work this way; plugin | ||
| * panels are owned by their plugin and are not persisted here. | ||
| */ | ||
| export type PersistedPanelKey = Extract< | ||
| keyof DesktopLayoutSettings, | ||
| "browserPanelVisible" | "commentsPanelVisible" | ||
| >; | ||
|
|
||
| /** Read a panel's persisted visibility, bypassing React so callers can seed. */ | ||
| export function isPanelVisibleInSettings(key: PersistedPanelKey): boolean { | ||
| return useDesktopSettingsStore.getState().desktopSettings.layout[key]; | ||
| } | ||
|
|
||
| /** Persist a panel's visibility. A no-op when the value already matches. */ | ||
| export function setPanelVisibleInSettings(key: PersistedPanelKey, visible: boolean): void { | ||
| const { desktopSettings, setDesktopSettings } = useDesktopSettingsStore.getState(); | ||
| if (desktopSettings.layout[key] === visible) return; | ||
| setDesktopSettings({ | ||
| ...desktopSettings, | ||
| layout: { ...desktopSettings.layout, [key]: visible }, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Move a panel to `visible`, showing it collapsed on its rail so re-enabling it | ||
| * does not jump to an expanded panel that buries its neighbour. Bails out when | ||
| * the panel is already where it is being asked to go, so a caller re-applying an | ||
| * unchanged value cannot collapse a panel the user had expanded. | ||
| */ | ||
| export function applyRightPanelVisibility(panelId: string, visible: boolean): void { | ||
| if (isRightPanelVisible(panelId) === visible) return; | ||
| if (visible) { | ||
| openRightPanel(panelId); | ||
| collapseRightPanel(panelId); | ||
| } else { | ||
| closeRightPanel(panelId); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Register a dockable right panel whose visibility is a persisted layout | ||
| * setting, and keep the two in step. Returns a disposer that unsubscribes | ||
| * before unregistering. | ||
| * | ||
| * Visibility is seeded from the setting at registration, so a panel the user | ||
| * turned off stays off across restarts instead of reopening on every launch | ||
| * (GeoLibre#1935). From then on the setting mirrors the registry, which matters | ||
| * because the panel can be closed from its own header as well as from Settings | ||
| * → Layout: for these panels closing is not a transient collapse, it removes | ||
| * the rail entry entirely and only Settings can bring it back, so it is a | ||
| * preference either way. Mirroring is what keeps the Settings checkbox, the | ||
| * panel on screen, and the stored value from ever disagreeing. | ||
| * | ||
| * Two registry events are deliberately *not* mirrored: | ||
| * | ||
| * - The `emit` from registration itself, because the panel is legitimately not | ||
| * visible yet. The subscription is therefore attached after the seed. | ||
| * - The `emit` from unregistering on unmount, which would otherwise persist | ||
| * `false` every time the shell tears down. `unregisterRightPanel` removes the | ||
| * panel from the registry before it emits, so the `getRightPanel` guard | ||
| * catches it; the disposer unsubscribing first makes that belt-and-braces. | ||
| * | ||
| * Being displaced by another panel is not a close (the registry keeps a | ||
| * displaced panel in `visibleIds`), so it correctly writes nothing. | ||
| */ | ||
| export function registerPersistedRightPanel( | ||
| registration: GeoLibreRightPanelRegistration, | ||
| key: PersistedPanelKey, | ||
| ): () => void { | ||
| const dispose = registerRightPanel(registration); | ||
| applyRightPanelVisibility(registration.id, isPanelVisibleInSettings(key)); | ||
| const unsubscribe = subscribeRightPanels(() => { | ||
| if (!getRightPanel(registration.id)) return; | ||
| setPanelVisibleInSettings(key, isRightPanelVisible(registration.id)); | ||
| }); | ||
| return () => { | ||
| unsubscribe(); | ||
| dispose(); | ||
| }; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import assert from "node:assert/strict"; | ||
| import { describe, it } from "node:test"; | ||
| import { | ||
| DEFAULT_DESKTOP_LAYOUT_SETTINGS, | ||
| normalizeDesktopSettings, | ||
| } from "../apps/geolibre-desktop/src/hooks/useDesktopSettings"; | ||
|
|
||
| // The Browser and Comments right panels used to be session-only: their Settings | ||
| // → Layout toggles moved the panel registry but nothing was persisted, so every | ||
| // launch reopened them (GeoLibre#1935). They are now layout settings like the | ||
| // Layers/Style panels, which means they have to round-trip through | ||
| // normalizeDesktopSettings and keep defaulting to on for existing users whose | ||
| // stored settings predate the keys. | ||
| describe("dockable panel layout settings", () => { | ||
| it("defaults both dockable panels to visible", () => { | ||
| assert.equal(DEFAULT_DESKTOP_LAYOUT_SETTINGS.browserPanelVisible, true); | ||
| assert.equal(DEFAULT_DESKTOP_LAYOUT_SETTINGS.commentsPanelVisible, true); | ||
| }); | ||
|
|
||
| it("keeps a disabled panel disabled across a load", () => { | ||
| const layout = normalizeDesktopSettings({ | ||
| layout: { browserPanelVisible: false, commentsPanelVisible: false }, | ||
| }).layout; | ||
| assert.equal(layout.browserPanelVisible, false); | ||
| assert.equal(layout.commentsPanelVisible, false); | ||
| }); | ||
|
|
||
| it("falls back to the defaults for settings saved before the keys existed", () => { | ||
| const layout = normalizeDesktopSettings({ | ||
| layout: { layerPanelVisible: false, stylePanelVisible: true, toolbarLabels: true }, | ||
| }).layout; | ||
| assert.equal(layout.layerPanelVisible, false); | ||
| assert.equal(layout.browserPanelVisible, true); | ||
| assert.equal(layout.commentsPanelVisible, true); | ||
| }); | ||
|
|
||
| it("rejects non-boolean values from tampered storage", () => { | ||
| const layout = normalizeDesktopSettings({ | ||
| layout: { browserPanelVisible: "no", commentsPanelVisible: 0 }, | ||
| }).layout; | ||
| assert.equal(layout.browserPanelVisible, true); | ||
| assert.equal(layout.commentsPanelVisible, true); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor/low confidence:
subscribeRightPanelsfires on every registry mutation (any panel opening/closing/moving dock, not just this one), so this callback re-reads and attempts to persist this panel's own visibility on each such event. It's harmless today becausesetPanelVisibleInSettingsbails when the value already matches, but it does mean unrelated churn in the panel registry (e.g. a plugin panel changing dock) triggers redundantgetRightPanel/isRightPanelVisiblelookups and a store-equality check for both Browser and Comments each time. Not worth blocking on, just flagging as a spot to scope the subscription if this ever becomes hot (e.g. frequent dock dragging).