[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
📝 WalkthroughWalkthroughAutomatic propagation of the selected Super Z stream into features was removed from the localization UI handlers: branches that set feature.superz_stream_name in stream-selection and current-feature callbacks were deleted. The correlation dialog gains a new control 🚥 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)
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).
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.
K4rishma
force-pushed
the
refinez_avail
branch
from
1fe8c92 to
2c55c3b
Compare
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)
801-801: 🛠️ Refactor suggestion | 🟠 MajorAdd a type annotation to
evtper project coding guidelines.
_on_z_targetingis invoked both from the button handler (with awx.CommandEvent) and directly withNone(line 707), so the correct annotation isOptional[wx.CommandEvent].- def _on_z_targeting(self, evt) -> None: + def _on_z_targeting(self, evt: Optional[wx.CommandEvent]) -> None:As per coding guidelines: "Always use type hints for function parameters and return types in Python code."
🤖 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` at line 801, _update the signature of _on_z_targeting to accept Optional[wx.CommandEvent] instead of an unannotated evt; add "from typing import Optional" (if missing) and reference wx.CommandEvent in the annotation so the signature becomes def _on_z_targeting(self, evt: Optional[wx.CommandEvent]) -> None:, and ensure the body safely handles evt being None (it is called from a button handler and directly with None).
🤖 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 807-809: The scheduled wx.CallLater call must be stored and
cancelled on teardown to avoid calling SetLabel on a destroyed widget: replace
the bare wx.CallLater(...) that updates self.txt_refinez_active with assigning
it to a member (e.g. self._refine_timer = wx.CallLater(1000,
self.txt_refinez_active.SetLabel, "")) and in stop() check for and cancel/stop
that timer (and clear the reference) so the callback is not invoked after the
dialog or self.txt_refinez_active has been destroyed; also consider clearing the
timer reference after it fires to avoid stale handles.
---
Outside diff comments:
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Line 801: _update the signature of _on_z_targeting to accept
Optional[wx.CommandEvent] instead of an unannotated evt; add "from typing import
Optional" (if missing) and reference wx.CommandEvent in the annotation so the
signature becomes def _on_z_targeting(self, evt: Optional[wx.CommandEvent]) ->
None:, and ensure the body safely handles evt being None (it is called from a
button handler and directly with None).
ℹ️ 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
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc
| self.txt_refinez_active.SetLabel("active ...") | ||
| wx.CallLater(1000, self.txt_refinez_active.SetLabel, "") | ||
|
|
There was a problem hiding this comment.
wx.CallLater fires on a potentially destroyed widget — store and cancel the timer.
If the correlation dialog is closed (or stop() is called) within 1 second of clicking Refine Z — or within 1 second of an automatic Z-targeting triggered by _on_current_target_changes — the scheduled callback fires on an already-deleted wxStaticText C++ object, raising:
RuntimeError: wrapped C/C++ object of type StaticText has been deleted
Because no handle is retained, the timer cannot be cancelled in stop().
🔧 Proposed fix — retain the timer and cancel it on teardown
+ self._refinez_timer: Optional[wx.CallLater] = None
def _on_z_targeting(self, evt) -> None:
...
if self._tab_data_model.main.currentTarget.value:
- self.txt_refinez_active.SetLabel("active ...")
- wx.CallLater(1000, self.txt_refinez_active.SetLabel, "")
+ self.txt_refinez_active.SetLabel("active ...")
+ if self._refinez_timer is not None and self._refinez_timer.IsRunning():
+ self._refinez_timer.Stop()
+ self._refinez_timer = wx.CallLater(1000, self.txt_refinez_active.SetLabel, "")And in stop():
def stop(self):
"""Gracefully stop the worker thread."""
+ if self._refinez_timer is not None and self._refinez_timer.IsRunning():
+ self._refinez_timer.Stop()
+ self._refinez_timer = None
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 - 809, The
scheduled wx.CallLater call must be stored and cancelled on teardown to avoid
calling SetLabel on a destroyed widget: replace the bare wx.CallLater(...) that
updates self.txt_refinez_active with assigning it to a member (e.g.
self._refine_timer = wx.CallLater(1000, self.txt_refinez_active.SetLabel, ""))
and in stop() check for and cancel/stop that timer (and clear the reference) so
the callback is not invoked after the dialog or self.txt_refinez_active has been
destroyed; also consider clearing the timer reference after it fires to avoid
stale handles.
There was a problem hiding this comment.
@K4rishma code rabbit is correct, but as it's just 1s timeout, and the worse case is that the error is shown in the log, I'd suggest to not handle this edge case due to the extra complexity it brings to the code... but if you feel like implementing the suggestion, go ahead!
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| self.txt_refinez_active.SetLabel("active ...") | ||
| wx.CallLater(1000, self.txt_refinez_active.SetLabel, "") |
There was a problem hiding this comment.
Please move this code after the check for if not streams_projections: . This way, if it fails to start, it'll not claim it's active.
| self.txt_refinez_active.SetLabel("active ...") | ||
| wx.CallLater(1000, self.txt_refinez_active.SetLabel, "") | ||
|
|
There was a problem hiding this comment.
@K4rishma code rabbit is correct, but as it's just 1s timeout, and the worse case is that the error is shown in the log, I'd suggest to not handle this edge case due to the extra complexity it brings to the code... but if you feel like implementing the suggestion, go ahead!
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)