From 6593c08ca7460d3edfaeedfc127cbb6005073577 Mon Sep 17 00:00:00 2001 From: giswqs Date: Tue, 18 Aug 2026 18:33:44 -0400 Subject: [PATCH] fix(desktop): scrub Print Layout blocks when their layer is deleted Follow-up to #1994, which landed before these two review findings could be folded into it. - Scrub the Print Layout data/atlas blocks in removeLayer and deleteLayerGroup, alongside the storymap/widget/comment/legend scrubbing already there. Deleting a layer a block referenced, then saving without reopening the composer, wrote a dangling layer id into the project file; it only self-healed on the next load. Adds scrubPrintLayoutForRemovedLayers, the delete-time counterpart of the load-time scrub, matching scrubLegendForRemovedLayers' shape. - Gate the "fixed scale needs a physical page" correction on `open`, the last effect still writing a persisted field ungated. It also runs on the mount that every project load triggers, so a hand-edited file pairing a pixel page with scale mode would be corrected, and the project marked dirty, before the composer had ever been opened. Refs https://github.com/opengeos/GeoLibre/discussions/1992 --- .../components/layout/PrintLayoutDialog.tsx | 8 ++- packages/core/src/print-layout-config.ts | 57 ++++++++++++++----- packages/core/src/store.ts | 7 +++ tests/core-project.test.ts | 25 ++++++++ tests/print-layout-config.test.ts | 34 +++++++++++ 5 files changed, 116 insertions(+), 15 deletions(-) diff --git a/apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx b/apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx index 5a26e57221..e3ba4b84a4 100644 --- a/apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx +++ b/apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx @@ -1678,9 +1678,15 @@ export function PrintLayoutDialog({ // Fixed scale is only meaningful on physical paper (like the manual scale // input); fall back to margin mode when the page switches to pixel sizes. + // `open`-gated for the same reason as the defaulting effects above: this also + // runs on the mount that every project load triggers, so a hand-edited file + // pairing a pixel page with scale mode would be corrected — and the project + // marked dirty — before the composer had ever been opened. Nothing acts on + // the pairing until the composer is open, and opening it runs this. useEffect(() => { + if (!open) return; if (!isMmPage && atlasExtentMode === "scale") setAtlasExtentMode("margin"); - }, [isMmPage, atlasExtentMode]); + }, [open, isMmPage, atlasExtentMode]); // Two-way scale sync: reflect the captured view's scale into the input unless // the user is actively editing it. diff --git a/packages/core/src/print-layout-config.ts b/packages/core/src/print-layout-config.ts index 18eae1d131..afd497eedf 100644 --- a/packages/core/src/print-layout-config.ts +++ b/packages/core/src/print-layout-config.ts @@ -21,6 +21,8 @@ * a corner on one side without the other fails the build. */ +import { removedLayerIdSet } from "./layer-ref-scrub"; + /** A page corner an overlay block can be pinned to. */ export type PrintLayoutCorner = "top-left" | "top-right" | "bottom-left" | "bottom-right"; @@ -489,6 +491,28 @@ export function printLayoutConfigsEqual(a: PrintLayoutConfig, b: PrintLayoutConf return true; } +/** + * Clear each data/atlas block whose layer `missing` rejects. Shared by the two + * entry points below, which differ only in how they decide what is gone. + * + * @returns The same object when every block's layer survives. + */ +function clearBlocksForMissingLayers( + config: PrintLayoutConfig, + missing: (layerId: string) => boolean, +): PrintLayoutConfig { + const gone = (id: string) => id !== "" && missing(id); + if (!gone(config.tableLayerId) && !gone(config.chartLayerId) && !gone(config.atlasLayerId)) { + return config; + } + return { + ...config, + ...(gone(config.tableLayerId) ? { tableLayerId: "", showDataTable: false } : {}), + ...(gone(config.chartLayerId) ? { chartLayerId: "", showDataChart: false } : {}), + ...(gone(config.atlasLayerId) ? { atlasLayerId: "", atlasEnabled: false } : {}), + }; +} + /** * Drop references to layers the project no longer carries, so a composer that * pointed at a since-deleted layer opens with the block cleared instead of @@ -503,18 +527,23 @@ export function scrubPrintLayoutForLayers( config: PrintLayoutConfig, existingLayerIds: ReadonlySet, ): PrintLayoutConfig { - const missing = (id: string) => id !== "" && !existingLayerIds.has(id); - if ( - !missing(config.tableLayerId) && - !missing(config.chartLayerId) && - !missing(config.atlasLayerId) - ) { - return config; - } - return { - ...config, - ...(missing(config.tableLayerId) ? { tableLayerId: "", showDataTable: false } : {}), - ...(missing(config.chartLayerId) ? { chartLayerId: "", showDataChart: false } : {}), - ...(missing(config.atlasLayerId) ? { atlasLayerId: "", atlasEnabled: false } : {}), - }; + return clearBlocksForMissingLayers(config, (id) => !existingLayerIds.has(id)); +} + +/** + * The delete-time counterpart, matching `scrubLegendForRemovedLayers` and its + * siblings: called when layers are removed from the open project so the saved + * file never carries a block pointing at a layer that is already gone. + * + * @param config - The config to scrub. + * @param layerIds - The removed layer id, or ids. + * @returns The same object when no block referenced a removed layer. + */ +export function scrubPrintLayoutForRemovedLayers( + config: PrintLayoutConfig, + layerIds: string | Iterable, +): PrintLayoutConfig { + const removed = removedLayerIdSet(layerIds); + if (removed.size === 0) return config; + return clearBlocksForMissingLayers(config, (id) => removed.has(id)); } diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 731056a027..c623848bde 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -20,6 +20,7 @@ import { initialLayerStyle } from "./layer-defaults"; import { createDefaultPrintLayout, printLayoutConfigsEqual, + scrubPrintLayoutForRemovedLayers, type PrintLayoutConfig, } from "./print-layout-config"; import { @@ -1700,6 +1701,9 @@ export const useAppStore = create()( widgets: scrubWidgetsForRemovedLayers(s.widgets, id), comments: scrubCommentsForRemovedLayers(s.comments, id), legend: scrubLegendForRemovedLayers(s.legend, id), + // Clear a Print Layout data/atlas block built on the removed layer, + // so a save that follows the delete cannot write a dangling id. + printLayout: scrubPrintLayoutForRemovedLayers(s.printLayout, id), selectedLayerId: s.selectedLayerId === id ? (s.layers.find((l) => l.id !== id)?.id ?? null) @@ -2045,6 +2049,9 @@ export const useAppStore = create()( ? scrubCommentsForRemovedLayers(s.comments, removedIds) : s.comments, legend: removeChildren ? scrubLegendForRemovedLayers(s.legend, removedIds) : s.legend, + printLayout: removeChildren + ? scrubPrintLayoutForRemovedLayers(s.printLayout, removedIds) + : s.printLayout, selectedLayerId: selectionRemoved ? (layers[layers.length - 1]?.id ?? null) : s.selectedLayerId, diff --git a/tests/core-project.test.ts b/tests/core-project.test.ts index e698f9a074..f35672cf91 100644 --- a/tests/core-project.test.ts +++ b/tests/core-project.test.ts @@ -1556,6 +1556,31 @@ describe("print layout persistence", () => { assert.equal(applied.printLayout.chartLayerId, "kept"); }); + it("clears a composer block when its layer is deleted from the open project", () => { + const store = useAppStore.getState(); + const kept = store.addGeoJsonLayer("Kept", { type: "FeatureCollection", features: [] }); + const doomed = useAppStore + .getState() + .addGeoJsonLayer("Doomed", { type: "FeatureCollection", features: [] }); + useAppStore.getState().setPrintLayout({ + ...createDefaultPrintLayout(), + showDataTable: true, + tableLayerId: doomed, + showDataChart: true, + chartLayerId: kept, + }); + + useAppStore.getState().removeLayer(doomed); + + // Otherwise a save taken before the composer is next opened would write a + // block pointing at a layer the file no longer carries. + const after = useAppStore.getState().printLayout; + assert.equal(after.tableLayerId, ""); + assert.equal(after.showDataTable, false); + assert.equal(after.chartLayerId, kept); + assert.equal(after.showDataChart, true); + }); + it("ignores a write that changes nothing, so opening the composer is not an edit", () => { assert.equal(useAppStore.getState().isDirty, false); // The dialog replays its seeded values into the store on mount. diff --git a/tests/print-layout-config.test.ts b/tests/print-layout-config.test.ts index 3da22335e1..575f948c1d 100644 --- a/tests/print-layout-config.test.ts +++ b/tests/print-layout-config.test.ts @@ -7,6 +7,7 @@ import { normalizePrintLayoutConfig, printLayoutConfigsEqual, scrubPrintLayoutForLayers, + scrubPrintLayoutForRemovedLayers, type PrintLayoutConfig, } from "../packages/core/src/print-layout-config"; @@ -267,3 +268,36 @@ describe("scrubPrintLayoutForLayers", () => { assert.equal(scrubPrintLayoutForLayers(config, new Set()), config); }); }); + +describe("scrubPrintLayoutForRemovedLayers", () => { + it("clears the blocks built on a removed layer and leaves the rest alone", () => { + const config = withOverrides({ + showDataTable: true, + tableLayerId: "gone", + showDataChart: true, + chartLayerId: "kept", + atlasEnabled: true, + atlasLayerId: "gone", + }); + const scrubbed = scrubPrintLayoutForRemovedLayers(config, "gone"); + assert.equal(scrubbed.tableLayerId, ""); + assert.equal(scrubbed.showDataTable, false); + assert.equal(scrubbed.atlasLayerId, ""); + assert.equal(scrubbed.atlasEnabled, false); + assert.equal(scrubbed.chartLayerId, "kept"); + assert.equal(scrubbed.showDataChart, true); + }); + + it("accepts a set of ids, as a group delete passes", () => { + const config = withOverrides({ showDataChart: true, chartLayerId: "b" }); + const scrubbed = scrubPrintLayoutForRemovedLayers(config, new Set(["a", "b"])); + assert.equal(scrubbed.chartLayerId, ""); + assert.equal(scrubbed.showDataChart, false); + }); + + it("returns the same object when no block named a removed layer", () => { + const config = withOverrides({ showDataTable: true, tableLayerId: "kept" }); + assert.equal(scrubPrintLayoutForRemovedLayers(config, "gone"), config); + assert.equal(scrubPrintLayoutForRemovedLayers(config, []), config); + }); +});