-
-
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -417,12 +417,25 @@ export function SettingsDialog({ | |
| const showSettingsItem = (id: string) => isMenuItemVisible(desktopSettings.uiProfile, id); | ||
| const [open, setOpen] = useState(false); | ||
| const [section, setSection] = useState<SettingsSection>("map"); | ||
| // The Browser is a dockable right panel (open/close via the registry), not a | ||
| // persisted layout preference, so its Layout toggle acts on the live registry | ||
| // state directly rather than through the draft settings. | ||
| // Browser and Comments are dockable right panels, so their checkboxes read the | ||
| // live registry state (the user can also close them from their own header) | ||
| // while the toggle writes the matching persisted layout setting. The setting | ||
| // is what their registration hooks seed from on the next launch, so the | ||
| // toggle survives a restart (#1935). | ||
| const rightPanelState = useRightPanelState(); | ||
| const browserPanelOpen = rightPanelState.visibleIds.includes(BROWSER_PANEL_ID); | ||
| const commentsPanelOpen = rightPanelState.visibleIds.includes(COMMENTS_PANEL_ID); | ||
| // These apply live rather than on Save, so the draft is patched alongside the | ||
| // saved settings: the draft was snapshotted when the dialog opened, and Save | ||
| // writes it wholesale, which would otherwise revert the toggle the user just | ||
| // made in this same dialog. | ||
| const applyPanelVisibility = ( | ||
| key: "browserPanelVisible" | "commentsPanelVisible", | ||
| visible: boolean, | ||
| ) => { | ||
| updateSavedLayoutSettings({ [key]: visible }); | ||
| updateDraftLayoutSettings({ [key]: visible }); | ||
| }; | ||
| // Show it collapsed on the shared Layers rail, matching its default state, so | ||
| // re-enabling from Settings doesn't jump to an expanded panel that buries the | ||
| // Layers panel. | ||
|
|
@@ -433,6 +446,7 @@ export function SettingsDialog({ | |
| } else { | ||
| closeRightPanel(BROWSER_PANEL_ID); | ||
| } | ||
| applyPanelVisibility("browserPanelVisible", show); | ||
| }; | ||
| // Collapsed for the same reason as Browser above, and to match the state | ||
| // Comments registers itself in on mount. | ||
|
|
@@ -443,6 +457,7 @@ export function SettingsDialog({ | |
| } else { | ||
| closeRightPanel(COMMENTS_PANEL_ID); | ||
| } | ||
| applyPanelVisibility("commentsPanelVisible", show); | ||
| }; | ||
| // A field a deep-link asked us to focus once its section renders; cleared | ||
| // after the focus lands so a later open without a focus request stays put. | ||
|
|
@@ -932,6 +947,11 @@ export function SettingsDialog({ | |
|
|
||
| const resetLayoutSettings = () => { | ||
| updateDraftLayoutSettings(DEFAULT_DESKTOP_LAYOUT_SETTINGS); | ||
| // The Browser/Comments checkboxes render the live registry state, not the | ||
| // draft, so reset has to move the panels themselves or those two rows would | ||
| // ignore the button. | ||
| toggleBrowserPanel(DEFAULT_DESKTOP_LAYOUT_SETTINGS.browserPanelVisible); | ||
| toggleCommentsPanel(DEFAULT_DESKTOP_LAYOUT_SETTINGS.commentsPanelVisible); | ||
| }; | ||
|
Comment on lines
917
to
919
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That means clicking Reset and then Cancel produces a partial reset: the two panel-visibility settings are permanently changed (survive the dialog close), but Confidence: medium — this follows directly from the code, but it may be an accepted trade-off of the "panels apply live" design described in the PR. |
||
|
|
||
| // The accent scheme applies live (instant preview) rather than waiting for | ||
|
|
||
| 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); | ||
| }); | ||
| }); |
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.
The Browser/Comments checkboxes are bound to
browserPanelOpen/commentsPanelOpen, which read the live right-panel registry (rightPanelState.visibleIds), not the draft settings. That registry can also change from outside this dialog — e.g. the user closes the panel from its own header, which is explicitly designed to be session-only and not written back tolayout.browserPanelVisible/commentsPanelVisible(per the PR description).Net effect: if a user closes a panel from its header, then opens Settings → Layout, the checkbox shows unchecked (matching the live state) even though the persisted setting is still
true. If they then click Save Settings without touching that checkbox, the save writes the untouched draft value (true), so the panel reopens on the next launch — silently contradicting what the checkbox displayed in the dialog they just saved from.This looks intentional per the PR's stated design, but it's a real display/persistence divergence worth confirming is the desired UX. Confidence: low-medium.