feat: Use QScrollArea for components selection#1328
Open
Conversation
Contributor
Reviewer's GuideWrap the components selection widget in a vertically scrollable QScrollArea, adjust its API to separate component list initialization from selection updates, and integrate it into the ROI/stack settings lifecycle while improving layout behavior and type hints. Sequence diagram for ROI update and components list initializationsequenceDiagram
participant Client
participant StackSettings
participant BaseSettings
participant ChosenComponents
Client->>StackSettings: set_project_info(data)
activate StackSettings
StackSettings->>StackSettings: compute new_roi_info and selected_components
StackSettings->>BaseSettings: roi = new_roi_info
activate BaseSettings
BaseSettings->>BaseSettings: set _roi_info
BaseSettings->>BaseSettings: clear _additional_layers
BaseSettings->>StackSettings: post_roi_set()
deactivate BaseSettings
activate StackSettings
StackSettings->>ChosenComponents: set_chose(roi_info.bound_info.keys(), [])
deactivate StackSettings
Note over StackSettings,ChosenComponents: Later, when restoring selection
StackSettings->>ChosenComponents: set_chosen(selected_components)
deactivate StackSettings
Class diagram for updated ChosenComponents and settings integrationclassDiagram
class QScrollArea
class ChosenComponents {
+dict~int,ComponentCheckBox~ check_box
+QPushButton check_all_btn
+QPushButton uncheck_all_btn
+FlowLayout check_layout
+check_change_signal
+mouse_enter
+mouse_leave
+ChosenComponents()
+other_component_choose(num int)
+check_all()
+uncheck_all()
+remove_components()
+new_choose(num int, chosen_components Sequence_int_)
+set_chose(components_index Sequence_int_, chosen_components Sequence_int_)
+set_chosen(chosen_components Sequence_int_)
+check_change()
+change_state(num int, val bool)
+get_state(num int) bool
+get_chosen() list_int_
}
QScrollArea <|-- ChosenComponents
class BaseSettings {
-ROIInfo _roi_info
-dict _additional_layers
+roi ROIInfo
+roi_changed
+set_roi(val Union_np_ndarray_ROIInfo_)
+post_roi_set()
}
class StackSettings {
+ChosenComponents chosen_components_widget
+set_project_info(data MaskProjectTuple_or_PointsInfo)
+_set_roi_info(state, new_roi_info ROIInfo, segmentation_parameters dict, list_of_components list_int_, save_chosen bool)
+post_roi_set()
}
BaseSettings <|-- StackSettings
StackSettings --> ChosenComponents : manages_selection
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughConverted the components chooser into a scrollable widget with updated selection API, added ImageSettings.post_roi_set hook invoked when ROI changes, adjusted StackSettings to use new chooser API and added post_roi_set synchronization, and refined a napari image view type signature; AlgorithmOptions now nests chooser in a vertical splitter. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageSettings
participant StackSettings
participant ChosenComponents
participant UI
User->>ImageSettings: change/select ROI
ImageSettings->>ImageSettings: set _roi_info, clear layers
ImageSettings->>StackSettings: post_roi_set()
StackSettings->>StackSettings: compute component lists (list_of_components)
StackSettings->>ChosenComponents: set_components(component_keys, [])
ChosenComponents->>UI: update checkboxes (no rebuild if possible)
UI->>ChosenComponents: ensureWidgetVisible(toggled_checkbox)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.4)package/PartSeg/_roi_mask/stack_settings.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1328 +/- ##
===========================================
+ Coverage 93.12% 93.13% +0.01%
===========================================
Files 211 211
Lines 33272 33297 +25
===========================================
+ Hits 30983 31010 +27
+ Misses 2289 2287 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="package/PartSeg/_roi_mask/stack_settings.py" line_range="303-311" />
<code_context>
if save_chosen:
</code_context>
<issue_to_address>
**question (bug_risk):** set_chosen no longer constrains components to the available ROI parameters list.
Previously, the `save_chosen` path constrained components via `set_chosen(sorted(state2.roi_extraction_parameters.keys()))`. Now it uses only `state2.selected_components` and relies on `post_roi_set` / `bound_info.keys()`, so the component list is no longer tied to `roi_extraction_parameters`. If `roi_extraction_parameters` and `bound_info` diverge (e.g. params exist for components no longer in the ROI), some components with saved parameters may become unselectable. If that’s unintended, consider deriving the component list from the parameter keys, or intersecting `roi_extraction_parameters` and `bound_info` explicitly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/PartSeg/_roi_mask/main_window.py`:
- Around line 473-476: The loop in set_chosen causes many stateChanged signals
and repeated image refreshes; wrap per-checkbox updates with signal blocking and
then trigger a single update at the end: in set_chosen (method name) call
check.blockSignals(True) before check.setChecked(...) and
check.blockSignals(False) after the loop (or for each check), then explicitly
emit the existing check_change_signal or call image_view.refresh_selected() once
after the loop to perform a single refresh; this prevents per-checkbox
stateChanged handlers from firing during the batch update invoked by
StackSettings.
In `@package/PartSeg/_roi_mask/stack_settings.py`:
- Around line 313-315: post_roi_set may be called before the UI widget exists
(chosen_components_widget is initialized to None), causing an AttributeError;
update post_roi_set to guard the call by checking that
self.chosen_components_widget is truthy (or hasattr/set to non-None) before
calling set_chose(self.roi_info.bound_info.keys(), []), so headless or pre-UI
usage of post_roi_set/setting self.roi doesn't raise.
In `@package/PartSeg/common_backend/base_settings.py`:
- Around line 158-163: The post_roi_set() hook is only called in one branch, so
ensure it runs on every ROI update path: call self.post_roi_set() before any
early returns in the ROI setter (so it executes even when ROI is set to None)
and add a call to self.post_roi_set() inside
BaseSettings.set_segmentation_result (the method referenced as
set_segmentation_result) after the internal ROI/state is updated; keep the
existing self.roi_changed.emit(self._roi_info) behavior but ensure
post_roi_set() is invoked consistently before or right after emitting so
subclasses are always synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 767af4e3-be73-433d-b152-e3afa0efc1f2
📒 Files selected for processing (4)
package/PartSeg/_roi_mask/main_window.pypackage/PartSeg/_roi_mask/stack_settings.pypackage/PartSeg/common_backend/base_settings.pypackage/PartSeg/common_gui/napari_image_view.py
Comment on lines
+473
to
+476
| def set_chosen(self, chosen_components: Sequence[int]): | ||
| chosen_components = set(chosen_components) | ||
| for num, check in self.check_box.items(): | ||
| check.setChecked(num in chosen_components) |
Contributor
There was a problem hiding this comment.
Batch set_chosen() updates into a single refresh.
Each setChecked() here can emit stateChanged, which reaches check_change_signal and image_view.refresh_selected() via Line 544. Since StackSettings now calls set_chosen() after ROI loads at Line 174 and Line 306 in package/PartSeg/_roi_mask/stack_settings.py, loading a segmentation with many selected components will refresh the view once per component.
Proposed fix
def set_chosen(self, chosen_components: Sequence[int]):
chosen_components = set(chosen_components)
- for num, check in self.check_box.items():
- check.setChecked(num in chosen_components)
+ self.blockSignals(True)
+ try:
+ for num, check in self.check_box.items():
+ check.setChecked(num in chosen_components)
+ finally:
+ self.blockSignals(False)
+ self.check_change_signal.emit()📝 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.
Suggested change
| def set_chosen(self, chosen_components: Sequence[int]): | |
| chosen_components = set(chosen_components) | |
| for num, check in self.check_box.items(): | |
| check.setChecked(num in chosen_components) | |
| def set_chosen(self, chosen_components: Sequence[int]): | |
| chosen_components = set(chosen_components) | |
| self.blockSignals(True) | |
| try: | |
| for num, check in self.check_box.items(): | |
| check.setChecked(num in chosen_components) | |
| finally: | |
| self.blockSignals(False) | |
| self.check_change_signal.emit() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/PartSeg/_roi_mask/main_window.py` around lines 473 - 476, The loop in
set_chosen causes many stateChanged signals and repeated image refreshes; wrap
per-checkbox updates with signal blocking and then trigger a single update at
the end: in set_chosen (method name) call check.blockSignals(True) before
check.setChecked(...) and check.blockSignals(False) after the loop (or for each
check), then explicitly emit the existing check_change_signal or call
image_view.refresh_selected() once after the loop to perform a single refresh;
this prevents per-checkbox stateChanged handlers from firing during the batch
update invoked by StackSettings.
Comment on lines
+158
to
+163
| self.post_roi_set() | ||
| self.roi_changed.emit(self._roi_info) | ||
|
|
||
| def post_roi_set(self) -> None: | ||
| """called after roi is set, for subclasses to override""" | ||
|
|
Contributor
There was a problem hiding this comment.
Make post_roi_set() run on every ROI update path.
Right now this hook only fires in the non-None branch here. The early return above and BaseSettings.set_segmentation_result() at Line 537 still bypass it, so subclasses overriding post_roi_set() will stay synchronized for some ROI updates but not others.
Proposed fix
def roi(self, val: Union[np.ndarray, ROIInfo]):
if val is None:
self._roi_info = ROIInfo(val)
self._additional_layers = {}
+ self.post_roi_set()
self.roi_clean.emit()
return
try:
if isinstance(val, np.ndarray):
self._roi_info = ROIInfo(self.image.fit_array_to_image(val))
@@
if result.points is not None:
self.points = result.points
self._roi_info = roi_info
+ self.post_roi_set()
self.roi_changed.emit(self._roi_info)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/PartSeg/common_backend/base_settings.py` around lines 158 - 163, The
post_roi_set() hook is only called in one branch, so ensure it runs on every ROI
update path: call self.post_roi_set() before any early returns in the ROI setter
(so it executes even when ROI is set to None) and add a call to
self.post_roi_set() inside BaseSettings.set_segmentation_result (the method
referenced as set_segmentation_result) after the internal ROI/state is updated;
keep the existing self.roi_changed.emit(self._roi_info) behavior but ensure
post_roi_set() is invoked consistently before or right after emitting so
subclasses are always synchronized.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
package/PartSeg/_roi_mask/main_window.py (1)
473-477:⚠️ Potential issue | 🟠 MajorBatch
set_chosen()into one signal emission.Because Line 545 wires
check_change_signaltoimage_view.refresh_selected, each changedsetChecked()here can still trigger a full refresh before the final explicit emit. On ROI loads with many selected components, that turns one sync into N+1 view refreshes.💡 Proposed fix
def set_chosen(self, chosen_components: Sequence[int]): chosen_components = set(chosen_components) for num, check in self.check_box.items(): - check.setChecked(num in chosen_components) + check.blockSignals(True) + try: + check.setChecked(num in chosen_components) + finally: + check.blockSignals(False) self.check_change_signal.emit()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/PartSeg/_roi_mask/main_window.py` around lines 473 - 477, Batch the per-checkbox updates in set_chosen by temporarily blocking signals on each QCheckBox while calling setChecked so intermediate stateChanged signals don't trigger image_view.refresh_selected; for example, in set_chosen iterate self.check_box.items(), call check.blockSignals(True) (or use QSignalBlocker(check)), call check.setChecked(...), then unblock (blockSignals(False)) and after the loop emit self.check_change_signal.emit() once. Ensure you reference the set_chosen method and self.check_box and leave the final explicit emit as the single notification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@package/PartSeg/_roi_mask/main_window.py`:
- Around line 473-477: Batch the per-checkbox updates in set_chosen by
temporarily blocking signals on each QCheckBox while calling setChecked so
intermediate stateChanged signals don't trigger image_view.refresh_selected; for
example, in set_chosen iterate self.check_box.items(), call
check.blockSignals(True) (or use QSignalBlocker(check)), call
check.setChecked(...), then unblock (blockSignals(False)) and after the loop
emit self.check_change_signal.emit() once. Ensure you reference the set_chosen
method and self.check_box and leave the final explicit emit as the single
notification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b9be0c1-0617-4b8b-a660-58ac2fce6f04
📒 Files selected for processing (3)
package/PartSeg/_roi_mask/main_window.pypackage/PartSeg/_roi_mask/stack_settings.pypackage/PartSeg/common_backend/base_settings.py
🚧 Files skipped from review as they are similar to previous changes (2)
- package/PartSeg/common_backend/base_settings.py
- package/PartSeg/_roi_mask/stack_settings.py
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
package/PartSeg/_roi_mask/stack_settings.py (2)
306-311: Same null check concern applies here.The
set_chosen()calls at lines 306 and 310 lack the null guard thatpost_roi_set()has. For consistency, consider adding a similar check.🛡️ Proposed defensive guard
self.roi = state2.roi_info - self.chosen_components_widget.set_chosen(state2.selected_components) + if self.chosen_components_widget is not None: + self.chosen_components_widget.set_chosen(state2.selected_components) self.components_parameters_dict = state2.roi_extraction_parameters else: self.roi = new_roi_info - self.chosen_components_widget.set_chosen(list_of_components) + if self.chosen_components_widget is not None: + self.chosen_components_widget.set_chosen(list_of_components) self.components_parameters_dict = segmentation_parameters🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/PartSeg/_roi_mask/stack_settings.py` around lines 306 - 311, The calls to chosen_components_widget.set_chosen(...) in the conditional branches (using state2.selected_components and list_of_components) lack a null guard like the one used in post_roi_set(); update both usages to first check that chosen_components_widget is not None (or is truthy) before calling set_chosen so you don't call a method on a missing widget—apply the same defensive pattern used in post_roi_set() around the set_chosen invocations and leave the rest of the assignments (self.components_parameters_dict, self.roi) unchanged.
174-181: Consider adding null checks forchosen_components_widgetfor consistency.Unlike
post_roi_set()which guards againstchosen_components_widgetbeingNone(line 314), these call sites don't have a similar guard. Ifset_project_infois invoked before the widget is attached,post_roi_set()will return early but the subsequentset_chosen()call will raiseAttributeError.🛡️ Proposed defensive guard
self.roi = state2.roi_info - self.chosen_components_widget.set_chosen(state2.selected_components) - + if self.chosen_components_widget is not None: + self.chosen_components_widget.set_chosen(state2.selected_components) self.components_parameters_dict = state2.roi_extraction_parameters else: self.set_history(data.history) self.roi = data.roi_info - self.chosen_components_widget.set_chosen(data.selected_components) - + if self.chosen_components_widget is not None: + self.chosen_components_widget.set_chosen(data.selected_components) self.components_parameters_dict = data.roi_extraction_parameters🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/PartSeg/_roi_mask/stack_settings.py` around lines 174 - 181, The calls to self.chosen_components_widget.set_chosen in set_project_info lack a null check and can raise AttributeError if the widget isn't attached; update set_project_info to guard before calling set_chosen (e.g., if self.chosen_components_widget is not None) in both branches where state2.selected_components and data.selected_components are used, mirroring the defensive check in post_roi_set so that when chosen_components_widget is None the method returns or skips set_chosen safely while still setting history/roi/components_parameters_dict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package/PartSeg/_roi_mask/stack_settings.py`:
- Around line 306-311: The calls to chosen_components_widget.set_chosen(...) in
the conditional branches (using state2.selected_components and
list_of_components) lack a null guard like the one used in post_roi_set();
update both usages to first check that chosen_components_widget is not None (or
is truthy) before calling set_chosen so you don't call a method on a missing
widget—apply the same defensive pattern used in post_roi_set() around the
set_chosen invocations and leave the rest of the assignments
(self.components_parameters_dict, self.roi) unchanged.
- Around line 174-181: The calls to self.chosen_components_widget.set_chosen in
set_project_info lack a null check and can raise AttributeError if the widget
isn't attached; update set_project_info to guard before calling set_chosen
(e.g., if self.chosen_components_widget is not None) in both branches where
state2.selected_components and data.selected_components are used, mirroring the
defensive check in post_roi_set so that when chosen_components_widget is None
the method returns or skips set_chosen safely while still setting
history/roi/components_parameters_dict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74abb391-3a08-49c5-bd38-19fc3b3f4a6b
📒 Files selected for processing (1)
package/PartSeg/_roi_mask/stack_settings.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Make component selection scrollable and update ROI handling hooks to keep chosen components in sync with ROI changes.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Refactor