[MSD-293][feature] Refine Z button available in super Z workflow#3377
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAutomatic propagation of the selected Super Z stream into features was removed from the localization UI callbacks: code that assigned Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Localization UI
participant Controller as Localization Controller
participant FeatureModel as Feature Model
participant Disk as Disk/Storage
User->>UI: select stream
UI->>Controller: notify stream selected
alt Old flow (before change)
Controller->>FeatureModel: set feature.superz_stream_name
Controller->>Disk: save_features(feature list)
Disk-->>Controller: save ack
else New flow (after change)
Controller-->>UI: update selection state only
end
sequenceDiagram
participant User
participant UI as Correlation Dialog
participant Controller as CorrelationPointsController
participant ZSystem as Z-targeting subsystem
User->>UI: trigger Z-targeting
UI->>Controller: request Z-target
Controller->>UI: set txt_refinez_active "active..." (visible)
Controller->>Controller: schedule reset (1s)
Controller->>ZSystem: perform Z-targeting logic
ZSystem-->>Controller: result
Controller->>UI: clear/reset txt_refinez_active (after 1s)
🚥 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 Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
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.
@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!
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
2c55c3b to
cdb351c
Compare
3 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)