[MSD-264][feature] Improve FIBSEM tab UI #3307
Conversation
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🧹 Nitpick comments (2)
src/odemis/gui/comp/file.py (1)
71-71: Update the docstring to reflect the new default value.The docstring still references the old default value
'change..'. Update it to'Change..'to match the new default at line 60.🔎 Apply this diff to update the docstring:
- btn_label (str): The label of the button which opens the file dialog window (default is 'change..'). + btn_label (str): The label of the button which opens the file dialog window (default is 'Change..').src/odemis/gui/comp/milling.py (1)
125-128: Unusedevtparameter in lambda.The lambda captures
evtbut doesn't use it. Consider renaming to_to indicate it's intentionally unused, which also addresses the static analysis warning (ARG005).🔎 Suggested fix:
- value_ctrl.Bind(wx.EVT_KILL_FOCUS, - lambda evt, ctrl=value_ctrl: ctrl.GetEventHandler().ProcessEvent( - wx.CommandEvent(wx.EVT_COMMAND_ENTER.typeId, ctrl.GetId()) - )) + value_ctrl.Bind(wx.EVT_KILL_FOCUS, + lambda _, ctrl=value_ctrl: ctrl.GetEventHandler().ProcessEvent( + wx.CommandEvent(wx.EVT_COMMAND_ENTER.typeId, ctrl.GetId()) + ))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/odemis/acq/feature.py(2 hunks)src/odemis/acq/milling/millmng.py(3 hunks)src/odemis/gui/comp/file.py(1 hunks)src/odemis/gui/comp/milling.py(4 hunks)src/odemis/gui/cont/features.py(1 hunks)src/odemis/gui/cont/milling.py(15 hunks)src/odemis/gui/main_xrc.py(27 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/comp/file.pysrc/odemis/gui/comp/milling.pysrc/odemis/gui/main_xrc.py
📚 Learning: 2025-10-20T10:02:02.606Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:360-379
Timestamp: 2025-10-20T10:02:02.606Z
Learning: In the cryo workflow, POIs (Points of Interest) are linked to features and should always be derived from the latest feature position and focus values when loading correlation data, rather than using any saved POI values from correlation_data.fm_pois. This ensures POIs reflect the current feature state.
Applied to files:
src/odemis/acq/feature.py
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/acq/feature.py
🧬 Code graph analysis (3)
src/odemis/acq/milling/millmng.py (2)
src/odemis/acq/feature.py (1)
CryoFeature(144-241)src/odemis/acq/milling/tasks.py (1)
MillingTaskSettings(69-107)
src/odemis/acq/feature.py (1)
src/odemis/model/_vattributes.py (1)
VigilantAttribute(105-342)
src/odemis/gui/cont/milling.py (2)
src/odemis/gui/comp/overlay/rectangle.py (1)
RectangleOverlay(81-565)src/odemis/acq/move.py (1)
to_sample_stage_from_stage_position(700-715)
🪛 GitHub Actions: Linting
src/odemis/acq/milling/millmng.py
[warning] 352-352: W293: blank line contains whitespace
🪛 Ruff (0.14.8)
src/odemis/gui/comp/milling.py
126-126: Unused lambda argument: evt
(ARG005)
src/odemis/gui/cont/milling.py
354-354: Unused method argument: tasks
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (21)
src/odemis/gui/comp/file.py (1)
60-60: LGTM - Button label capitalization improved.The capitalization change improves UI consistency across the application.
src/odemis/acq/feature.py (1)
162-163: LGTM - Position initialization improved with technical debt noted.The change to initialize
positiondirectly from thestage_positionparameter is clearer than deriving it from the feature name. The FIXME comment appropriately documents that this attribute should eventually use SampleStage coordinates rather than stage-bare coordinates.src/odemis/gui/main_xrc.py (12)
1629-1629: LGTM: Consistent label capitalization.The capitalization of button labels ("Change…" and "Cancel") improves UI consistency across the interface.
Also applies to: 2806-2806, 10224-10224, 10333-10333, 10694-10694, 12204-12204, 12301-12301, 12522-12522, 17984-17984
4765-4766: LGTM: Border and spacing refinements.These spacing adjustments align with the PR objective to make small layout improvements to the wxPython GUI.
Also applies to: 4782-4782, 5145-5145, 9949-9950, 9971-9972
9767-9787: LGTM: Improved button layout and labeling.Wrapping the "Create / Move" and "Go to Feature" buttons in panels with explicit sizing (120×24) provides better layout control and consistent button widths. The label improvements (spacing and capitalization) also enhance readability.
Also applies to: 9812-9832
9760-9760: LGTM: Combo box size adjustments.The explicit sizing of combo boxes improves layout consistency.
Also applies to: 9803-9803
9844-9861: LGTM: Button styling refinements.The explicit sizing and border adjustments for the "SAVE POSITION" button improve visual consistency.
9886-9934: LGTM: Improved mode-switching button layout.The horizontal reorganization of "SEM IMAGING" and "MILLING" buttons with equal expansion (
option=1) creates a cleaner, more balanced layout for the mode-switching interface.
9981-9984: LGTM: Whitespace for readability.
10184-10192: LGTM: Checkbox spacing adjustment.
10249-10279: LGTM: Acquisition button layout improvements.Wrapping acquisition buttons in panels with explicit sizing and centered alignment improves layout consistency across the interface.
Also applies to: 10374-10412
10449-10599: LGTM: Major milling panel restructuring.This restructuring successfully implements the PR objectives:
- Separates the automation controls (MILLING) from pattern controls (PATTERNS) into distinct fold panels
- Renames "RUN AUTOMATED MILLING" button to "MILL" as intended
- Adds comprehensive progress tracking with checklist boxes, progress gauge, estimated/remaining time labels, and cancel button
- Uses proper vertical centering for the progress gauge with spacer elements
The nested panel structure for vertical alignment is appropriate for achieving precise layout control.
10613-10615: LGTM: Vertical spacing adjustment.
11791-11805: LGTM: Consistent button styling across sections.These changes mirror the earlier button improvements (hunks 6 and 8) for a different UI section, ensuring consistent styling with explicit sizing (
minsize=120,24), proper colors, and improved labels. The added comment clarifies layout intent.Also applies to: 11827-11843
src/odemis/gui/comp/milling.py (1)
13-32: LGTM on initialization refactoring.The switch to
super().__init__with a dedicated_panelandGridBagSizeris cleaner. The initialization order (creating panel and sizers before setting task and row state) is logical.src/odemis/acq/milling/millmng.py (2)
358-369: Per-feature gating logic is correct.The skip conditions properly handle:
- Already rough milled features during rough milling
- Already polished features during polishing
- Polished features during rough milling (implies already rough milled)
The chained comparison
status_map[workflow_task] == feature.status.value == FEATURE_Xworks correctly in Python to check if the feature's current status matches both the workflow's target status and the specific constant.
431-444: LGTM on signature change.The updated
_run_milling_taskssignature now acceptsmilling_tasksdirectly instead of deriving them internally. This aligns with the new workflow where tasks are computed per-feature in therunloop and passed explicitly.src/odemis/gui/cont/milling.py (4)
110-110: LGTM - hiding selection points.Setting
show_selection_points=Falsehides the blue control points on rectangle overlays, matching the PR objective to hide pattern selection markers since they aren't individually draggable yet.
319-322: Good optimization to avoid UI flicker.Early exit when tasks are unchanged prevents unnecessary panel rebuilding and visual flicker.
439-446: LGTM - panel visibility toggling.Properly shows/hides pattern panels based on task selection and triggers layout refresh.
732-733: Good guard against None future.The early return when
futureisNoneprevents attribute errors when the dialog is cancelled before starting the milling process.
src/odemis/acq/milling/millmng.py
Outdated
| logging.error(error_text) | ||
| raise ValueError(error_text) | ||
|
|
||
There was a problem hiding this comment.
Fix trailing whitespace.
Pipeline failure indicates this blank line contains whitespace characters (W293).
🔎 Suggested fix:
-
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
🧰 Tools
🪛 GitHub Actions: Linting
[warning] 352-352: W293: blank line contains whitespace
🤖 Prompt for AI Agents
In src/odemis/acq/milling/millmng.py around line 352, there's a blank line
containing trailing whitespace (W293); remove the extra spaces/tabs so the line
is a true empty line (no whitespace) to satisfy linting.
| if not milling_tasks: | ||
| logging.info(f"Skipping {feature.name.value} as it has not tasks to mill.") | ||
| continue |
There was a problem hiding this comment.
Typo in log message.
"has not tasks" should be "has no tasks".
🔎 Suggested fix:
- logging.info(f"Skipping {feature.name.value} as it has not tasks to mill.")
+ logging.info(f"Skipping {feature.name.value} as it has no tasks to mill.")🤖 Prompt for AI Agents
In src/odemis/acq/milling/millmng.py around lines 376 to 378, the log message
contains a typo "has not tasks"; change the logging.info call to use "has no
tasks" (e.g., update the message to "Skipping {feature.name.value} as it has no
tasks to mill.") so the grammar is correct while preserving the same
interpolation and behavior.
| if ans == wx.ID_YES: | ||
| self._tab_data_model.main.features.value.remove(current_feature) | ||
| self._tab_data_model.main.currentFeature.value = None | ||
| self._tab.milling_task_controller.draw_milling_tasks() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the features.py file to verify the review comment claims
cat -n src/odemis/gui/cont/features.py | head -130 | tail -50Repository: delmic/odemis
Length of output: 2842
🏁 Script executed:
#!/bin/bash
# Search for milling_task_controller usage and initialization
rg -n "milling_task_controller" src/odemis/gui/cont/features.py -A 2 -B 2Repository: delmic/odemis
Length of output: 595
🏁 Script executed:
#!/bin/bash
# Look at line 354 to verify the mode check pattern
sed -n '350,360p' src/odemis/gui/cont/features.pyRepository: delmic/odemis
Length of output: 705
🏁 Script executed:
#!/bin/bash
# Search for tab initialization to understand how milling_task_controller is created
fd -t f "tab\.py" src/odemis/gui/cont/ | head -5 | xargs rg -n "milling_task_controller\s*=" -B 3 -A 3Repository: delmic/odemis
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Find all tab implementations to see where milling_task_controller is created
find src/odemis/gui/cont -name "*tab*.py" -type fRepository: delmic/odemis
Length of output: 1225
🏁 Script executed:
#!/bin/bash
# Search for where btn_delete_feature is bound/created
rg -n "btn_delete_feature" src/odemis/gui/cont/ -B 2 -A 2Repository: delmic/odemis
Length of output: 1372
🏁 Script executed:
#!/bin/bash
# Look for where milling_task_controller is instantiated
rg -n "milling_task_controller\s*=" src/odemis/gui -B 3 -A 3Repository: delmic/odemis
Length of output: 614
🏁 Script executed:
#!/bin/bash
# Check the initialization of the controller class to see if delete button access is mode-specific
sed -n '70,90p' src/odemis/gui/cont/features.pyRepository: delmic/odemis
Length of output: 1227
Add mode check before accessing milling_task_controller in feature deletion.
Line 107 calls self._tab.milling_task_controller.draw_milling_tasks() without verifying the acquisition mode. Since the delete button is enabled in both FLM and FIBSEM modes, but milling_task_controller only exists on FIBSEM tabs, this will cause an AttributeError when deleting features in FLM mode. Line 354 guards similar controller access with a mode check—apply the same pattern here:
if ans == wx.ID_YES:
self._tab_data_model.main.features.value.remove(current_feature)
self._tab_data_model.main.currentFeature.value = None
- self._tab.milling_task_controller.draw_milling_tasks()
+ if self.acqui_mode is guimod.AcquiMode.FIBSEM:
+ self._tab.milling_task_controller.draw_milling_tasks()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._tab.milling_task_controller.draw_milling_tasks() | |
| if self.acqui_mode is guimod.AcquiMode.FIBSEM: | |
| self._tab.milling_task_controller.draw_milling_tasks() |
🤖 Prompt for AI Agents
In src/odemis/gui/cont/features.py around line 107, the code unconditionally
calls self._tab.milling_task_controller.draw_milling_tasks() which raises
AttributeError in FLM mode because milling_task_controller exists only on FIBSEM
tabs; add the same guard used at line 354 (check the tab mode is FIBSEM and/or
that milling_task_controller is present) and only call draw_milling_tasks when
in FIBSEM mode (or when hasattr(self._tab, "milling_task_controller") and it is
not None) to avoid accessing the attribute in FLM mode.
| def _on_saving_position(self, tasks): | ||
| pos = self.pm.to_sample_stage_from_stage_position(self._tab_data.main.currentFeature.value.position.value, MILLING) | ||
| self.draw_milling_tasks((pos['x'], pos['y'])) |
There was a problem hiding this comment.
Unused tasks parameter and potential None dereference.
- The
tasksparameter is unused (ARG002 from static analysis). Consider renaming to_if intentionally ignored. self._tab_data.main.currentFeature.valuecould beNone, which would cause anAttributeErrorwhen accessing.position.value.
🔎 Suggested fix:
- def _on_saving_position(self, tasks):
- pos = self.pm.to_sample_stage_from_stage_position(self._tab_data.main.currentFeature.value.position.value, MILLING)
- self.draw_milling_tasks((pos['x'], pos['y']))
+ def _on_saving_position(self, _):
+ current_feature = self._tab_data.main.currentFeature.value
+ if current_feature is None:
+ return
+ pos = self.pm.to_sample_stage_from_stage_position(current_feature.position.value, MILLING)
+ self.draw_milling_tasks((pos['x'], pos['y']))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _on_saving_position(self, tasks): | |
| pos = self.pm.to_sample_stage_from_stage_position(self._tab_data.main.currentFeature.value.position.value, MILLING) | |
| self.draw_milling_tasks((pos['x'], pos['y'])) | |
| def _on_saving_position(self, _): | |
| current_feature = self._tab_data.main.currentFeature.value | |
| if current_feature is None: | |
| return | |
| pos = self.pm.to_sample_stage_from_stage_position(current_feature.position.value, MILLING) | |
| self.draw_milling_tasks((pos['x'], pos['y'])) |
🧰 Tools
🪛 Ruff (0.14.8)
354-354: Unused method argument: tasks
(ARG002)
🤖 Prompt for AI Agents
In src/odemis/gui/cont/milling.py around lines 354 to 356, the parameter `tasks`
is unused and `self._tab_data.main.currentFeature.value` may be None leading to
AttributeError; rename the unused parameter to `_` (or `_tasks`) and add a
None-check chain before accessing `.position.value` (e.g., get currentFeature =
self._tab_data.main.currentFeature.value and if not currentFeature: return;
similarly check currentFeature.position/value), then only call
self.pm.to_sample_stage_from_stage_position and self.draw_milling_tasks when the
position and converted pos exist.
ilyushkin
force-pushed
the
msd-264-improve-fibsem-tab-ui
branch
from
4a9d0f3 to
fbf5665
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/odemis/acq/milling/millmng.py (2)
352-352: Fix trailing whitespace.The pipeline failure indicates this blank line contains whitespace characters (W293).
🔎 Apply this diff to remove the trailing whitespace:
- +
371-378: Typo in log message.Line 377: "has not tasks" should be "has no tasks".
🔎 Apply this diff to fix the typo:
- logging.info(f"Skipping {feature.name.value} as it has not tasks to mill.") + logging.info(f"Skipping {feature.name.value} as it has no tasks to mill.")
Approve the task association logic.
The implementation correctly computes per-feature milling tasks using
get_associated_tasksand skips features with no associated tasks, aligning with the PR's updated milling task handling.src/odemis/gui/cont/milling.py (1)
354-356: Add None-check to prevent AttributeError.The
tasksparameter is unused, andself._tab_data.main.currentFeature.valuecould beNone, which would cause anAttributeErrorwhen accessing.position.value.🔎 Apply this diff to fix the issue:
- def _on_saving_position(self, tasks): - pos = self.pm.to_sample_stage_from_stage_position(self._tab_data.main.currentFeature.value.position.value, MILLING) - self.draw_milling_tasks((pos['x'], pos['y'])) + def _on_saving_position(self, _): + current_feature = self._tab_data.main.currentFeature.value + if current_feature is None: + return + pos = self.pm.to_sample_stage_from_stage_position(current_feature.position.value, MILLING) + self.draw_milling_tasks((pos['x'], pos['y']))
🧹 Nitpick comments (1)
src/odemis/gui/main_xrc.py (1)
9767-9787: Consider DPI-aware sizing for button panels.The buttons are wrapped in panels with hardcoded pixel sizes (e.g.,
120,24,200,48). This may not scale properly on high-DPI displays or when users have custom font sizes.Also note that similar buttons use different sizing approaches:
- Lines 9768 & 9813: Use
<size>120,24</size>- Lines 11799 & 11837: Use
<minsize>120,24</minsize>Consider using
minsizeconsistently (which allows growth) or implementing DPI-aware sizing.Also applies to: 9812-9832, 10249-10279, 10374-10413
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/odemis/acq/feature.py(1 hunks)src/odemis/acq/milling/millmng.py(3 hunks)src/odemis/gui/comp/file.py(1 hunks)src/odemis/gui/comp/milling.py(4 hunks)src/odemis/gui/cont/features.py(1 hunks)src/odemis/gui/cont/milling.py(15 hunks)src/odemis/gui/main_xrc.py(27 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/odemis/gui/cont/features.py
- src/odemis/acq/feature.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/gui/main_xrc.pysrc/odemis/gui/comp/milling.pysrc/odemis/acq/milling/millmng.py
🧬 Code graph analysis (2)
src/odemis/acq/milling/millmng.py (2)
src/odemis/acq/feature.py (1)
CryoFeature(144-241)src/odemis/acq/milling/tasks.py (1)
MillingTaskSettings(69-107)
src/odemis/gui/cont/milling.py (2)
src/odemis/gui/comp/overlay/rectangle.py (1)
RectangleOverlay(81-565)src/odemis/acq/move.py (1)
to_sample_stage_from_stage_position(700-715)
🪛 GitHub Actions: Linting
src/odemis/acq/milling/millmng.py
[error] 352-352: flake8: W293 blank line contains whitespace.
🪛 Ruff (0.14.8)
src/odemis/gui/comp/milling.py
126-126: Unused lambda argument: evt
(ARG005)
src/odemis/gui/cont/milling.py
354-354: Unused method argument: tasks
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (29)
src/odemis/gui/comp/file.py (1)
60-60: LGTM!The capitalization change aligns with the PR's UI polish objectives and maintains consistency with other button labels across the application.
src/odemis/gui/comp/milling.py (7)
13-13: LGTM!Using
super().__init__is the modern Python idiom and improves maintainability.
21-29: LGTM!The panel reorganization provides better separation between the container and content, improving layout control and maintainability.
35-39: LGTM!The header styling improvements (bold weight and increased font size) enhance visual hierarchy and user experience.
47-52: LGTM!The label consistency improvements (sentence case) and addition of the "spacing" configuration entry enhance UI consistency and expand configuration options.
57-58: LGTM!Adding "rate" and "dwell_time" to
unsupported_parameterscorrectly implements the PR objective to hide parameters that are managed centrally in milling.config.
105-110: LGTM!The explicit minimum size constraints improve layout consistency and ensure controls are properly sized for readability.
125-128: LGTM!The kill-focus binding ensures value updates are processed when the user tabs away without pressing Enter, improving UX. The unused
evtparameter is required by the wxPython event binding signature—this is a standard pattern in event handlers.src/odemis/acq/milling/millmng.py (3)
358-369: LGTM!The feature status gating logic correctly implements idempotency by skipping features that:
- Are not ready for milling (ACTIVE)
- Have already completed the current workflow task (ROUGH_MILLED/POLISHED matching the task)
- Are beyond the current workflow task (POLISHED when doing RoughMilling)
This prevents redundant milling operations and aligns with the PR objective to skip features based on their milling status.
396-396: LGTM!Passing the computed
milling_tasksto_run_milling_taskscorrectly implements the updated architecture where milling tasks are externally provisioned rather than derived internally.
431-432: LGTM!The signature change to accept
milling_tasksas a parameter correctly implements the architectural shift to external task provisioning. Since this is a private method, the breaking change is contained within the class.src/odemis/gui/main_xrc.py (8)
1629-1629: LGTM! Consistent button label capitalization.The capitalization of "Cancel" and "Change…" button labels across all instances improves UI consistency and professionalism.
Also applies to: 2806-2806, 10224-10224, 10333-10333, 10694-10694, 12204-12204, 12301-12301, 12522-12522, 17984-17984
4765-4766: LGTM! Layout spacing adjustments.The border and flag adjustments use standard wxPython values (5px, 10px) and improve visual consistency.
Also applies to: 4782-4782, 5145-5145, 9949-9950, 9971-9972, 10191-10192
9753-9754: LGTM! Color and sizing adjustments for consistency.The color additions maintain the dark theme (#333333 background, #FFFFFF text), and size adjustments improve visual alignment.
Also applies to: 9760-9760, 9803-9803, 9844-9862
10449-10590: LGTM! Comprehensive automation UI restructuring.The restructuring successfully implements the PR objectives:
- Renames "RUN AUTOMATED MILLING" to "MILL" (line 10486)
- Adds progress tracking with gauge, time estimates, and status labels
- Separates features and tasks into distinct checklists
The layout hierarchy is well-organized with proper expansion flags.
10514-10531: Verify gauge visibility and time label width.Two potential UI concerns:
Gauge height: The progress gauge height is only 8px (lines 10515, 10521), which may be too thin to see clearly, especially on high-DPI displays.
Time label width: The time remaining label panel is constrained to 80px wide (line 10538). Depending on the time format (e.g., "12h 34m 56s"), this might truncate longer time strings.
Please verify these dimensions render acceptably across different display configurations.
Also applies to: 10537-10553
9886-9950: LGTM! Radio button layout improvements.The restructuring provides better control over button spacing with consistent 10px borders and proper expansion flags. The changes improve visual consistency between the SEM IMAGING and MILLING mode buttons.
9981-9984: LGTM! Whitespace and spacing adjustments.The empty lines improve XML readability, the label text maintains proper spacing, and the 10px vertical spacer improves visual separation between UI elements.
Also applies to: 10184-10184, 10613-10615
11794-11803: LGTM! Button layout with minsize approach.The use of
minsizeinstead of fixedsizeallows these buttons to grow if needed, which is better for flexibility. The dark theme colors and center-vertical alignment are appropriate.Note: These buttons use
border=52while similar buttons at lines 9785 and 9830 useborder=82. This difference may be intentional for different layout contexts.Also applies to: 11830-11843
src/odemis/gui/cont/milling.py (10)
49-51: LGTM!The import of
MILLINGis correctly added and properly used in the_on_saving_positionmethod for coordinate transformation.
110-110: LGTM!Hiding selection points is appropriate since they're not individually draggable yet, as mentioned in the PR objectives.
181-182: LGTM!Renaming from
_create_panelsto_update_panelsaccurately reflects the method's purpose, as it's called dynamically when milling tasks change.
185-191: LGTM!Hiding automated milling UI elements by default is appropriate; they're properly shown/hidden during the milling workflow lifecycle.
212-277: LGTM!The
_update_panelsmethod correctly rebuilds the pattern panels dynamically. Settingproportion=0is appropriate for fixed-size panels, and the VigilantAttributeConnector setup properly links UI controls to the underlying data model.
318-349: LGTM!The early return optimization prevents unnecessary panel flickering, and the explicit scrolled window layout recalculation (lines 344-349) is a good workaround for wxPython's deferred sizing behavior.
435-447: LGTM!Dynamically showing/hiding pattern panels based on task selection improves the UI experience by reducing clutter.
521-521: LGTM!Synchronizing the automated milling estimate text label with the standard milling estimate ensures consistent status messaging across the UI.
Also applies to: 555-555, 560-560, 565-565
676-682: LGTM!The UI state transitions properly hide estimate text and show progress elements when automated milling starts. The TODO comment at line 661 notes that estimated time calculation is a known enhancement.
732-733: LGTM!Adding a guard against
Nonefuture is essential, as_on_automation_done(None)is explicitly called when the user cancels the confirmation dialog (line 673).
ilyushkin
force-pushed
the
msd-264-improve-fibsem-tab-ui
branch
from
fbf5665 to
c122958
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/odemis/acq/milling/millmng.py (1)
376-378: Typo in log message."has not tasks" should be "has no tasks".
🔎 Apply this diff:
- logging.info(f"Skipping {feature.name.value} as it has not tasks to mill.") + logging.info(f"Skipping {feature.name.value} as it has no tasks to mill.")src/odemis/gui/cont/milling.py (1)
354-356: Unused parameter and potential None dereference.
- The
tasksparameter is unused. Rename to_to indicate it's intentionally ignored.self._tab_data.main.currentFeature.valuecould beNone, which would cause anAttributeErrorwhen accessing.position.value.🔎 Apply this diff:
- def _on_saving_position(self, tasks): - pos = self.pm.to_sample_stage_from_stage_position(self._tab_data.main.currentFeature.value.position.value, MILLING) - self.draw_milling_tasks((pos['x'], pos['y'])) + def _on_saving_position(self, _): + current_feature = self._tab_data.main.currentFeature.value + if current_feature is None: + return + pos = self.pm.to_sample_stage_from_stage_position(current_feature.position.value, MILLING) + self.draw_milling_tasks((pos['x'], pos['y']))
🧹 Nitpick comments (2)
src/odemis/gui/main_xrc.py (2)
10514-10531: Consider flexible sizing for better DPI and i18n support.Multiple panels have hardcoded sizes (200,8 for gauge panel, 80,24 for time label panel). While this may work for the target resolution, it could cause layout issues with:
- High DPI displays
- Custom font size settings
- Internationalized text (e.g., longer translations)
Consider using minimum sizes with expand flags where possible instead of fixed sizes.
Also applies to: 10537-10550
9767-9787: Large border values may indicate alignment workaround.The "Create / Move" and "Go to Feature" buttons are wrapped in panels with a large left border of 82 pixels. While this achieves alignment, it's a brittle approach that may break with different screen configurations or theme changes.
Consider using wxGridBagSizer or wxFlexGridSizer positioning instead of large border values for more maintainable alignment.
Also applies to: 9812-9832
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/odemis/acq/feature.py(1 hunks)src/odemis/acq/milling/millmng.py(3 hunks)src/odemis/gui/comp/file.py(1 hunks)src/odemis/gui/comp/milling.py(4 hunks)src/odemis/gui/cont/features.py(1 hunks)src/odemis/gui/cont/milling.py(15 hunks)src/odemis/gui/main_xrc.py(27 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/odemis/gui/comp/file.py
- src/odemis/gui/cont/features.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/acq/feature.py
📚 Learning: 2025-10-20T10:00:09.185Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11982-11982
Timestamp: 2025-10-20T10:00:09.185Z
Learning: In `src/odemis/gui/main_xrc.py`, extra spaces in wxStaticText labels (such as "POI size " and "Use Current Z ") are intentionally added for GUI alignment purposes and should not be flagged as formatting issues.
Applied to files:
src/odemis/acq/milling/millmng.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/comp/milling.py
🧬 Code graph analysis (3)
src/odemis/acq/feature.py (1)
src/odemis/model/_vattributes.py (1)
VigilantAttribute(105-342)
src/odemis/acq/milling/millmng.py (2)
src/odemis/acq/feature.py (1)
CryoFeature(144-241)src/odemis/acq/milling/tasks.py (1)
MillingTaskSettings(69-107)
src/odemis/gui/cont/milling.py (3)
src/odemis/gui/comp/overlay/rectangle.py (1)
RectangleOverlay(81-565)src/odemis/gui/comp/milling.py (1)
MillingTaskPanel(9-183)src/odemis/acq/move.py (1)
to_sample_stage_from_stage_position(700-715)
🪛 Ruff (0.14.8)
src/odemis/gui/cont/milling.py
354-354: Unused method argument: tasks
(ARG002)
src/odemis/gui/comp/milling.py
126-126: Unused lambda argument: evt
(ARG005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (22)
src/odemis/gui/main_xrc.py (3)
1629-1629: LGTM! Proper use of ellipsis for action buttons.The "Change…" labels correctly use the ellipsis character to indicate that these buttons will open a dialog or file chooser, which follows standard UI conventions.
Also applies to: 10224-10224, 12204-12204, 17984-17984
2806-2806: LGTM! Cancel button capitalization aligned with PR objectives.All Cancel button labels have been properly capitalized, which is consistent with the PR objectives and improves UI consistency.
Also applies to: 10333-10333, 10559-10559, 10694-10694, 12301-12301, 12522-12522
10449-10590: LGTM! Major MILLING section restructuring aligns with PR objectives.The restructuring successfully:
- Renames "RUN AUTOMATED MILLING" button to "MILL" (line 10486)
- Adds comprehensive automated milling UI elements (checklists, progress gauge, time estimates, cancel button)
- Separates MILLING and PATTERNS sections for better organization
- Maintains proper control assignments with XRCED tags
This layout enhances the user experience for automated milling workflows.
src/odemis/gui/comp/milling.py (5)
13-32: Panel initialization refactoring looks good.The new initialization pattern with
self._paneland deferrednum_rows/taskassignment improves clarity and ensures the panel hierarchy is established before use.
35-39: Header styling enhancement is appropriate.Making the task name bold and slightly larger improves the visual hierarchy of the milling task panel.
47-48: Verify label capitalization is intentional.The labels were changed to lowercase ("Align at milling current" and "Milling mode"). Ensure this matches the project's UI style guide, as most GUI labels typically use title case for consistency.
55-58: Correctly hides unsupported parameters.Adding
rateanddwell_timeto the unsupported parameters list aligns with the PR objective to hide parameters managed centrally in milling.config.
105-110: Explicit minimum sizes improve layout consistency.Setting minimum sizes for controls ensures predictable UI behavior across different milling task configurations.
src/odemis/acq/milling/millmng.py (3)
358-369: Feature gating logic is well-structured.The per-feature skip conditions correctly handle:
- Features not ready for milling (ACTIVE)
- Features already at the target status for the current workflow task
- Features that have progressed beyond the current workflow task (e.g., already polished during rough milling)
This prevents redundant milling operations.
396-396: External milling tasks provisioning improves design.Passing
milling_taskscomputed in the outer loop to_run_milling_tasksseparates concern of task selection from execution, making the code more maintainable.
431-432: Signature change is consistent.The updated signature correctly reflects that
_run_milling_tasksnow consumes externally provided tasks rather than deriving them internally.src/odemis/gui/cont/milling.py (11)
49-51: Import addition is necessary.The
MILLINGimport is correctly added to support position conversion in_on_saving_position(line 355).
110-110: Correctly hides selection points.Setting
show_selection_points=Falsealigns with the PR objective to hide pattern selection markers until individual dragging is supported.
177-191: Initialization changes improve UI state management.Replacing the initial panel creation with
_update_panelsenables dynamic panel rebuilding, and hiding automated widgets by default ensures a clean initial state.
212-276: Dynamic panel management is well-implemented.The
_update_panelsmethod correctly:
- Cleans up existing panels
- Rebuilds the panel hierarchy
- Sets
proportion=0to prevent unnecessary vertical expansion- Establishes VA connectors and subscriptions
319-322: Early exit optimization prevents unnecessary updates.Checking if tasks are unchanged before updating prevents UI flickering and unnecessary recalculations.
327-349: Subscription management and layout forcing are correct.The unsubscribe/resubscribe pattern ensures clean handler management, and forcing the scrolled parent to recalculate (
FitInside()andSendSizeEvent()) prevents stale layout issues.
402-402: Spelling normalization is appropriate.Changing "image centre" to "image center" (American spelling) improves consistency across the codebase.
439-446: Panel visibility management is correct.Showing/hiding pattern panels based on task selection and triggering layout updates ensures the UI reflects the current state.
521-521: Automated milling UI synchronization is correct.The changes ensure automated milling buttons and estimate labels stay synchronized with the regular milling UI state, providing consistent user feedback.
Also applies to: 555-555, 560-560, 565-565
676-682: UI state transitions for automated milling are correct.The widget visibility changes and flag settings properly manage the UI state when starting automated milling.
732-733: None check prevents errors.The guard against
Nonefutures correctly handles the case where_on_automation_doneis called when the user cancels at the confirmation dialog (line 673).
| # FIXME: The 'position' parameter should eventually contain the SampleStage coordinates and not stage bare from the stage_position! | ||
| self.position = model.VigilantAttribute(stage_position, unit="m") # sample stage aka "ideal stage", with x, y, z axes |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read src/odemis/acq/feature.py around lines 162-163 to see the actual code
echo "=== Reading src/odemis/acq/feature.py lines 150-180 ==="
cat -n src/odemis/acq/feature.py | sed -n '150,180p'Repository: delmic/odemis
Length of output: 2307
🏁 Script executed:
#!/bin/bash
# Also check the features.py file mentioned in the learning
echo "=== Reading src/odemis/gui/cont/features.py lines 360-380 ==="
if [ -f "src/odemis/gui/cont/features.py" ]; then
cat -n src/odemis/gui/cont/features.py | sed -n '360,380p'
else
echo "File not found at expected path"
find . -name "features.py" -type f | head -5
fiRepository: delmic/odemis
Length of output: 1262
🏁 Script executed:
#!/bin/bash
# Check usage of self.position in feature.py to understand current behavior
echo "=== Checking usage of self.position in CryoFeature class ==="
grep -n "self\.position" src/odemis/acq/feature.py | head -20Repository: delmic/odemis
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Search for documentation or comments about stage_position parameter
echo "=== Searching for stage_position parameter definition ==="
grep -B5 -A5 "def __init__" src/odemis/acq/feature.py | head -40Repository: delmic/odemis
Length of output: 1806
🏁 Script executed:
#!/bin/bash
# Search for usage of .position in the codebase (not just initialization)
echo "=== Searching for usage of .position throughout the codebase ==="
rg -n "\.position\b" --type=py src/ | head -40Repository: delmic/odemis
Length of output: 4048
🏁 Script executed:
#!/bin/bash
# Look for CryoFeature instantiation to see what's passed as stage_position
echo "=== Searching for CryoFeature instantiation ==="
rg -n "CryoFeature(" --type=py src/ -A 3 | head -50Repository: delmic/odemis
Length of output: 196
🏁 Script executed:
#!/bin/bash
# Check the actual Target class and how it uses coordinates
echo "=== Searching for Target class definition and usage ==="
rg -n "class Target" --type=py src/ -A 20 | head -60Repository: delmic/odemis
Length of output: 4197
🏁 Script executed:
#!/bin/bash
# Search for CryoFeature usage more carefully
echo "=== Searching for CryoFeature instances (avoiding regex issues) ==="
rg -n "CryoFeature" --type=py src/ | head -30Repository: delmic/odemis
Length of output: 3570
🏁 Script executed:
#!/bin/bash
# Search specifically for feature.position usage (not other .position)
echo "=== Searching for feature.position or currentFeature.position ==="
rg -n "feature\.position|currentFeature\.position" --type=py src/Repository: delmic/odemis
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Check features.py to understand how Target objects are created and what coords they use
echo "=== Searching for Target instantiation in features.py ==="
rg -n "Target\(" --type=py src/odemis/gui/cont/features.py -B 2 -A 2 | head -60Repository: delmic/odemis
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Check the exact context of CryoFeature instantiation in tab_gui_data.py
echo "=== Context around CryoFeature instantiation at line 289 ==="
cat -n src/odemis/gui/model/tab_gui_data.py | sed -n '280,320p'Repository: delmic/odemis
Length of output: 2425
🏁 Script executed:
#!/bin/bash
# Check the TODO comment about position at line 264 in overlay/cryo_feature.py
echo "=== Context around line 264 in cryo_feature.py ==="
cat -n src/odemis/gui/comp/overlay/cryo_feature.py | sed -n '260,320p'Repository: delmic/odemis
Length of output: 3662
🏁 Script executed:
#!/bin/bash
# Search for Target instantiation anywhere in the codebase
echo "=== Searching for Target class instantiation ==="
rg -n "Target\(" --type=py src/ -A 1 | head -80Repository: delmic/odemis
Length of output: 3258
🏁 Script executed:
#!/bin/bash
# Check the Target instantiation context more carefully - see what coordinates are passed
echo "=== Detailed context of Target instantiation in tab_gui_data.py around line 532 ==="
cat -n src/odemis/gui/model/tab_gui_data.py | sed -n '520,560p'Repository: delmic/odemis
Length of output: 2416
🏁 Script executed:
#!/bin/bash
# Check multi_point_correlation.py Target instantiation
echo "=== Target instantiation in multi_point_correlation.py around line 476 ==="
cat -n src/odemis/gui/cont/multi_point_correlation.py | sed -n '470,495p'Repository: delmic/odemis
Length of output: 1971
🏁 Script executed:
#!/bin/bash
# Get the function get_feature_position_at_posture definition
echo "=== get_feature_position_at_posture function definition ==="
rg -n "def get_feature_position_at_posture" --type=py src/ -A 20Repository: delmic/odemis
Length of output: 1943
🏁 Script executed:
#!/bin/bash
# Check if there's any direct read of .position attribute on CryoFeature
echo "=== Checking for direct reads/assignments of .position ==="
rg -n "feature\.position|\.position\.value|\.position\[" --type=py src/ | grep -E "(acq|gui)" | head -20Repository: delmic/odemis
Length of output: 2066
Remove misleading comment or clarify attribute redundancy.
Line 163's inline comment claims self.position contains "sample stage aka 'ideal stage'" coordinates, but this contradicts the FIXME on line 162 and the actual implementation. The attribute is initialized with stage_position (stage-bare coordinates), while line 164 shows self.stage_position receives the same value with correct documentation.
Additionally, self.position appears unused throughout the codebase; all coordinate transformations rely on self.stage_position with explicit conversion via pm.to_posture() and pm.to_sample_stage_from_stage_position(). The redundant attribute and misleading comment create unnecessary confusion.
Recommendation: Either remove the self.position attribute entirely (since self.stage_position serves the same purpose and is actually used), or update the comment on line 163 to accurately reflect that it currently contains stage-bare coordinates pending future refactoring (as noted in the FIXME).
🤖 Prompt for AI Agents
In src/odemis/acq/feature.py around lines 162 to 163, the inline comment and the
self.position attribute are misleading because self.position is initialized with
stage_position (stage-bare coordinates) while the code uses self.stage_position
for all transformations; either remove the redundant self.position assignment
entirely (and its misleading comment) or, if you want to keep it for future
refactor, update the comment to state explicitly that self.position currently
holds stage-bare coordinates pending conversion and ensure it's not used
anywhere else until refactored.
| value_ctrl.Bind(wx.EVT_KILL_FOCUS, | ||
| lambda evt, ctrl=value_ctrl: ctrl.GetEventHandler().ProcessEvent( | ||
| wx.CommandEvent(wx.EVT_COMMAND_ENTER.typeId, ctrl.GetId()) | ||
| )) |
There was a problem hiding this comment.
Unused lambda parameter.
The evt parameter in the lambda is unused. Rename it to _ to indicate it's intentionally ignored.
🔎 Apply this diff:
- value_ctrl.Bind(wx.EVT_KILL_FOCUS,
- lambda evt, ctrl=value_ctrl: ctrl.GetEventHandler().ProcessEvent(
- wx.CommandEvent(wx.EVT_COMMAND_ENTER.typeId, ctrl.GetId())
- ))
+ value_ctrl.Bind(wx.EVT_KILL_FOCUS,
+ lambda _, ctrl=value_ctrl: ctrl.GetEventHandler().ProcessEvent(
+ wx.CommandEvent(wx.EVT_COMMAND_ENTER.typeId, ctrl.GetId())
+ ))🧰 Tools
🪛 Ruff (0.14.8)
126-126: Unused lambda argument: evt
(ARG005)
🤖 Prompt for AI Agents
In src/odemis/gui/comp/milling.py around lines 125 to 128, the lambda bound to
wx.EVT_KILL_FOCUS declares an unused parameter named evt; rename that parameter
to _ to indicate it is intentionally ignored and update the lambda signature
accordingly so the binding remains the same but the unused parameter is clearly
marked.
| </object> | ||
| <object class="sizeritem"> | ||
| <object class="ImageTextButton" name="btn_run_automated_milling"> | ||
| <icon>______img_icon_ico_sem_png</icon> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for milling icon references
rg -n "ico_milling" --type=py
# Search for all icon references containing "milling"
rg -n "milling.*icon|icon.*milling" --type=py
# Search for the MILL button to see if icon is set programmatically elsewhere
rg -n "btn_run_automated_milling" --type=py -A 5 -B 5Repository: delmic/odemis
Length of output: 8826
MILL button uses incorrect icon.
At line 10483, the btn_run_automated_milling button uses ______img_icon_ico_sem_png (SEM icon). Since milling-specific icons exist in the codebase (e.g., ______img_icon_ico_milling_png), the MILL button should use the milling icon for consistency with other milling button controls in the file (lines 4834 and 9912).
🤖 Prompt for AI Agents
In src/odemis/gui/main_xrc.py around line 10483, the MILL button's icon is set
to the SEM icon (______img_icon_ico_sem_png); replace that value with the
milling icon constant (______img_icon_ico_milling_png) so the
btn_run_automated_milling uses the correct milling icon, matching other milling
controls at lines ~4834 and ~9912; update the <icon> entry accordingly and save.
| if isinstance(val, model.FloatContinuous): | ||
| value_ctrl = UnitFloatCtrl(self._panel, value=val.value, | ||
| style=wx.NO_BORDER, **conf) | ||
| value_ctrl.Bind(wx.EVT_KILL_FOCUS, |
There was a problem hiding this comment.
This looks like quite a hack. Please add a comment above to indicate why this is necessary, and how it works.
If I understand correctly, that's to force the typed value to be processed on loosing the focus. I wonder why not doing it for all the types of inputs? Is that something special with the UnitFloatCtrl? This class (parent) already has a on_kill_focus method to precisely do this. What is special here that prevents it from working? I've tested it without the fix, and it seemed to behave fine... so maybe I don't really understand what the issue is.
There was a problem hiding this comment.
You cannot just modify the main_xrc.py file. This is an auto-generated code. Instead, you have modify the odemis/gui/xmlh/resources/panel_tab_fibsem.xrc file.
2 participants
milling.config.