[MSD-293][feature] Refine Z button available in super Z workflow#3377
[MSD-293][feature] Refine Z button available in super Z workflow#3377K4rishma wants to merge 1 commit intodelmic:masterfrom
Conversation
Refine Z button in the multi_point_correlation.py will also be available for Super Z GUI. The disabling of Refine Z button depends if the Locate Z button in the Localization button is used or not. So in the same GUI, depending on the feature's usage of Super Z loacalization, the Refine Z button can be enabled or disabled.
📝 WalkthroughWalkthroughThis change reorganizes when Super Z stream information is assigned to features during cryo-localization and enhances the correlation UI with status indicators. The automatic stream-to-feature propagation on stream selection is removed from the selection handlers and deferred to when localization measurement begins. Additionally, new UI controls are introduced to display "Refine Z Active" and "Super Z Info" status in the correlation dialog, with the Z-targeting button remaining visible when Super Z is available, and status text dynamically updated during targeting operations. 🚥 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 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
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/cont/multi_point_correlation.py (1)
807-814:⚠️ Potential issue | 🟡 Minor
wx.CallLatertimer is never canceled — potential crash on dialog close and timer accumulation.Two related issues:
Widget destruction: The
wx.CallLaterreturn value is discarded. If the user closes the correlation dialog within 1 second of triggering Refine Z (e.g. while z-targeting just ran),stop()proceeds with frame teardown, but the pending callback will still fire after 1 second — callingSetLabelon an already-destroyedwx.StaticTextC++ object →RuntimeError.Timer accumulation: With MIP enabled,
_on_z_targeting(None)is called on every target selection via_on_current_target_changes. Each call schedules a newCallLaterwithout canceling the previous one. Multiple timers stack up and all fire independently.Fix: store the timer as an instance variable and cancel it in
stop()and before scheduling a new one.🔧 Proposed fix
In
__init__, after the other instance-variable declarations:+ self._refinez_label_timer: Optional[wx.CallLater] = NoneIn
_on_z_targeting:if self._tab_data_model.main.currentTarget.value: - self.txt_refinez_active.SetLabel("active ...") - wx.CallLater(1000, self.txt_refinez_active.SetLabel, "") + if self._refinez_label_timer and self._refinez_label_timer.IsRunning(): + self._refinez_label_timer.Stop() + self.txt_refinez_active.SetLabel("active ...") + self._refinez_label_timer = wx.CallLater(1000, self.txt_refinez_active.SetLabel, "")In
stop():def stop(self): """Gracefully stop the worker thread.""" + if self._refinez_label_timer and self._refinez_label_timer.IsRunning(): + self._refinez_label_timer.Stop() self.change_queue.put(None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/multi_point_correlation.py` around lines 807 - 814, The CallLater timers scheduled in _on_z_targeting are never tracked or cancelled which can call txt_refinez_active.SetLabel after the widget is destroyed and allows timers to accumulate; fix this by adding an instance variable (e.g. self._refinez_timer initialized in __init__), canceling any existing timer before scheduling a new one inside _on_z_targeting (call Stop()/Cancel on the previous CallLater and set/replace self._refinez_timer with the new wx.CallLater return value), and ensure stop() cancels and clears self._refinez_timer so no pending callback runs after teardown.
🧹 Nitpick comments (2)
src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc (1)
109-117: Inconsistent indentation intxt_superz_infosizeritem block.The opening
<object class="sizeritem">(line 109) and inner tags are not indented to match the surrounding XML hierarchy, unlike thetxt_refinez_activeblock above (lines 95–103).🔧 Proposed indentation fix
+ <object class="sizeritem"> + <object class="wxStaticText" name="txt_superz_info"> + <label>SuperZ Info : NA</label> + <fg>#E5E5E5</fg> + <hidden>1</hidden> + </object> + <flag>wxALL</flag> + <border>10</border> + </object> - <object class="sizeritem"> - <object class="wxStaticText" name="txt_superz_info"> - <label>SuperZ Info : NA</label> - <fg>#E5E5E5</fg> - <hidden>1</hidden> - </object> - <flag>wxALL</flag> - <border>10</border> - </object>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc` around lines 109 - 117, The XML block for the sizer item containing the wxStaticText named txt_superz_info has inconsistent indentation; re-indent the opening <object class="sizeritem"> and all its child tags (including the <object class="wxStaticText" name="txt_superz_info">, <label>, <fg>, <hidden>, <flag>, and <border>) to match the surrounding sizeritem blocks (e.g., mirror the indentation style used by txt_refinez_active) so the hierarchy is visually aligned and consistent.src/odemis/gui/main_xrc.py (1)
2035-2043: Inconsistent XML indentation intxt_superz_infoblock.The child
<object class="wxStaticText">on line 2036 starts at the same indentation level as its parent<object class="sizeritem">on line 2035, and the closing</object>on line 2043 doesn't align with its opening tag. Compare with the well-formattedtxt_refinez_activeblock above (lines 2021–2029).🔧 Suggested indentation fix (cosmetic)
<object class="sizeritem"> - <object class="wxStaticText" name="txt_superz_info"> - <label>SuperZ Info : NA</label> - <fg>#E5E5E5</fg> - <hidden>1</hidden> - </object> - <flag>wxALL</flag> - <border>10</border> - </object> + <object class="wxStaticText" name="txt_superz_info"> + <label>SuperZ Info : NA</label> + <fg>#E5E5E5</fg> + <hidden>1</hidden> + </object> + <flag>wxALL</flag> + <border>10</border> + </object>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/main_xrc.py` around lines 2035 - 2043, The XML for the wxStaticText node txt_superz_info is mis‑indented relative to its parent sizeritem; reformat the block so the <object class="wxStaticText" name="txt_superz_info"> and its child tags (<label>, <fg>, <hidden>) are indented one level deeper than the enclosing <object class="sizeritem"> and ensure the closing </object> for the wxStaticText aligns with its opening tag, matching the indentation style used by the txt_refinez_active block.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/odemis/gui/cont/acquisition/cryo_z_localization.pysrc/odemis/gui/cont/multi_point_correlation.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/acquisition/cryo_z_localization.py
🤖 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/cont/multi_point_correlation.py`:
- Around line 188-198: The UI elements txt_superz_info and txt_refinez_active
are always shown but only meaningful in mutually exclusive modes; update the
visibility logic so txt_superz_info is only shown (Show(True)) and its label set
when not self.refinez_active (i.e., SuperZ mode), otherwise hide it
(Show(False)), and likewise show txt_refinez_active only when
self.refinez_active and hide it otherwise; make these changes in the
initialization block that currently references txt_superz_info and
txt_refinez_active (and ensure this compensates for _on_z_targeting not being
called in the SuperZ flow).
---
Outside diff comments:
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Around line 807-814: The CallLater timers scheduled in _on_z_targeting are
never tracked or cancelled which can call txt_refinez_active.SetLabel after the
widget is destroyed and allows timers to accumulate; fix this by adding an
instance variable (e.g. self._refinez_timer initialized in __init__), canceling
any existing timer before scheduling a new one inside _on_z_targeting (call
Stop()/Cancel on the previous CallLater and set/replace self._refinez_timer with
the new wx.CallLater return value), and ensure stop() cancels and clears
self._refinez_timer so no pending callback runs after teardown.
---
Nitpick comments:
In `@src/odemis/gui/main_xrc.py`:
- Around line 2035-2043: The XML for the wxStaticText node txt_superz_info is
mis‑indented relative to its parent sizeritem; reformat the block so the <object
class="wxStaticText" name="txt_superz_info"> and its child tags (<label>, <fg>,
<hidden>) are indented one level deeper than the enclosing <object
class="sizeritem"> and ensure the closing </object> for the wxStaticText aligns
with its opening tag, matching the indentation style used by the
txt_refinez_active block.
In `@src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc`:
- Around line 109-117: The XML block for the sizer item containing the
wxStaticText named txt_superz_info has inconsistent indentation; re-indent the
opening <object class="sizeritem"> and all its child tags (including the <object
class="wxStaticText" name="txt_superz_info">, <label>, <fg>, <hidden>, <flag>,
and <border>) to match the surrounding sizeritem blocks (e.g., mirror the
indentation style used by txt_refinez_active) so the hierarchy is visually
aligned and consistent.
| # Access the Super Z Info text | ||
| self.txt_superz_info = self._panel.txt_superz_info | ||
| self.txt_superz_info.Show(True) | ||
| if self.refinez_active: | ||
| self.txt_superz_info.SetLabel("SuperZ Info : NA") | ||
| else: | ||
| self.txt_superz_info.SetLabel("SuperZ Info : Available") | ||
|
|
||
| # Access the Refine Z text (to check if refine_z is working or not) | ||
| self.txt_refinez_active = self._panel.txt_refinez_active | ||
| self.txt_refinez_active.Show(True) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and understand its size
find . -name "multi_point_correlation.py" -type fRepository: delmic/odemis
Length of output: 105
🏁 Script executed:
# Read the file to see lines 188-198 and surrounding context
wc -l src/odemis/gui/cont/multi_point_correlation.pyRepository: delmic/odemis
Length of output: 107
🏁 Script executed:
# Check the actual implementation around lines 188-198
head -n 250 src/odemis/gui/cont/multi_point_correlation.py | tail -n +150Repository: delmic/odemis
Length of output: 5288
🏁 Script executed:
# Find all references to txt_superz_info and txt_refinez_active
rg "txt_superz_info|txt_refinez_active" src/odemis/gui/cont/multi_point_correlation.py -nRepository: delmic/odemis
Length of output: 554
🏁 Script executed:
# Look at _on_z_targeting method
sed -n '/def _on_z_targeting/,/^ def /p' src/odemis/gui/cont/multi_point_correlation.py | head -n 30Repository: delmic/odemis
Length of output: 1680
🏁 Script executed:
# Look at _on_target_changes method
sed -n '/def _on_target_changes/,/^ def /p' src/odemis/gui/cont/multi_point_correlation.py | head -n 40Repository: delmic/odemis
Length of output: 2364
🏁 Script executed:
# Search for the XRC file that defines these UI elements
find . -name "*.xrc" -type f | xargs grep -l "txt_superz_info\|txt_refinez_active" 2>/dev/nullRepository: delmic/odemis
Length of output: 116
🏁 Script executed:
# Check the XRC file for txt_superz_info and txt_refinez_active definitions
grep -A 5 -B 2 "txt_superz_info\|txt_refinez_active" ./src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcRepository: delmic/odemis
Length of output: 953
🏁 Script executed:
# Get more context around these UI elements in XRC
grep -n "txt_superz_info\|txt_refinez_active" ./src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcRepository: delmic/odemis
Length of output: 234
🏁 Script executed:
# Final check: confirm no other Show/Hide calls for these elements
rg "txt_superz_info|txt_refinez_active" src/odemis/gui/cont/multi_point_correlation.py -B 2 -A 2Repository: delmic/odemis
Length of output: 881
Align text element visibility with active workflow mode to avoid UI clutter.
txt_superz_info is unconditionally shown but displays only "SuperZ Info : NA" in Refine Z mode, providing no useful information. Similarly, txt_refinez_active is always shown but remains as whitespace when SuperZ is active (since _on_z_targeting never executes in that path). Both elements should be shown only in their relevant contexts: txt_superz_info when SuperZ is available (not self.refinez_active), and txt_refinez_active when Refine Z is enabled (self.refinez_active).
🔧 Suggested fix to align visibility with design intent
- # Access the Super Z Info text
- self.txt_superz_info = self._panel.txt_superz_info
- self.txt_superz_info.Show(True)
- if self.refinez_active:
- self.txt_superz_info.SetLabel("SuperZ Info : NA")
- else:
- self.txt_superz_info.SetLabel("SuperZ Info : Available")
-
- # Access the Refine Z text (to check if refine_z is working or not)
- self.txt_refinez_active = self._panel.txt_refinez_active
- self.txt_refinez_active.Show(True)
+ # Access the Super Z Info text — only shown when SuperZ data is available
+ self.txt_superz_info = self._panel.txt_superz_info
+ if not self.refinez_active:
+ self.txt_superz_info.SetLabel("SuperZ Info : Available")
+ self.txt_superz_info.Show(True)
+
+ # Access the Refine Z active indicator — only relevant when refine Z is enabled
+ self.txt_refinez_active = self._panel.txt_refinez_active
+ if self.refinez_active:
+ self.txt_refinez_active.Show(True)📝 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.
| # Access the Super Z Info text | |
| self.txt_superz_info = self._panel.txt_superz_info | |
| self.txt_superz_info.Show(True) | |
| if self.refinez_active: | |
| self.txt_superz_info.SetLabel("SuperZ Info : NA") | |
| else: | |
| self.txt_superz_info.SetLabel("SuperZ Info : Available") | |
| # Access the Refine Z text (to check if refine_z is working or not) | |
| self.txt_refinez_active = self._panel.txt_refinez_active | |
| self.txt_refinez_active.Show(True) | |
| # Access the Super Z Info text — only shown when SuperZ data is available | |
| self.txt_superz_info = self._panel.txt_superz_info | |
| if not self.refinez_active: | |
| self.txt_superz_info.SetLabel("SuperZ Info : Available") | |
| self.txt_superz_info.Show(True) | |
| # Access the Refine Z active indicator — only relevant when refine Z is enabled | |
| self.txt_refinez_active = self._panel.txt_refinez_active | |
| if self.refinez_active: | |
| self.txt_refinez_active.Show(True) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/gui/cont/multi_point_correlation.py` around lines 188 - 198, The
UI elements txt_superz_info and txt_refinez_active are always shown but only
meaningful in mutually exclusive modes; update the visibility logic so
txt_superz_info is only shown (Show(True)) and its label set when not
self.refinez_active (i.e., SuperZ mode), otherwise hide it (Show(False)), and
likewise show txt_refinez_active only when self.refinez_active and hide it
otherwise; make these changes in the initialization block that currently
references txt_superz_info and txt_refinez_active (and ensure this compensates
for _on_z_targeting not being called in the SuperZ flow).
| if self._tab_data.main.currentFeature.value: | ||
| feature = self._tab_data.main.currentFeature.value | ||
| feature.superz_stream_name = self._selected_stream.name.value | ||
|
|
||
| # Save the stream name in the config file | ||
| acq_conf = conf.get_acqui_conf() | ||
| save_features(acq_conf.pj_last_path, self._tab_data.main.features.value) |
There was a problem hiding this comment.
I don't really get the changes of this file. They don't seem related to the Refine Z button. I don't really mind if you're fixing something... but could you explain in the commit text?
There was a problem hiding this comment.
It is related to the setting of feature.superz_stream_name. Based on this it is decided if refine z button should be disabled.
But feature.superz_stream_name this was set even before the Locate z button was pressed, so I removed it from on_current changes and made sure that is only set on z_localization.
There was a problem hiding this comment.
OK. Thanks for the clarification. Wouldn't it also consider feature where the SuperZ was run but failed as "SuperZ activated"? To avoid it such issue, would it work to look for the PoI related to the feature, and check whether superz_focused == True?
| if self.refinez_active: | ||
| self.txt_superz_info.SetLabel("SuperZ Info : NA") | ||
| else: | ||
| self.txt_superz_info.SetLabel("SuperZ Info : Available") |
There was a problem hiding this comment.
Let's not make the GUI too verbose. I don't think you need to add an extra label. Just the tooltip on the Refine Z button (when disabled) is enough.
There was a problem hiding this comment.
I ll check with the apps team
There was a problem hiding this comment.
The apps team indicated they like to have this info. Then let's leave it... but I'd still suggest to avoid the "SuperZ info: NA" text. Instead, drop txt_superz_info and only use the txt_refinez_active (ie, next to the Refine Z button). If there is SuperZ info, disable the button and set the text to "Super Z information in use" (and set the tool tip of the button as you did). This way nothing special will appear on METEOR without SuperZ.
| if self.refinez_active: | ||
| self.txt_superz_info.SetLabel("SuperZ Info : NA") | ||
| else: | ||
| self.txt_superz_info.SetLabel("SuperZ Info : Available") |
There was a problem hiding this comment.
The apps team indicated they like to have this info. Then let's leave it... but I'd still suggest to avoid the "SuperZ info: NA" text. Instead, drop txt_superz_info and only use the txt_refinez_active (ie, next to the Refine Z button). If there is SuperZ info, disable the button and set the text to "Super Z information in use" (and set the tool tip of the button as you did). This way nothing special will appear on METEOR without SuperZ.
2 participants
Refine Z button in the multi_point_correlation.py will also be available for Super Z GUI. The disabling of Refine Z button depends if the Locate Z button in the Localization button is used or not. So in the same GUI, depending on the feature's usage of Super Z loacalization, the Refine Z button can be enabled or disabled.
Locate Z button not clicked in the GUI
Locate Z button clicked ( 2 photos)