Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions apps/geolibre-desktop/src/components/layout/DesktopShell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,13 @@
);
}, [t, mapReadyGeneration]);

// Keep the Layer Swipe panel's grouped base-layer label translated. That
// panel lives outside React and reads labels from the controller bridge, so
// re-push on language change (t identity) and controller (re)init.
useEffect(() => {
mapControllerRef.current?.setBackgroundLabel(t("layers.background"));
}, [t, mapReadyGeneration]);

const handleMapDiagnosticEvent = useCallback((event: MapDiagnosticEvent) => {
appendDiagnostic({
category: "map",
Expand Down Expand Up @@ -1153,7 +1160,7 @@
disposed = true;
unlisten?.();
};
}, [clearDropMessageLater,

Check warning on line 1163 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test

React Hook useEffect has a missing dependency: 't'. Either include it or remove the dependency array
finishDrop,
addDroppedRasters,
addDroppedPhotos,
Expand Down Expand Up @@ -1293,7 +1300,7 @@
clearDropMessageLater();
}
},
[clearDropMessageLater,

Check warning on line 1303 in apps/geolibre-desktop/src/components/layout/DesktopShell.tsx

View workflow job for this annotation

GitHub Actions / Build and test

React Hook useCallback has a missing dependency: 't'. Either include it or remove the dependency array
finishDrop,
addDroppedRasters,
addDroppedPhotos,
Expand Down
15 changes: 6 additions & 9 deletions apps/geolibre-desktop/src/lib/swipe-style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,15 +259,14 @@ const enhanceSwipeSelects = () => {

let swipeEnhanceFrame: number | null = null;

// Matches the label used by the main layer manager for the grouped base layer.
// English fallback for the grouped base layer, used until the controller
// publishes the translated label through the layer-label bridge.
const SWIPE_BASEMAP_LABEL = "Background";

const getSwipeLayerLabel = (layerId: string): string => {
if (layerId === "__basemap__") return SWIPE_BASEMAP_LABEL;
return (
(window as GeoLibreLayerLabelWindow).__GEOLIBRE_LAYER_LABELS__?.[layerId] ??
layerId
);
const labels = (window as GeoLibreLayerLabelWindow).__GEOLIBRE_LAYER_LABELS__;
if (layerId === "__basemap__") return labels?.[layerId] ?? SWIPE_BASEMAP_LABEL;
return labels?.[layerId] ?? layerId;
};

const syncSwipeLayerLabels = () => {
Expand All @@ -286,9 +285,7 @@ const syncSwipeLayerLabels = () => {

const displayName = getSwipeLayerLabel(layerId);
const title =
layerId === "__basemap__"
? SWIPE_BASEMAP_LABEL
: `${displayName} (${layerId})`;
layerId === "__basemap__" ? displayName : `${displayName} (${layerId})`;
if (label.textContent !== displayName) {
label.textContent = displayName;
}
Expand Down
39 changes: 34 additions & 5 deletions packages/map/src/map-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ export class MapController {
private fullscreenControl: maplibregl.FullscreenControl | null = null;
private compassControl: ResetBearingControl | null = null;
private compassLabel = "Reset pitch & bearing";
private backgroundLabel = "Background";
private geolocateControl: maplibregl.GeolocateControl | null = null;
private globeControl: maplibregl.GlobeControl | null = null;
private terrainControl: maplibregl.TerrainControl | null = null;
Expand Down Expand Up @@ -574,7 +575,7 @@ export class MapController {
this.map?.remove();
this.map = null;
this.styleReady = false;
this.publishLayerDisplayNames([]);
this.clearLayerDisplayNames();
}

setStyle(url: string): void {
Expand Down Expand Up @@ -1577,11 +1578,28 @@ export class MapController {
if (typeof window === "undefined") return;

const labelWindow = window as GeoLibreLayerLabelWindow;
labelWindow.__GEOLIBRE_LAYER_LABELS__ = Object.fromEntries(
layers
labelWindow.__GEOLIBRE_LAYER_LABELS__ = Object.fromEntries([
...layers
.flatMap((layer) => this.getNamedStyleLayers(layer))
.map(({ id, name }) => [id, name]),
);
.map(({ id, name }): [string, string] => [id, name]),
// The Layer Swipe panel groups all basemap layers under "__basemap__";
// publish the translated base-layer label last so this synthetic key
// always wins over a layer that happens to share the id, matching the
// sidebar. It is published even with no overlay layers, since the panel
// always lists the basemap entry.
["__basemap__", this.backgroundLabel],
Comment thread
giswqs marked this conversation as resolved.
]);
Comment thread
giswqs marked this conversation as resolved.
window.dispatchEvent(new CustomEvent("geolibre-layer-labels-change"));
}

/**
* Clear all published layer display names. Used on teardown so the bridge
* does not retain stale labels; kept separate from publishLayerDisplayNames,
* which always re-publishes the basemap entry.
*/
private clearLayerDisplayNames(): void {
if (typeof window === "undefined") return;
(window as GeoLibreLayerLabelWindow).__GEOLIBRE_LAYER_LABELS__ = {};
window.dispatchEvent(new CustomEvent("geolibre-layer-labels-change"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dispatching geolibre-layer-labels-change on teardown is deliberate (keeps the bridge consistent with the swipe panel if it is still visible), but it has a mild side-effect: the swipe panel's scheduleEnhanceSwipePanel listener fires a requestAnimationFrame callback after the controller is destroyed. The RAF runs harmlessly—document.querySelectorAll returns an empty NodeList when the panel is gone—but if the panel is still mounted at teardown time, it will briefly flash the fallback "Background" label before the new controller's first publish restores the translated value.

This matters only in the reinitialisation path (destroy → new controller created → mapReadyGeneration increments → setBackgroundLabel fires again). The RAF coalescing in scheduleEnhanceSwipePanel (if (swipeEnhanceFrame !== null) return) means the second publish's event typically wins if the two events land in the same frame. In practice the gap is sub-frame, so no visible flicker is expected. Just something to be aware of if a future test asserts on intermediate label state.

No code change needed—documenting the trade-off.

}

Expand Down Expand Up @@ -1663,6 +1681,17 @@ export class MapController {
this.compassControl?.setLabel(label);
}

/**
* Update the label used for the grouped base layer (e.g. after a UI language
* change). It is published through the layer-display-name bridge so the
* Layer Swipe panel, which lives outside React, shows the same translated
* base-layer label as the main layer manager.
*/
setBackgroundLabel(label: string): void {
this.backgroundLabel = label;
this.publishLayerDisplayNames(this.syncedLayers);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One heads-up: setBackgroundLabel always calls publishLayerDisplayNames(this.syncedLayers), which iterates every synced layer and calls getNamedStyleLayers per layer. This is the same work that syncLayers does on every render, so it is not a new hot path—but it means a single label-push is O(n) in the layer count. If setBackgroundLabel were ever called in a tight loop (it currently is not—only on language-change or controller re-init), this could be noticeable with a large layer list. Not an issue today; just worth knowing if the call site ever changes.

}

private addGeolocateControl(): boolean {
if (
!this.map ||
Expand Down
62 changes: 62 additions & 0 deletions tests/map-controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -842,3 +842,65 @@ describe("MapController geolocate permission-denied recovery", () => {
}
}));
});

interface LayerLabelWindow {
__GEOLIBRE_LAYER_LABELS__?: Record<string, string>;
dispatchEvent: (event: unknown) => boolean;
}

// publishLayerDisplayNames is guarded on `window`, which `node --test` lacks,
// so stub a minimal one for the duration of a test. Dispatched event types are
// recorded so a test can assert the swipe panel's change event actually fires.
function withStubbedLabelWindow(
run: (win: LayerLabelWindow, dispatched: string[]) => void,
): void {
const globals = globalThis as { window?: LayerLabelWindow };
const original = globals.window;
const dispatched: string[] = [];
const stub: LayerLabelWindow = {
dispatchEvent: (event: unknown) => {
dispatched.push((event as { type: string }).type);
return true;
},
};
globals.window = stub;
try {
run(stub, dispatched);
} finally {
if (original === undefined) delete globals.window;
else globals.window = original;
}
}

describe("MapController base-layer label", () => {
it("publishes the grouped basemap label so the swipe panel can localize it", () => {
withStubbedLabelWindow((win, dispatched) => {
const controller = createMapController();

// An explicit English push publishes under the "__basemap__" key the
// swipe panel reads, and fires the change event the panel listens for.
controller.setBackgroundLabel("Background");
assert.equal(win.__GEOLIBRE_LAYER_LABELS__?.__basemap__, "Background");
assert.deepEqual(dispatched, ["geolibre-layer-labels-change"]);

// A language change re-publishes the translated label under the same key
// and fires the event again so the panel re-syncs.
controller.setBackgroundLabel("Hintergrund");
assert.equal(win.__GEOLIBRE_LAYER_LABELS__?.__basemap__, "Hintergrund");
assert.equal(dispatched.length, 2);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor test-quality nit: the second dispatch assertion only checks the count but not the event name. The first check (assert.deepEqual(dispatched, ["geolibre-layer-labels-change"])) is strict; the second should be equally strict:

Suggested change
assert.equal(dispatched.length, 2);
assert.deepEqual(dispatched, [
"geolibre-layer-labels-change",
"geolibre-layer-labels-change",
]);

Also: the companion "clears published labels on destroy" test (withStubbedLabelWindow((win) => …)) ignores the dispatched array entirely, so it does not verify that clearLayerDisplayNames() fires (or does not fire) the change event on teardown. Consider threading in dispatched there too and asserting dispatched.length === 2 (one for setBackgroundLabel, one for destroy).

});
});

it("clears published labels on destroy", () => {
withStubbedLabelWindow((win) => {
const controller = createMapController();
controller.setBackgroundLabel("Background");
assert.equal(win.__GEOLIBRE_LAYER_LABELS__?.__basemap__, "Background");

// Teardown clears the bridge entirely (including the basemap entry),
// rather than leaving the last label behind.
controller.destroy();
assert.deepEqual(win.__GEOLIBRE_LAYER_LABELS__, {});
});
});
});
Comment thread
giswqs marked this conversation as resolved.
Loading