Feat/ref distances v2 #17
Conversation
…te for reference distance feature
…ng reference distances
…ne module with comprehensive tests
…th refactored state management
…istance input modal for reference distance entry with validation and unit selection
…es for scale and measure markers and labels in the UI
…into a reusable function for cleaner code
…d rely on conversion function for improved clarity
…ility and integrate into distance validation
…lined user experience
…structure. I also utilized createScaleModeState() and createMeasureModeState() from the logic module to ensure consistent initial states. Logic Integration: Updated all calls to pure functions (like handleScaleModePoint or computeMeasurement) to pass only the logic sub-object. UI Management: Updated all Leaflet-related code to store and retrieve markers/lines from the ui sub-object
Summary of ChangesHello @cs-util, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application by introducing robust functionality for manual scale definition and distance measurement on uploaded photos. It achieves this through a well-structured refactoring that separates core logic into testable modules and integrates new UI elements, laying a solid foundation for future distance-related features. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that introduces a significant and well-architected feature for defining reference distances and measuring on the map. The separation of concerns is particularly noteworthy, with pure functions for calculations (scale.js), a clean state machine for interaction logic (scale-mode.js), and UI-specific code in index.js. This makes the new functionality robust, maintainable, and highly testable. The addition of comprehensive unit tests for the new modules further demonstrates a commitment to quality.
I've added a couple of suggestions for minor improvements related to logic and performance. Overall, this is a very impressive piece of work.
| function handleDistanceModalConfirm() { | ||
| const inputValue = dom.distanceInput.value; | ||
| const unit = dom.distanceUnit.value; | ||
|
|
||
| // Update preferred unit for future use | ||
| state.preferredUnit = unit; | ||
|
|
||
| const validation = validateDistanceInput(inputValue, unit); | ||
|
|
||
| if (!validation.valid) { | ||
| dom.distanceError.classList.remove('hidden'); | ||
| dom.distanceInput.focus(); | ||
| return; | ||
| } | ||
|
|
||
| const { meters } = validation; | ||
|
|
||
| hideDistanceModal(); | ||
|
|
||
| const result = computeReferenceDistanceFromInput(state.scaleMode.logic, meters); | ||
|
|
||
| if (!result.success) { | ||
| showToast('Could not compute scale. Points may be too close.', { tone: 'warning' }); | ||
| cancelScaleMode(); | ||
| return; | ||
| } | ||
|
|
||
| state.referenceDistance = result.referenceDistance; | ||
| clearScaleModeMarkers(); | ||
| state.scaleMode.logic = cancelScaleModeState(); | ||
|
|
||
| drawReferenceVisualization(); | ||
| updateMeasureButtonState(); | ||
| showToast(`Scale set: ${formatDistance(result.referenceDistance.meters, state.preferredUnit)} = ${result.referenceDistance.metersPerPixel.toFixed(4)} m/px`, { tone: 'success' }); | ||
| } |
There was a problem hiding this comment.
The user's preferred unit (state.preferredUnit) is updated immediately, even if the subsequent validation of the distance input fails. This could lead to saving a unit preference from an otherwise invalid submission. It would be more robust to update the preferred unit only after the input has been successfully validated.
function handleDistanceModalConfirm() {
const inputValue = dom.distanceInput.value;
const unit = dom.distanceUnit.value;
const validation = validateDistanceInput(inputValue, unit);
if (!validation.valid) {
dom.distanceError.classList.remove('hidden');
dom.distanceInput.focus();
return;
}
// Update preferred unit for future use only on success
state.preferredUnit = unit;
const { meters } = validation;
hideDistanceModal();
const result = computeReferenceDistanceFromInput(state.scaleMode.logic, meters);
if (!result.success) {
showToast('Could not compute scale. Points may be too close.', { tone: 'warning' });
cancelScaleMode();
return;
}
state.referenceDistance = result.referenceDistance;
clearScaleModeMarkers();
state.scaleMode.logic = cancelScaleModeState();
drawReferenceVisualization();
updateMeasureButtonState();
showToast(`Scale set: ${formatDistance(result.referenceDistance.meters, state.preferredUnit)} = ${result.referenceDistance.metersPerPixel.toFixed(4)} m/px`, { tone: 'success' });
}| function updateMeasureLabel() { | ||
| const { p1, p2 } = state.measureMode.logic; | ||
| if (!p1 || !p2) { | ||
| return; | ||
| } | ||
|
|
||
| const result = computeMeasurement(state.measureMode.logic, state); | ||
| if (!result.success) { | ||
| return; | ||
| } | ||
|
|
||
| const midLat = (p1.y + p2.y) / 2; | ||
| const midLng = (p1.x + p2.x) / 2; | ||
| const sourceIcon = result.source === 'manual' ? '📏' : '📡'; | ||
|
|
||
| if (state.measureMode.ui.label) { | ||
| state.measureMode.ui.label.remove(); | ||
| } | ||
|
|
||
| state.measureMode.ui.label = L.marker(L.latLng(midLat, midLng), { | ||
| icon: L.divIcon({ | ||
| className: 'measure-label', | ||
| html: `<div class="distance-label distance-label--measure" style="background:${COLORS.MEASURE};">${sourceIcon} ${formatDistance(result.meters, state.preferredUnit)}</div>`, | ||
| iconAnchor: [0, 0], | ||
| }), | ||
| }).addTo(state.photoMap); | ||
| } |
There was a problem hiding this comment.
During drag operations in measure mode, this function is called frequently. Removing and recreating the label marker (state.measureMode.ui.label) on every call is inefficient and can cause flickering or performance issues.
A more performant approach is to create the marker once and then update its position using setLatLng() and its content by directly manipulating its DOM element's innerHTML. This avoids the overhead of repeated marker creation and removal from the map.
function updateMeasureLabel() {
const { p1, p2 } = state.measureMode.logic;
if (!p1 || !p2) {
if (state.measureMode.ui.label) {
state.measureMode.ui.label.remove();
state.measureMode.ui.label = null;
}
return;
}
const result = computeMeasurement(state.measureMode.logic, state);
if (!result.success) {
return;
}
const midLat = (p1.y + p2.y) / 2;
const midLng = (p1.x + p2.x) / 2;
const sourceIcon = result.source === 'manual' ? '📏' : '📡';
const midPoint = L.latLng(midLat, midLng);
const newHtml = `<div class="distance-label distance-label--measure" style="background:${COLORS.MEASURE};">${sourceIcon} ${formatDistance(result.meters, state.preferredUnit)}</div>`;
if (state.measureMode.ui.label) {
state.measureMode.ui.label.setLatLng(midPoint);
const iconElement = state.measureMode.ui.label.getElement();
if (iconElement && iconElement.innerHTML !== newHtml) {
iconElement.innerHTML = newHtml;
}
} else {
state.measureMode.ui.label = L.marker(midPoint, {
icon: L.divIcon({
className: 'measure-label',
html: newHtml,
iconAnchor: [0, 0],
}),
}).addTo(state.photoMap);
}
}
No description provided.