Implemented different eraser and selector modes#1111
Conversation
(which also replaces the fullSelection corner setting)
|
Yippie, I got number #1111 |
|
Hi, thanks for contributing. 1111 :) |
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified HitElementMode to control how elements are hit-tested for selection and erasing, aiming to address cases like #833 (erasing inside a shape) and to replace the previous “full selection” utility toggle with a per-tool setting.
Changes:
- Added
HitElementModeto tool models (select/eraser/path eraser) and wired it through selection/eraser handlers intoDocumentBlocray casting. - Updated hit-testing APIs (
HitCalculator) and implementations (notably shapes) to supportnone/full/touchEdges/touchAnywhere. - Updated tool property UIs and English localization; removed the old
fullSelectionutility state + UI.
Reviewed changes
Copilot reviewed 18 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| app/lib/visualizer/property.dart | Adds localized labels for the new HitElementMode values in the UI. |
| app/lib/selections/tools/tool.dart | Adds SelectToolSelection wiring for tool property panels. |
| app/lib/selections/tools/select.dart | New selection-tool properties UI for choosing HitElementMode. |
| app/lib/selections/tools/path_eraser.dart | Adds “Erase shapes” mode UI + renames “Delete elements” label. |
| app/lib/selections/tools/eraser.dart | Adds “Erase shapes” mode UI + renames “Delete elements” label. |
| app/lib/selections/selection.dart | Registers the new select-tool selection panel part. |
| app/lib/selections/document.dart | Removes the old fullSelection checkbox from document utilities UI. |
| app/lib/renderers/renderer.dart | Updates HitCalculator API and DefaultHitCalculator signature to use HitElementMode. |
| app/lib/renderers/elements/shape.dart | Implements HitElementMode semantics for shape hit-testing (incl. touchEdges/none). |
| app/lib/renderers/elements/polygon.dart | Updates polygon hit-testing signature to accept HitElementMode (logic mostly unchanged). |
| app/lib/renderers/elements/pen.dart | Updates pen-path hit-testing signature to accept HitElementMode (logic mostly unchanged). |
| app/lib/l10n/app_en.arb | Adds English strings for new modes and renames the eraser checkbox label. |
| app/lib/handlers/select.dart | Passes hitElementMode from SelectTool into ray casting. |
| app/lib/handlers/path_eraser.dart | Passes hitElementMode from PathEraserTool into ray casting. |
| app/lib/handlers/eraser.dart | Passes hitElementMode from EraserTool into ray casting. |
| app/lib/bloc/document_bloc.dart | Replaces full raycast parameter with hitElementMode; removes unused typedef. |
| api/lib/src/models/utilities.g.dart | Removes fullSelection from utilities JSON serialization. |
| api/lib/src/models/utilities.freezed.dart | Removes fullSelection from the freezed utilities model. |
| api/lib/src/models/utilities.dart | Removes fullSelection field from UtilitiesState. |
| api/lib/src/models/tool.g.dart | Adds JSON (de)serialization for hitElementMode and HitElementMode enum mapping. |
| api/lib/src/models/tool.freezed.dart | Adds hitElementMode to select/eraser/path eraser tool variants (generated). |
| api/lib/src/models/tool.dart | Introduces HitElementMode enum and adds hitElementMode to select/eraser/path eraser tools. |
Files not reviewed (4)
- api/lib/src/models/tool.freezed.dart: Language not supported
- api/lib/src/models/tool.g.dart: Language not supported
- api/lib/src/models/utilities.freezed.dart: Language not supported
- api/lib/src/models/utilities.g.dart: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return switch (this) { | ||
| HitElementMode.none => loc.eraseShapeModeNone, | ||
| HitElementMode.touchEdges => loc.eraseShapeModeTouchEdges, | ||
| HitElementMode.touchAnywhere => loc.eraseShapeModeTouchAnywhere, | ||
| _ => '', // this shouldn't happen | ||
| }; | ||
| } else { | ||
| return switch (this) { | ||
| HitElementMode.full => loc.fullSelection, | ||
| HitElementMode.touchEdges => loc.selectElementModeTouchEdges, | ||
| HitElementMode.touchAnywhere => loc.selectElementModeTouchAnywhere, | ||
| _ => '', // this shouldn't happen | ||
| }; |
There was a problem hiding this comment.
@CodeDoctorDE Do we have something like this? loc.notSet() is a thing that's actually used for text in the app.
There was a problem hiding this comment.
yeah i like the idea of notSet:
Butterfly/app/lib/l10n/app_en.arb
Line 289 in 7cbf4f4
| return switch (this) { | ||
| HitElementMode.none => loc.eraseShapeModeNone, | ||
| HitElementMode.touchEdges => loc.eraseShapeModeTouchEdges, | ||
| HitElementMode.touchAnywhere => loc.eraseShapeModeTouchAnywhere, | ||
| _ => '', // this shouldn't happen | ||
| }; | ||
| } else { | ||
| return switch (this) { | ||
| HitElementMode.full => loc.fullSelection, | ||
| HitElementMode.touchEdges => loc.selectElementModeTouchEdges, | ||
| HitElementMode.touchAnywhere => loc.selectElementModeTouchAnywhere, | ||
| _ => '', // this shouldn't happen | ||
| }; |
There was a problem hiding this comment.
yeah i would add notSet also here
| if (hitElementMode == HitElementMode.full) { | ||
| return _allPointsInside(_points, rect.contains); | ||
| } |
There was a problem hiding this comment.
@CodeDoctorDE Should polygon elements also have the new behaviour that shapes have with this pr?
| @override | ||
| bool hit(Rect rect, {bool full = false}) { | ||
| bool hit( | ||
| Rect rect, { | ||
| HitElementMode hitElementMode = HitElementMode.touchAnywhere, | ||
| }) { | ||
| if (!this.rect.inflate(element.property.strokeWidth).overlaps(rect)) { |
There was a problem hiding this comment.
A long time I didn't add tests and I needed to fixed many regressions. I would prefer adding tests now for new features. I'm currently adding more and more tests to the app to have it in a good state
|
Oh, I think I build in a bug, where shapes that are fully contained in the lasso don't get selected. Let me fix that really quick |
|
Hi, can you update with the latest develop changes? |
|
Waiting for #1118 to be resolved so hitting rotating shapes can be tested |
# Conflicts: # app/lib/renderers/elements/shape.dart # app/lib/renderers/renderer.dart # app/test/renderers/shape_renderer_test.dart
|
These commits should also fix unknown bug I found where triangles can't be selected by a rect in full mode. Also selecting polygons should now work on any segement not only on the vertices. |
CodeDoctorDE
left a comment
There was a problem hiding this comment.
lgtm, will be merged once develop is on 2.6
|
I think polygon elements should also count as stroke elements as with this pr the interaction with them is the same as it is with shapes. |
|
yeah feel free to add it |
See #833