Skip to content
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

fix osm data warning and fix infinite loop #2556

Merged
merged 1 commit into from
Feb 17, 2025
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
16 changes: 15 additions & 1 deletion src/components/EnhancedMap/OSMDataLayer/OSMDataLayer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,19 @@ const generateLayer = (props, map, _leaflet) => {
pane: props.leaflet?.pane,
});

// Clear existing layers to ensure proper ordering
layerGroup.clearLayers();

// Get all features and sort them by type to ensure consistent ordering
const features = layerGroup.buildFeatures(props.xmlData);
const sortedFeatures = features.sort((a, b) => {
const typeOrder = { way: 1, area: 2, node: 3, changeset: 4 };
return typeOrder[a.type] - typeOrder[b.type];
});

// Add features in the sorted order
layerGroup.addData(sortedFeatures);

layerGroup.eachLayer((layer) => {
layer.options.mrLayerLabel = props.intl.formatMessage(layerMessages.showOSMDataLabel);
layer.options.fill = false;
Expand Down Expand Up @@ -166,7 +179,8 @@ const OSMDataLayerComponent = createPathComponent(
if (!_isEqual(prevProps.showOSMElements, props.showOSMElements)) {
instance.clearLayers();
const newLayers = generateLayer(props, instance);
newLayers.eachLayer((layer) => instance.addLayer(layer));
// Transfer all layers at once to maintain ordering
instance.addLayer(newLayers);
}
},
);
Expand Down
1 change: 1 addition & 0 deletions src/components/HOCs/WithCurrentTask/WithCurrentTask.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ export const mapDispatchToProps = (dispatch, ownProps) => {
fetchOSMData: (bbox) => {
return fetchOSMData(bbox).catch((error) => {
dispatch(addError(error));
throw error;
});
},

Expand Down
17 changes: 17 additions & 0 deletions src/components/TaskPane/TaskMap/Messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,21 @@ export default defineMessages({
id: "Map.layerSelectionList.header",
defaultMessage: "Select Desired Feature",
},
osmDataAreaTooLarge: {
id: "Task.osmData.areaTooLarge",
defaultMessage:
"The selected area is too large to load OSM data. Please zoom in further to view OSM features.",
},
osmDataTooLarge: {
id: "Task.map.osmData.tooLarge",
defaultMessage: "OSM Data Area Too Large",
},
zoomInForOSMData: {
id: "Task.map.osmData.zoomInRequired",
defaultMessage: "Please zoom in closer to view OSM data for this area",
},
osmDataError: {
id: "Task.map.osmData.error",
defaultMessage: "Error Loading OSM Data",
},
});
85 changes: 58 additions & 27 deletions src/components/TaskPane/TaskMap/TaskMap.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ export const TaskMapContent = (props) => {
const [showTaskFeatures, setShowTaskFeatures] = useState(true);
const [osmData, setOsmData] = useState(null);
const [showOSMData, setShowOSMData] = useState(false);
const [showOSMElements, setShowOSMElements] = useState({ nodes: true, ways: true, areas: true });
const [showOSMElements, setShowOSMElements] = useState({
nodes: true,
ways: true,
areas: true,
});
const [osmDataLoading, setOsmDataLoading] = useState(false);
const [mapillaryViewerImage, setMapillaryViewerImage] = useState(null);
const [openStreetCamViewerImage, setOpenStreetCamViewerImage] = useState(null);
Expand Down Expand Up @@ -264,43 +268,62 @@ export const TaskMapContent = (props) => {
setShowTaskFeatures((prevState) => !prevState);
};

const fetchOSMData = () => {
setOsmDataLoading(true);
props.fetchOSMData(map.getBounds().toBBoxString()).then((xmlData) => {
// Indicate the map should skip fitting to bounds as the OSM data could
// extend beyond the current view and we don't want the map to zoom out
setOsmData(xmlData);
const fetchOSMData = async () => {
try {
if (showOSMData) {
const bounds = map.getBounds()?.toBBoxString();
if (!bounds) {
throw new Error("Invalid map bounds");
}

const xmlData = await props.fetchOSMData(bounds);
setOsmData(xmlData);
setShowOSMData(true);
} else {
setOsmData(null);
setShowOSMData(false);
}
} catch (error) {
console.error("Error handling OSM data:", error);
setOsmData(null);
setShowOSMData(false);
} finally {
setOsmDataLoading(false);
});
}
};

/**
* Invoked by LayerToggle when the user wishes to toggle visibility of
* OSM data on or off.
*/
const toggleOSMDataVisibility = () => {
const toggleOSMDataVisibility = async () => {
// Prevent multiple requests while loading
if (osmDataLoading) {
return;
}

const loadOSMData = !showOSMData;
setOsmDataLoading(true);
setShowOSMData(loadOSMData);

if (loadOSMData) {
props
.fetchOSMData(map.getBounds().toBBoxString())
.then((xmlData) => {
setOsmData(xmlData);
})
.catch((error) => {
console.error("Error fetching OSM data:", error);
})
.finally(() => {
setOsmDataLoading(false);
});
} else {

try {
if (loadOSMData) {
const bounds = map.getBounds()?.toBBoxString();
if (!bounds) {
throw new Error("Invalid map bounds");
}

const xmlData = await props.fetchOSMData(bounds);
setOsmData(xmlData);
setShowOSMData(true);
} else {
setOsmData(null);
setShowOSMData(false);
}
} catch (error) {
console.error("Error handling OSM data:", error);
setOsmData(null);
setShowOSMData(false);
} finally {
setOsmDataLoading(false);
}
};
Expand Down Expand Up @@ -426,7 +449,7 @@ export const TaskMapContent = (props) => {
if (showOSMData && !osmData) {
fetchOSMData();
}
}, [props, osmData]);
}, [showOSMData, osmData]);

useEffect(() => {
if (features.length !== 0) {
Expand Down Expand Up @@ -625,7 +648,11 @@ export const TaskMapContent = (props) => {
}

return (
<div className={classNames("task-map task", { "full-screen-map": props.isMobile })}>
<div
className={classNames("task-map task", {
"full-screen-map": props.isMobile,
})}
>
<LayerToggle
{...props}
showTaskFeatures={showTaskFeatures}
Expand Down Expand Up @@ -681,7 +708,11 @@ const TaskMap = (props) => {
};

return (
<div className={classNames("task-map task", { "full-screen-map": props.isMobile })}>
<div
className={classNames("task-map task", {
"full-screen-map": props.isMobile,
})}
>
<MapContainer
taskBundle={props.taskBundle}
center={props.centerPoint}
Expand Down
Loading