Web viewer: ruler enhancements#10444
Conversation
…or editing Display control: - Add Misc > Rulers visibility toggle (default on) to show/hide all rulers and snap indicators, matching the Qt GUI. Manhattan L-shaped rulers: - When euclidian=false, draw an L-shaped path through a join point instead of a straight line. Join point matches the Qt GUI's getManhattanJoinPt(): horizontal-first when |dx|>=|dy|, vertical- first otherwise. Endcap ticks perpendicular to each segment. Distance label at the corner. Inspector editing: - Ruler properties (Name, Label, Point coordinates, Euclidian) are now editable in the inspector. Click to edit, Enter to confirm, Escape to cancel. Generic inspector support via editable flag and onPropertyChange callback. Auto-select: - Newly created rulers are immediately selected in the inspector. Signed-off-by: Matt Liberty <matt.liberty@gmail.com> Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
![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 pull request introduces Manhattan (L-shaped) paths for rulers, adds a toggle for ruler visibility in the display controls, and implements editable properties within the inspector panel. The feedback highlights several redundant function calls in ruler.js where _rerenderAll is triggered multiple times unnecessarily. Additionally, there is a concern regarding the inspector's re-rendering logic on blur events, which currently disrupts keyboard navigation and focus flow.
| this._rulers.push(ruler); | ||
| this._renderRuler(ruler); | ||
| this._selectRuler(id); |
There was a problem hiding this comment.
The call to _renderRuler(ruler) is redundant here. The subsequent call to _selectRuler(id) (line 395) invokes _rerenderAll(), which clears the layer group and renders all rulers in the list, including the one just added.
| this._rulers.push(ruler); | |
| this._renderRuler(ruler); | |
| this._selectRuler(id); | |
| this._rulers.push(ruler); | |
| this._selectRuler(id); |
| this._rerenderAll(); | ||
| this._selectRuler(ruler.id); |
There was a problem hiding this comment.
The call to this._rerenderAll() is redundant. this._selectRuler(ruler.id) already calls _rerenderAll() as its first step (line 492).
| this._rerenderAll(); | |
| this._selectRuler(ruler.id); | |
| this._selectRuler(ruler.id); |
| valEl.addEventListener('blur', () => { | ||
| const newVal = valEl.textContent; | ||
| if (newVal !== prop.value) { | ||
| data.onPropertyChange(prop.name, newVal); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Re-rendering the entire inspector panel on every blur event causes the DOM elements to be destroyed and recreated. This results in a loss of focus, which breaks keyboard navigation (e.g., using the Tab key to move between editable fields). Consider using a more targeted update approach or deferring the re-render to avoid interrupting the user's focus flow.
1 participant
Summary
Add three ruler enhancements to match the Qt GUI:
editableflag andonPropertyChangecallbackType of Change
Impact
Rulers now have a visibility toggle under Misc, can display Manhattan (L-shaped) paths, and support inline editing of properties in the inspector panel. New rulers are auto-selected for immediate inspection.
Verification
./etc/Build.sh).Related Issues
None