feat: Use QScrollArea for components selection#1328
Merged
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConverted the components chooser into a scrollable QScrollArea with a revised selection API, added ImageSettings.post_roi_set hook invoked when ROI changes, updated StackSettings to use the new chooser API and added post-ROI synchronization, and tightened a napari image view type signature. 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 list_of_components
StackSettings->>ChosenComponents: set_components(component_keys, [])
ChosenComponents->>UI: update checkbox widgets
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 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1328 +/- ##
===========================================
- Coverage 93.13% 93.13% -0.01%
===========================================
Files 211 211
Lines 33272 33306 +34
===========================================
+ Hits 30988 31019 +31
- Misses 2284 2287 +3 ☔ 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
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
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/PartSeg/_roi_mask/main_window.py (1)
464-481: Inconsistent signal blocking pattern inset_components.Other methods (
check_all,un_check_all,set_chosen) use the safer pattern that saves the previous blocking state and restores it in afinallyblock. This method should follow the same pattern for consistency and to ensure signals are restored if an exception occurs.♻️ Proposed fix for consistent signal blocking
def set_components(self, components_index, chosen_components: Union[Sequence[int], None] = None): if chosen_components is None: chosen_components = [] chosen_components = set(chosen_components) - self.blockSignals(True) - self.remove_components() - for el in components_index: - check = ComponentCheckBox(el) - if el in chosen_components: - check.setChecked(True) - check.stateChanged.connect(self.check_change) - check.mouse_enter.connect(self.mouse_enter.emit) - check.mouse_leave.connect(self.mouse_leave.emit) - self.check_box[el] = check - self.check_layout.addWidget(check) - self.blockSignals(False) + prev = self.blockSignals(True) + try: + self.remove_components() + for el in components_index: + check = ComponentCheckBox(el) + if el in chosen_components: + check.setChecked(True) + check.stateChanged.connect(self.check_change) + check.mouse_enter.connect(self.mouse_enter.emit) + check.mouse_leave.connect(self.mouse_leave.emit) + self.check_box[el] = check + self.check_layout.addWidget(check) + finally: + self.blockSignals(prev) self.update() 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 464 - 481, The set_components method currently calls self.blockSignals(True) and later self.blockSignals(False) directly; change it to mirror the safer pattern used in check_all/un_check_all/set_chosen by saving the previous blocking state (e.g., prev = self.blockSignals()) then calling self.blockSignals(True) before remove_components and the loop, and ensuring you restore the original state inside a finally block (self.blockSignals(prev)). Keep existing logic that creates ComponentCheckBox, connects check.stateChanged to self.check_change and mouse_enter/mouse_leave to self.mouse_enter.emit/self.mouse_leave.emit, and still emit self.check_change_signal at the end.
🤖 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/main_window.py`:
- Around line 464-481: The set_components method currently calls
self.blockSignals(True) and later self.blockSignals(False) directly; change it
to mirror the safer pattern used in check_all/un_check_all/set_chosen by saving
the previous blocking state (e.g., prev = self.blockSignals()) then calling
self.blockSignals(True) before remove_components and the loop, and ensuring you
restore the original state inside a finally block (self.blockSignals(prev)).
Keep existing logic that creates ComponentCheckBox, connects check.stateChanged
to self.check_change and mouse_enter/mouse_leave to
self.mouse_enter.emit/self.mouse_leave.emit, and still emit
self.check_change_signal at the end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5dc4138b-7b9e-4450-8090-a8647b42969b
📒 Files selected for processing (2)
package/PartSeg/_roi_mask/main_window.pypackage/PartSeg/common_backend/base_settings.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/PartSeg/common_backend/base_settings.py (1)
515-538: Previous review concern addressed:post_roi_set()now runs on all ROI update paths.By using
self.roi = roi_info(Line 538) instead of direct_roi_infoassignment, the property setter is invoked, ensuringpost_roi_set()is called consistently.Minor inefficiency:
fit_to_imagecalled twice.At Line 533,
roi_info = result.roi_info.fit_to_image(self.image)fits the ROI. Then at Line 538, the property setter callsfit_to_imageagain (Line 155). This is redundant if the operation is idempotent. Consider an internal setter that bypasses the re-fit, or verify that double-fitting is cheap.♻️ Potential optimization
try: roi_info = result.roi_info.fit_to_image(self.image) except ValueError as e: # pragma: no cover raise ValueError(ROI_NOT_FIT) from e if result.points is not None: self.points = result.points - self.roi = roi_info + # Already fitted, assign directly and call hook + self._roi_info = roi_info + self._additional_layers = {} + self.post_roi_set() + self.roi_changed.emit(self._roi_info)Alternatively, keep the current approach if
fit_to_imageis cheap/idempotent and code simplicity is preferred.🤖 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 515 - 538, The code currently fits the ROI twice: set_segmentation_result calls result.roi_info.fit_to_image(self.image) then assigns self.roi which triggers the roi property setter that calls fit_to_image again; fix by providing a setter path that accepts an already-fit ROI and bypasses the second fit (e.g., add an internal method or a parameter to the roi property setter such as _set_roi_info_no_refit or roi = fitted_roi with a flag already_fitted=True), then in set_segmentation_result assign the pre-fitted roi via that internal setter (and keep existing public roi property behavior unchanged elsewhere).
🤖 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/common_backend/base_settings.py`:
- Around line 515-538: The code currently fits the ROI twice:
set_segmentation_result calls result.roi_info.fit_to_image(self.image) then
assigns self.roi which triggers the roi property setter that calls fit_to_image
again; fix by providing a setter path that accepts an already-fit ROI and
bypasses the second fit (e.g., add an internal method or a parameter to the roi
property setter such as _set_roi_info_no_refit or roi = fitted_roi with a flag
already_fitted=True), then in set_segmentation_result assign the pre-fitted roi
via that internal setter (and keep existing public roi property behavior
unchanged elsewhere).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9de4b143-256c-4ed4-9d2b-73ec39a02773
📒 Files selected for processing (2)
package/PartSeg/_roi_mask/main_window.pypackage/PartSeg/common_backend/base_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
Bug Fixes