fix: Fix scale bar migration for canvas change in napari 0.8.1 - #1439
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates napari version handling and scale bar access to be compatible with napari 0.8.1’s canvas/overlay changes, while simplifying the render/view accessors to always use the scene canvas path. Sequence diagram for updated scale bar tick handling across napari versionssequenceDiagram
participant Settings
participant NapariImageView
participant NapariViewer
participant Canvas
participant ScaleBar
Settings->>NapariImageView: _update_scale_bar_ticks()
NapariImageView->>Settings: get_from_profile(scale_bar_ticks, True)
Settings-->>NapariImageView: ticks_value
alt _napari_gt_8_0
NapariImageView->>NapariViewer: canvas
NapariViewer-->>NapariImageView: Canvas
NapariImageView->>Canvas: overlays.scale_bar.ticks = ticks_value
else not _napari_gt_8_0
NapariImageView->>NapariViewer: scale_bar
NapariViewer-->>NapariImageView: ScaleBar
NapariImageView->>ScaleBar: ticks = ticks_value
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughNapari compatibility handling now branches scale-bar tick updates by version, while ChangesNapari compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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, and left some high level feedback:
- The
_napari_ge_5and_napari_le_7_0version flags appear to no longer be used after the_rendersimplification; consider removing them (and any related dead code) to keep the version handling clear and minimal. - If older napari versions (<0.5) are still supported, the change to always call
self.canvas._scene_canvas.render()in_rendermay break those environments; either restore a version guard or explicitly drop support for those versions and remove related compatibility code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_napari_ge_5` and `_napari_le_7_0` version flags appear to no longer be used after the `_render` simplification; consider removing them (and any related dead code) to keep the version handling clear and minimal.
- If older napari versions (<0.5) are still supported, the change to always call `self.canvas._scene_canvas.render()` in `_render` may break those environments; either restore a version guard or explicitly drop support for those versions and remove related compatibility code.
## Individual Comments
### Comment 1
<location path="package/PartSeg/common_gui/napari_image_view.py" line_range="235-239" />
<code_context>
def _update_scale_bar_ticks(self):
- self.viewer.scale_bar.ticks = self.settings.get_from_profile("scale_bar_ticks", True)
+ if _napari_gt_8_0:
+ self.viewer.canvas.overlays.scale_bar.ticks = self.settings.get_from_profile("scale_bar_ticks", True)
+ else:
+ self.viewer.scale_bar.ticks = self.settings.get_from_profile("scale_bar_ticks", True)
def toggle_points_visibility(self):
</code_context>
<issue_to_address>
**suggestion:** Avoid recomputing the profile value twice in `_update_scale_bar_ticks`.
`self.settings.get_from_profile("scale_bar_ticks", True)` is called in both branches. Assign it to a local variable before the `if` (e.g. `ticks = ...`) and reuse it to avoid duplication and simplify maintenance.
```suggestion
def _update_scale_bar_ticks(self):
ticks = self.settings.get_from_profile("scale_bar_ticks", True)
if _napari_gt_8_0:
self.viewer.canvas.overlays.scale_bar.ticks = ticks
else:
self.viewer.scale_bar.ticks = ticks
```
</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: 1
🧹 Nitpick comments (1)
package/PartSeg/common_gui/napari_image_view.py (1)
235-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both scale-bar API branches.
The existing
package/tests/test_PartSeg/test_common_gui.py:857-865test verifies only the profile value; it would not catch a failure while assigning ticks to the newer canvas path. Test both the legacyviewer.scale_barbranch and the Napari 0.8.1 Qt-canvas branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/PartSeg/common_gui/napari_image_view.py` around lines 235 - 239, Extend the tests for _update_scale_bar_ticks to cover both _napari_gt_8_0 branches: verify the legacy viewer.scale_bar.ticks assignment and the newer viewer.canvas.overlays.scale_bar.ticks assignment, using the configured profile value in each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package/PartSeg/common_gui/napari_image_view.py`:
- Around line 235-239: Update the newer-Napari branch in _update_scale_bar_ticks
to access the canvas through the QtViewer stored in self.viewer_widget, rather
than self.viewer; preserve the existing scale_bar_ticks setting and legacy
self.viewer.scale_bar path.
---
Nitpick comments:
In `@package/PartSeg/common_gui/napari_image_view.py`:
- Around line 235-239: Extend the tests for _update_scale_bar_ticks to cover
both _napari_gt_8_0 branches: verify the legacy viewer.scale_bar.ticks
assignment and the newer viewer.canvas.overlays.scale_bar.ticks assignment,
using the configured profile value in each case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 270a2d75-53fc-400d-98ef-bce7eb66c2f8
📒 Files selected for processing (1)
package/PartSeg/common_gui/napari_image_view.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1439 +/- ##
===========================================
- Coverage 92.58% 92.56% -0.02%
===========================================
Files 211 211
Lines 33224 33224
===========================================
- Hits 30760 30755 -5
- Misses 2464 2469 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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
Update napari image viewer integration to support API changes introduced in napari 0.8.1 while simplifying version-dependent rendering logic.
Bug Fixes:
Enhancements:
Summary by CodeRabbit