[feat] METEOR overlay cryofeature: clarify the current posture in "recalculate position" message box#3379
Conversation
…calculate position" message box In the FIBSEM tab, there are two view: SEM and FIB. If one moves the feature in the FIB view, she/he could beleive that this changes the MILLING posture, while moving the feature in SEM view would adjust the SEM posture. However, that's not the case, it's unrelated to the view, and only linked to the current posture (based on the current stage position). So adjust the dialog to highlight which posture is modified, if not updating all the posture.
There was a problem hiding this comment.
Pull request overview
This PR improves the clarity of the dialog shown when moving cryo features in the FIBSEM tab. The dialog now explicitly indicates which posture (SEM IMAGING, MILLING, FIB IMAGING, etc.) is being modified, addressing user confusion about whether the change affects the view-specific posture or the current posture based on stage position.
Changes:
- Import
POSITION_NAMESdictionary to map posture constants to human-readable names - Update dialog message to specify which posture is being modified
- Customize button labels to show "All" vs "Only [posture name]" options
📝 WalkthroughWalkthroughThis change modifies the user confirmation dialog in the cryo feature overlay's position update workflow. It imports Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/gui/comp/overlay/cryo_feature.py`:
- Around line 254-258: The call to box.SetYesNoLabels(...) may fail; check its
boolean return and handle the False case by updating the dialog text to a
generic yes/no question that matches the default "Yes"/"No" labels.
Specifically, after creating the wx.MessageDialog as box and calling
box.SetYesNoLabels("All", f"Only {current_posture_name}"), test the return value
and if it is False call box.SetMessage(...) (or otherwise set the dialog text)
to a fallback like "Apply this feature position only to the current posture
(Yes) or to all postures (No)?" so the prompt makes sense when the custom labels
couldn’t be applied.
| box = wx.MessageDialog(wx.GetApp().main_frame, | ||
| message="Do you want to recalculate this feature position for all other postures?", | ||
| message=f"Do you want to update this feature position only for {current_posture_name} " | ||
| "or recalculate for all other postures?", | ||
| caption="Recalculate feature positions?", style=wx.YES_NO | wx.ICON_QUESTION | wx.CENTER) | ||
| box.SetYesNoLabels("All", f"Only {current_posture_name}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/odemis/gui/comp/overlay/cryo_feature.py | sed -n '250,265p'Repository: delmic/odemis
Length of output: 1179
🌐 Web query:
wxPython SetYesNoLabels return value documentation
💡 Result:
wx.MessageDialog.SetYesNoLabels(yes, no) returns a bool:
True: the Yes/No button labels were successfully changed.False: labels couldn’t be changed (e.g., not supported on that platform/port). In that case the dialog keeps the standard “Yes”/“No” labels. [1]
Also, changing the labels does not change the result code returned by ShowModal()—it still returns one of wx.ID_OK, wx.ID_CANCEL, wx.ID_YES, or wx.ID_NO (the custom IDs only affect the displayed text). [1]
Sources:
[1] (docs.wxpython.org)
Citations:
SetYesNoLabels return value is unchecked — fallback message missing
SetYesNoLabels may return False to indicate that the labels couldn't be changed; True means they were set successfully. When it fails the buttons will still read "Yes" / "No", but the current message text ("…only for X or recalculate for all other postures?") is not a yes/no question, leaving users with a confusing prompt. The recommended pattern is to check the return value and, in the else branch, set a rephrased message that fits the generic labels.
🛡️ Proposed fix — add a fallback message
- box.SetYesNoLabels("All", f"Only {current_posture_name}")
+ if not box.SetYesNoLabels("All", f"Only {current_posture_name}"):
+ # Custom labels not supported on this platform; rephrase for generic Yes/No
+ box.SetMessage(f"Do you want to recalculate this feature position "
+ f"for all other postures (Yes), or keep the change "
+ f"only for {current_posture_name} (No)?")📝 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.
| box = wx.MessageDialog(wx.GetApp().main_frame, | |
| message="Do you want to recalculate this feature position for all other postures?", | |
| message=f"Do you want to update this feature position only for {current_posture_name} " | |
| "or recalculate for all other postures?", | |
| caption="Recalculate feature positions?", style=wx.YES_NO | wx.ICON_QUESTION | wx.CENTER) | |
| box.SetYesNoLabels("All", f"Only {current_posture_name}") | |
| box = wx.MessageDialog(wx.GetApp().main_frame, | |
| message=f"Do you want to update this feature position only for {current_posture_name} " | |
| "or recalculate for all other postures?", | |
| caption="Recalculate feature positions?", style=wx.YES_NO | wx.ICON_QUESTION | wx.CENTER) | |
| if not box.SetYesNoLabels("All", f"Only {current_posture_name}"): | |
| # Custom labels not supported on this platform; rephrase for generic Yes/No | |
| box.SetMessage(f"Do you want to recalculate this feature position " | |
| f"for all other postures (Yes), or keep the change " | |
| f"only for {current_posture_name} (No)?") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/gui/comp/overlay/cryo_feature.py` around lines 254 - 258, The call
to box.SetYesNoLabels(...) may fail; check its boolean return and handle the
False case by updating the dialog text to a generic yes/no question that matches
the default "Yes"/"No" labels. Specifically, after creating the wx.MessageDialog
as box and calling box.SetYesNoLabels("All", f"Only {current_posture_name}"),
test the return value and if it is False call box.SetMessage(...) (or otherwise
set the dialog text) to a fallback like "Apply this feature position only to the
current posture (Yes) or to all postures (No)?" so the prompt makes sense when
the custom labels couldn’t be applied.
1 participant
In the FIBSEM tab, there are two view: SEM and FIB. If one moves the
feature in the FIB view, she/he could beleive that this changes the
MILLING posture, while moving the feature in SEM view would adjust the
SEM posture. However, that's not the case, it's unrelated to the view,
and only linked to the current posture (based on the current stage position).
So adjust the dialog to highlight which posture is modified, if not
updating all the posture.