[FVEM-255] [feat] FAST-EM add tool ROA and TOA editing #3336
Conversation
📝 WalkthroughWalkthroughAdds a new ROI tool (TOOL_ROI) to the UI and tool-management flow and registers it in the toolbar. FastEMMainCanvas gains a public collection Sequence DiagramsequenceDiagram
participant User
participant FastEMMainTab
participant FastEMMainCanvas
participant ShapesOverlay_ROI as ShapesOverlay(ROI)
participant EditableShape
User->>FastEMMainTab: select TOOL_ROI
FastEMMainTab->>FastEMMainTab: set active tool = TOOL_ROI
FastEMMainTab->>FastEMMainCanvas: notify tool change
FastEMMainCanvas->>FastEMMainCanvas: _on_shape_tool() invoked
alt TOOL_ROI active
FastEMMainCanvas->>ShapesOverlay_ROI: create/register ROI overlay (shape_cls=EditableShape, shape_creation_allowed=...)
FastEMMainCanvas->>FastEMMainCanvas: append overlay to shapes_overlay and rocs_overlay
FastEMMainCanvas->>ShapesOverlay_ROI: set active = true
ShapesOverlay_ROI->>EditableShape: accept clicks to create/edit ROI (respect shape_creation_allowed)
EditableShape-->>User: provide ROI editing/creation interaction
else TOOL_ROI not active
FastEMMainCanvas->>ShapesOverlay_ROI: set active = false (for each in rocs_overlay)
ShapesOverlay_ROI->>EditableShape: disable ROI interactions
end
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/odemis/gui/comp/miccanvas.py`:
- Line 2004: The line toggling self.roc_overlay.active.value in _on_shape_tool()
can raise AttributeError if roc_overlay is None because add_calibration_shape()
hasn't run; update _on_shape_tool() (subscribed in setView(init=True)) to first
check that self.roc_overlay is not None (and that it has an .active attribute)
before accessing .active.value—if roc_overlay is None simply skip the toggle or
initialize/attach a default overlay; reference roc_overlay,
add_calibration_shape(), _on_shape_tool(), and setView() when making the change.
In `@src/odemis/gui/comp/overlay/shapes.py`:
- Around line 373-376: The comment above the conditional using
self.shape_creation_allowed is misleading because the code creates a new shape
when shape_creation_allowed is True; update or remove the comment to accurately
reflect behavior (e.g., "when shape_creation_allowed is True, always create a
new shape") and ensure the block with self._selected_shape =
self._create_new_shape() and self._is_new_shape = True matches that description
(or adjust logic if the intended behavior differs).
🧹 Nitpick comments (2)
src/odemis/gui/comp/overlay/shapes.py (1)
187-187: Add type hint forshape_creation_allowedparameter.The parameter is missing a type hint. As per coding guidelines, type hints should be used for function parameters.
Suggested fix
- shape_creation_allowed=True, + shape_creation_allowed: bool = True,src/odemis/gui/comp/miccanvas.py (1)
1975-1985: Using abstract classEditableShapeasshape_clsmay cause issues if misused.
EditableShapeis an abstract base class with abstract methods. Whileshape_creation_allowed=Falseprevents_create_new_shape()from being called (which would fail since abstract classes cannot be instantiated), this creates a maintenance hazard. If someone later changesshape_creation_allowedtoTrueor refactors this code, it will fail at runtime.Consider adding a comment to clarify this constraint, or using a concrete implementation.
The tools for drawing an ROA or TOA were also used for selecting and editing ROAs and TOAs. Both selecting a shape and creation of a shape used left_down, this mean that when trying to draw a shape in another shape it would just select the shape and not draw a new one. This commit introduces a new tool that needs to be selected when you want to edit or move a shape. Another advantage is that previously for editing a rectangle you would need to select the rectangle tool, and the ellipse tool for ellipses etc.. Now the new tool can be used to edit and move any shape.
tepals
force-pushed
the
FVEM-255-improved-mouse-control-toa-roa-tool
branch
from
122d36b to
19d1a59
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/comp/overlay/shapes.py (1)
179-199: Add full type hints and remove type information from docstring entries.Parameters lack type hints, and the docstring includes type information in parentheses that should be removed per the reStructuredText style guide (without type information). Update the imports to include
AnyandType, then add type hints for all parameters and the return type, and remove type markers from the docstring.♻️ Suggested update
-from typing import Deque, List, Optional, Tuple, Union +from typing import Any, Deque, List, Optional, Tuple, Type, Union class ShapesOverlay(WorldOverlay): def __init__( self, - cnvs, - shape_cls, - tool=None, - tool_va=None, - shapes_va=None, - shape_to_copy_va=None, - shape_creation_allowed=True, - ): + cnvs: Any, + shape_cls: Type[EditableShape], + tool: Optional[int] = None, + tool_va: Optional[Any] = None, + shapes_va: Optional[Any] = None, + shape_to_copy_va: Optional[Any] = None, + shape_creation_allowed: bool = True, + ) -> None: """ :param cnvs: canvas for the overlay. - :param shape_cls: (EditableShape) The shape class whose creation, editing and removal + :param shape_cls: The shape class whose creation, editing and removal will be handled by this class. - :param tool: (int) The tool id of the shape cls. - :param tool_va: (None or VA of value TOOL_*) ShapesOverlay will be activated using this VA. + :param tool: The tool id of the shape cls. + :param tool_va: ShapesOverlay will be activated using this VA. If None, the user should externally implement a method to activate the ShapesOverlay. :param shapes_va: Possibility to pass a shared VA whose value is the list of all shapes. :param shape_to_copy_va: Possibility to pass a shared VA whose value is the shape to copy object. - :param shape_creation_allowed: (bool) If True, new shapes can be created. If False, only existing + :param shape_creation_allowed: If True, new shapes can be created. If False, only existing shapes can be selected and edited. """
tepals
changed the title
The statement is not correct! Previously one could edit an ellipse or polygon when rectangle tool was selected and likewise. |
|
@tepals we forgot to test one thing during pair-programming, if the undo-redo functionality is still working fine. |
undo-redo is working, just not across functionality. If the editing tool is selected I can undo-redo editing actions. If I then select a drawing tool it undos and redos the new drawing actions, but not the editing actions. I think this is fine. |
|
@nandishjpatel I applied your suggested changes and I can now only move the calibration boxes when the cursor tool is selected. Is that your intended behavior? I am not sure if it would make sense for the user to be able to move the calibration boxes when the editing is selected, or not. What do you think? EDIT: I discussed with Marre and using the cursor tool for moving calibration boxes makes the most sense 
|
…reation and editing of shapes
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/comp/miccanvas.py (1)
1857-1870:⚠️ Potential issue | 🟡 MinorRemove stale calibration overlays from
rocs_overlaylist when overlays are removed.When
remove_shape()removes an overlay viaremove_world_overlay(), it does not clean up the corresponding entry fromself.rocs_overlay(line 1866). This causes stale references to remain, which are later accessed during tool state changes (line 2005-2006:roc_ovrely.active.value = ...). Add cleanup inremove_shape()to remove the overlay fromrocs_overlay, or implement a clear mechanism during project reload.
🤖 Fix all issues with AI agents
In `@src/odemis/gui/comp/miccanvas.py`:
- Around line 1976-1987: The ROI overlay currently allows pasting via
shape_to_copy even when shape_creation_allowed is False; update ShapesOverlay to
block that by adding a gate: in ShapesOverlay.on_left_down() (or inside its
helper _copy_shape()) check shape_creation_allowed before performing any
paste/copy action, and/or clear shape_to_copy when the overlay is created for
TOOL_ROI; specifically, modify the logic around _copy_shape(),
shape_creation_allowed, and shape_to_copy so that if the overlay’s tool ==
guimodel.TOOL_ROI or shape_creation_allowed is False, the paste path is skipped
and shape_to_copy is not used (or is cleared) to prevent creating new shapes for
ROI-only overlays.
🧹 Nitpick comments (3)
src/odemis/gui/comp/miccanvas.py (3)
1834-1834: Consider typingrocs_overlayfor clarity.
This makes intent explicit and helps tooling.♻️ Suggested type annotation
- self.rocs_overlay = [] + self.rocs_overlay: list[FastEMROCOverlay] = []
1857-1870: Add type hints + reST docstring foradd_calibration_shape.
This method is touched here and should meet the Python guidelines.✍️ Suggested signature + docstring update
+from typing import Any- def add_calibration_shape(self, coordinates, label, sample_bbox, colour=gui.FG_COLOUR_WARNING): - """ - coordinates (TupleContinuousVA): VA of 4 floats representing region of calibration coordinates - label (str): label for the overlay (typically a number 1-9) - sample_bbox (tuple): bounding box coordinates of the sample holder (minx, miny, maxx, maxy) [m] - colour (str): border colour of ROA overlay, given as string of hex code - """ + def add_calibration_shape( + self, + coordinates: Any, + label: str, + sample_bbox: tuple[float, float, float, float], + colour: str = gui.FG_COLOUR_WARNING, + ) -> FastEMROCOverlay: + """ + Add a calibration shape overlay. + + :param coordinates: VA of 4 floats representing region of calibration coordinates. + :param label: Label for the overlay (typically a number 1-9). + :param sample_bbox: Bounding box coordinates of the sample holder (minx, miny, maxx, maxy) [m]. + :param colour: Border colour of ROA overlay, given as string of hex code. + :returns: The created calibration overlay. + """As per coding guidelines,
**/*.py: Always use type hints for function parameters and return types in Python code. Include docstrings for all functions and classes, following the reStructuredText style guide (without type information).
1997-2006: Add type hints + reST docstring for_on_shape_tool.✍️ Suggested signature + docstring update
- def _on_shape_tool(self, tool_id): + def _on_shape_tool(self, tool_id: int) -> None: + """ + Handle tool changes for shape overlays. + + :param tool_id: Selected tool id. + """ self.is_shape_tool_active = tool_id in (As per coding guidelines,
**/*.py: Always use type hints for function parameters and return types in Python code. Include docstrings for all functions and classes, following the reStructuredText style guide (without type information).
2 participants
The tools for drawing an ROA or TOA were also used for selecting and editing ROAs and TOAs. Both selecting a shape and creation of a shape used left_down, this mean that when trying to draw a shape in another shape it would just select the shape and not draw a new one. This commit introduces a new tool that needs to be selected when you want to edit or move a shape. Another advantage is that previously for editing a rectangle you would need to select the rectangle tool, and the ellipse tool for ellipses etc.. Now the new tool can be used to edit and move any shape.
FVEM-255