[MSD-572] [feat] move milling patterns horizontally#3494
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds keyboard controls in the milling GUI so users can move the selected milling patterns horizontally using Ctrl+Arrow (large step) and Ctrl+Shift+Arrow (small step).
Changes:
- Bind a keyboard handler (
wx.EVT_CHAR) on the FIB canvas in the milling controller. - Implement
on_charto translate selected milling tasks horizontally based on key modifiers.
K4rishma
force-pushed
the
move-miilling-boxes-horizontally
branch
from
f7a8284 to
357eed9
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant User
participant MillingTaskController
participant Project
participant Canvas
User->>MillingTaskController: Ctrl+Left/Right key event
MillingTaskController->>MillingTaskController: on_char computes horizontal offset
MillingTaskController->>Project: save_project()
MillingTaskController->>Canvas: draw_milling_tasks()
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
src/odemis/gui/cont/milling.py (1)
366-371: 💤 Low valueConsider removing unreachable else clause.
The
elseclause at lines 370-371 is unreachable because thevalid_keyscheck at line 346 ensureskeyis onlywx.WXK_LEFTorwx.WXK_RIGHT. While defensive, this code can never execute.♻️ Proposed simplification
ref_img = feature.reference_image if key == wx.WXK_LEFT: view_dx = -step_px - elif key == wx.WXK_RIGHT: + else: # key == wx.WXK_RIGHT view_dx = step_px - else: - view_dx = 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/gui/cont/milling.py` around lines 366 - 371, The else clause that sets view_dx = 0 in the key handling block is unreachable because the valid_keys check ensures the key is only wx.WXK_LEFT or wx.WXK_RIGHT. Remove the else clause containing view_dx = 0 and keep only the if key == wx.WXK_LEFT and elif key == wx.WXK_RIGHT branches to eliminate this dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/gui/cont/milling.py`:
- Around line 333-338: The on_char method is missing type hints and has type
information embedded in the docstring. Add type hints to the evt parameter and
the return type of the on_char method. Then update the docstring to remove the
type annotation from the evt parameter description, keeping only the descriptive
part without the (wx.Event) type information, as type hints should be in the
function signature not the docstring per coding guidelines.
- Line 351: The logging.debug statement at line 351 contains an incorrect
message that says "mouse down event" but this code is in a keyboard event
handler, not a mouse event handler. Update the log message to accurately reflect
that this is a keyboard event handler rather than a mouse down event, ensuring
the message is appropriate for the actual context where it appears.
- Line 349: Remove the trailing whitespace characters from the blank line at
line 349 in the milling.py file. The blank line should contain no characters at
all. You can use the autopep8 tool with the command provided in the comment
(autopep8 --in-place --select W291,W292,W293,W391) to automatically clean up
this whitespace issue, or manually delete any spaces/tabs on that blank line.
---
Nitpick comments:
In `@src/odemis/gui/cont/milling.py`:
- Around line 366-371: The else clause that sets view_dx = 0 in the key handling
block is unreachable because the valid_keys check ensures the key is only
wx.WXK_LEFT or wx.WXK_RIGHT. Remove the else clause containing view_dx = 0 and
keep only the if key == wx.WXK_LEFT and elif key == wx.WXK_RIGHT branches to
eliminate this dead code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b5713a8-fc1b-438e-b120-6a8c37d94078
📒 Files selected for processing (1)
src/odemis/gui/cont/milling.py
K4rishma
force-pushed
the
move-miilling-boxes-horizontally
branch
from
357eed9 to
ca1ce23
Compare
ilyushkin
left a comment
ilyushkin
left a comment
There was a problem hiding this comment.
The patterns might not be all centered eventually.
|
Is this action executed before updating the view position with arrow keys? I'm asking because it is also possible to move the view in the IV quadrant of the FIBSEM tab with Ctrl (Ctrl/Shift) + arrows. See odemis/src/odemis/gui/comp/canvas.py DraggableCanvas.on_char(). |
K4rishma
force-pushed
the
move-miilling-boxes-horizontally
branch
from
ca1ce23 to
8f977f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/gui/cont/milling.py`:
- Around line 359-395: The Ctrl+Arrow branch in on_char for milling.py is
handling pattern movement and then returning without letting the event continue,
which blocks the canvas’s default keyboard navigation. Update the
ctrl_mod/allow_milling_pattern_move path in on_char to call evt.Skip() before
exiting when the BaseCanvas handling should still run, so the milling move logic
and normal panning/navigation can both occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c4b36c8-0ea3-459a-a94e-136faa520acc
📒 Files selected for processing (1)
src/odemis/gui/cont/milling.py
| if (ctrl_mod | ||
| and self.allow_milling_pattern_move | ||
| and feature and feature.reference_image is not None | ||
| ): | ||
| # move in small step when both ctrl and shift are pressed, bigger step with only ctrl | ||
| if shift_mod: | ||
| step_px = MOVE_DELTA_X_SHORT | ||
| else: | ||
| step_px = MOVE_DELTA_X_LONG | ||
| ref_img = feature.reference_image | ||
| if key == wx.WXK_LEFT: | ||
| view_dx = -step_px | ||
| elif key == wx.WXK_RIGHT: | ||
| view_dx = step_px | ||
| else: | ||
| view_dx = 0 | ||
|
|
||
| # find the center position of each milling pattern and calculate the shift | ||
| # such that all milling patterns move horizontally by equidistance | ||
| for task in self.milling_tasks.values(): | ||
| for pattern in task.patterns: | ||
| selected_center_rel = pattern.center.value | ||
| offset = active_canvas.get_half_buffer_size() | ||
| center_phys = pos_to_absolute(selected_center_rel, ref_img) | ||
| center_view = active_canvas.phys_to_view(center_phys, offset) | ||
| new_center_view = (center_view[0] + view_dx, center_view[1]) | ||
| new_center_phys = active_canvas.view_to_phys(new_center_view, offset) | ||
| logging.debug(f"Move milling pattern {pattern.name.value} horizontally from physical position" | ||
| f" {center_phys}, to new position {new_center_phys}") | ||
|
|
||
| # move selected pattern to position | ||
| relative_pos = pos_to_relative(new_center_phys, feature.reference_image) | ||
| pattern.center.value = relative_pos | ||
|
|
||
| save_project(self._tab_data.main) | ||
| self.draw_milling_tasks() | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Ctrl+Arrow handling is consumed in milling controller
sed -n '336,410p' src/odemis/gui/cont/milling.py
# Inspect existing canvas keyboard handler contract referenced in PR discussion
sed -n '1300,1385p' src/odemis/gui/comp/canvas.py | sed -n '/def on_char/,/def /p'
# Find Ctrl/Arrow handling patterns and whether evt.Skip() is used
rg -n -C3 'Ctrl|ControlDown|WXK_LEFT|WXK_RIGHT|evt\.Skip\(' src/odemis/gui/cont/milling.py src/odemis/gui/comp/canvas.pyRepository: delmic/odemis
Length of output: 11042
Ctrl+Arrow handling incorrectly consumes the event, potentially blocking default canvas navigation.
The on_char handler returns at line 395 immediately after moving the milling patterns, without calling evt.Skip(). This prevents the default keyboard handling in the parent BaseCanvas class from executing, which may suppress intended panning or navigation behavior. Please add evt.Skip() before returning if the default navigation should also occur.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/gui/cont/milling.py` around lines 359 - 395, The Ctrl+Arrow branch
in on_char for milling.py is handling pattern movement and then returning
without letting the event continue, which blocks the canvas’s default keyboard
navigation. Update the ctrl_mod/allow_milling_pattern_move path in on_char to
call evt.Skip() before exiting when the BaseCanvas handling should still run, so
the milling move logic and normal panning/navigation can both occur.
It overwrites the default on_char operation of the canvas |
K4rishma
force-pushed
the
move-miilling-boxes-horizontally
branch
from
8f977f1 to
699dbea
Compare
control + shift + arrow keys move short distance and control + arrow keys move large distance
K4rishma
force-pushed
the
move-miilling-boxes-horizontally
branch
from
699dbea to
a59cf32
Compare
control + shift + arrow keys move short distance and control + arrow keys move large distance