Skip to content

Commit c335783

Browse files
authored
fix(layers): keep nested groups in order and movable inside their parent (#1744)
* fix(layers): keep nested groups in order and movable inside their parent The #1739 fix gave every empty folder a position, but derived it from the nearest group in array order and, for a nested folder, from the end of its parent's range. Neither reads as a sibling, so as soon as one child of a group gained a layer its sibling folders were placed against the wrong block: a nested folder could be pushed past an unrelated top-level group and out of the parent it belongs to, and "move up"/"move down" on a nested group reported a change while the panel snapped straight back. Panel order now anchors an unpositioned group on its *siblings* — the groups sharing its parent — whose ranges span their whole subtrees. An ancestor's range already contains the slot being chosen and an unrelated group's covers a different branch, so neither can place it; with no sibling positioned yet a nested folder falls to the end of its parent's block and a top-level one to the top of the panel. Reordering follows the same shape: a group steps over a whole neighbouring sibling block instead of one unit at a time into it, and the edge of its parent is a wall, so a nested group moves among its siblings only and the menu greys out at the first and last child. A top-level group now clears a neighbouring subtree in one move rather than landing inside it. Group headers all come from one core walk (layerPanelGroupHeaders, replacing placeUnpositionedGroups), so the group a row belongs to, the organizers above it, and the empty folders are emitted parents-first against the same anchors. That is what keeps a nested folder below the parent it sits in, and it drops the panel's duplicate anchor logic. * Address CodeRabbit review feedback - Correct the reorder docs about the parent boundary. CodeRabbit read moveGroupThroughUnits as letting a nested group move across layers its parent owns directly, and proposed rejecting that move. The observation is right but the move is not a defect: those rows are one more block inside the parent, so ordering a child against them is a real reorder, and the group still cannot cross the parent's boundary. Rejecting it would leave no way to order a child folder against its parent's own layers. What was wrong was the docstring, which claimed the parent's own rows end the travel; it now describes the boundary as the wall and notes that an empty folder is the one case that cannot cross layer rows, matching the limit empty folders already have at the top level. - Pin both behaviours with regression tests: a populated child steps over its parent's own layers and then stops at an unrelated top-level group, and an empty child reports no move when only layer rows are adjacent. * Address Claude review feedback - Add automated coverage at two levels of nesting, which the earlier tests did not reach: a grandchild folder placed by its own sibling rather than by either ancestor, the full ancestor-first header chain drawn against one layer row, and a grandchild moving among its siblings with its own parent (not the outer group) as the wall. branchUnder's ancestor walk and the sibling-anchoring loop are the parts that could regress at depth 2 without a test noticing.
1 parent 3108e4b commit c335783

3 files changed

Lines changed: 361 additions & 126 deletions

File tree

apps/geolibre-desktop/src/components/panels/LayerPanel.tsx

Lines changed: 13 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import {
3232
excludeHiddenFieldsFromGeojson,
3333
layerGroupDepth,
3434
layerGroupMoveability,
35-
placeUnpositionedGroups,
35+
layerPanelGroupHeaders,
3636
} from "@geolibre/core";
3737
import type { EllipsoidId, GeoLibreLayer, LayerGroup } from "@geolibre/core";
3838
import type { FeatureCollection } from "geojson";
@@ -920,56 +920,14 @@ export function LayerPanel({
920920
}),
921921
[groupById, layerGroups],
922922
);
923-
const firstMemberIdByGroup = useMemo(() => {
924-
const map = new Map<string, string>();
925-
for (const layer of visibleLayers) {
926-
if (layer.groupId && !map.has(layer.groupId)) {
927-
map.set(layer.groupId, layer.id);
928-
}
929-
}
930-
return map;
931-
}, [visibleLayers]);
932-
const descendantLayerAnchorByGroup = useMemo(() => {
933-
const result = new Map<string, string>();
934-
const displayGroupIds = visibleLayers
935-
.map((layer) => layer.groupId)
936-
.filter((id): id is string => Boolean(id && groupById.has(id)));
937-
for (const group of layerGroups) {
938-
if (firstMemberIdByGroup.has(group.id)) continue;
939-
const anchor = displayGroupIds.find((candidateId) => {
940-
let parentId = groupById.get(candidateId)?.parentId;
941-
const visited = new Set<string>();
942-
while (parentId && !visited.has(parentId)) {
943-
if (parentId === group.id) return true;
944-
visited.add(parentId);
945-
parentId = groupById.get(parentId)?.parentId;
946-
}
947-
return false;
948-
});
949-
if (anchor) result.set(group.id, anchor);
950-
}
951-
return result;
952-
}, [firstMemberIdByGroup, groupById, layerGroups, visibleLayers]);
953-
const organizerHeadersByAnchor = useMemo(() => {
954-
const result = new Map<string, LayerGroup[]>();
955-
for (const group of layerGroups) {
956-
const anchor = descendantLayerAnchorByGroup.get(group.id);
957-
if (!anchor) continue;
958-
const headers = result.get(anchor) ?? [];
959-
headers.push(group);
960-
result.set(anchor, headers);
961-
}
962-
for (const headers of result.values()) {
963-
headers.sort((a, b) => groupDepth(a) - groupDepth(b));
964-
}
965-
return result;
966-
}, [descendantLayerAnchorByGroup, groupDepth, layerGroups]);
967-
// Empty folders have no member to anchor them, so the core places them
968-
// against their neighbours in the group order instead: a folder keeps its
969-
// spot relative to the other folders when one of them gains a layer, and it
970-
// can still be reordered (GeoLibre#1739).
971-
const unpositionedGroups = useMemo(
972-
() => placeUnpositionedGroups(layers, layerGroups),
923+
// Every group header — the group a row belongs to, the organizers above it
924+
// whose layers all live in child groups, and the folders holding no layer at
925+
// all — comes from one core walk, anchored to the layer row it is drawn
926+
// above. Deriving them together is what keeps a nested folder below the
927+
// parent it sits in, and lets an empty folder keep its spot relative to its
928+
// siblings when one of them gains a layer (GeoLibre#1739).
929+
const groupHeaders = useMemo(
930+
() => layerPanelGroupHeaders(layers, layerGroups),
973931
[layers, layerGroups],
974932
);
975933
const groupMoveability = useMemo(
@@ -2939,7 +2897,6 @@ export function LayerPanel({
29392897
)}
29402898
{visibleLayers.map((layer, displayIndex) => {
29412899
const group = layer.groupId ? groupById.get(layer.groupId) : undefined;
2942-
const isFirstOfGroup = group ? firstMemberIdByGroup.get(group.id) === layer.id : false;
29432900
const groupCollapsed = group?.collapsed ?? false;
29442901
const groupAncestorCollapsed = group ? hasCollapsedAncestor(group) : false;
29452902
// When an ancestor group is hidden, a layer whose own visibility
@@ -3102,17 +3059,9 @@ export function LayerPanel({
31023059
const moveIds = selectedMoveIds(layer.id);
31033060
return (
31043061
<Fragment key={layer.id}>
3105-
{unpositionedGroups.aboveLayer.get(layer.id)?.map((empty) => (
3106-
<Fragment key={empty.id}>{renderGroupHeader(empty)}</Fragment>
3062+
{groupHeaders.aboveLayer.get(layer.id)?.map((header) => (
3063+
<Fragment key={header.id}>{renderGroupHeader(header)}</Fragment>
31073064
))}
3108-
{isFirstOfGroup &&
3109-
group &&
3110-
organizerHeadersByAnchor
3111-
.get(group.id)
3112-
?.map((organizer) => (
3113-
<Fragment key={organizer.id}>{renderGroupHeader(organizer)}</Fragment>
3114-
))}
3115-
{isFirstOfGroup && group && renderGroupHeader(group)}
31163065
{!groupCollapsed && !groupAncestorCollapsed && (
31173066
<div
31183067
data-layer-card=""
@@ -4016,9 +3965,9 @@ export function LayerPanel({
40163965
</Fragment>
40173966
);
40183967
})}
4019-
{/* Folders placed below the last layer row: the panel has no layer
3968+
{/* Headers placed below the last layer row: the panel has no layer
40203969
left to anchor them above. */}
4021-
{unpositionedGroups.bottom.map((group) => (
3970+
{groupHeaders.bottom.map((group) => (
40223971
<Fragment key={group.id}>{renderGroupHeader(group)}</Fragment>
40233972
))}
40243973
<div

0 commit comments

Comments
 (0)