fix(gsps): allow GSPS toggle when any viewport has overlay#29
fix(gsps): allow GSPS toggle when any viewport has overlay#29
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![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
🧹 Nitpick comments (1)
src/dicom.rs (1)
1228-1249: Maketest_stubconsistent withframe_count.Right now the stub reports
frame_count: 1, but both frame stores are empty, soframe_mono_pixels(0)always returnsNone. That makes the helper easy to misuse in tests that expect a minimal renderable image.Proposed fix
#[cfg(test)] impl DicomImage { pub(crate) fn test_stub(gsps_overlay: Option<GspsOverlay>) -> Self { Self { width: 1, height: 1, - mono_frames: MonoFrames::None, + mono_frames: MonoFrames::Eager(vec![Arc::<[i32]>::from( + vec![0].into_boxed_slice(), + )]), rgb_frames: RgbFrames::None, frame_count: 1, color_mode: ImageColorMode::Monochrome, samples_per_pixel: 1, invert: false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dicom.rs` around lines 1228 - 1249, The test_stub currently sets frame_count: 1 while both MonoFrames and RgbFrames are empty, making frame_mono_pixels(0) return None; update test_stub to set frame_count: 0 (or alternatively populate MonoFrames/RgbFrames with a single 1x1 frame) so the frame_count value matches the actual frame stores—modify the frame_count field in the test_stub constructor to 0 (or add a minimal MonoFrames::OneFrame containing a single pixel and keep frame_count as 1) and ensure consistency with functions like frame_mono_pixels.
🤖 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/app.rs`:
- Around line 323-329: The code sets self.status_line to an error when
!self.has_available_gsps_overlay(), but never clears that stale message when a
later successful toggle occurs; update the successful toggle path (the branch
where you currently flip self.gsps_overlay_visible = !self.gsps_overlay_visible)
to clear the error by resetting self.status_line (e.g. to an empty string) so
that when has_available_gsps_overlay() is true and you change
self.gsps_overlay_visible the old error is removed; ensure the change touches
the block containing has_available_gsps_overlay(), gsps_overlay_visible, and
status_line so the message is cleared only on a successful toggle.
---
Nitpick comments:
In `@src/dicom.rs`:
- Around line 1228-1249: The test_stub currently sets frame_count: 1 while both
MonoFrames and RgbFrames are empty, making frame_mono_pixels(0) return None;
update test_stub to set frame_count: 0 (or alternatively populate
MonoFrames/RgbFrames with a single 1x1 frame) so the frame_count value matches
the actual frame stores—modify the frame_count field in the test_stub
constructor to 0 (or add a minimal MonoFrames::OneFrame containing a single
pixel and keep frame_count as 1) and ensure consistency with functions like
frame_mono_pixels.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/app.rs (1)
323-330:⚠️ Potential issue | 🟡 MinorOnly clear the stale GSPS error, not every status message.
self.status_line.clear()also removes unrelated messages like successful load/status text whenever the user toggles GSPS successfully. This should only clear the known “No GSPS overlay…” message.Suggested change
fn toggle_gsps_overlay(&mut self) { + const NO_GSPS_MESSAGE: &str = + "No GSPS overlay available for the current image or group."; if !self.has_available_gsps_overlay() { self.gsps_overlay_visible = false; - self.status_line = - "No GSPS overlay available for the current image or group.".to_string(); + self.status_line = NO_GSPS_MESSAGE.to_string(); return; } - self.status_line.clear(); + if self.status_line == NO_GSPS_MESSAGE { + self.status_line.clear(); + } self.gsps_overlay_visible = !self.gsps_overlay_visible; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.rs` around lines 323 - 330, When toggling GSPS overlay in the block that checks has_available_gsps_overlay(), don't unconditionally clear self.status_line; instead only clear that field if it currently equals the specific "No GSPS overlay available for the current image or group." message so other status text remains. Update the code around has_available_gsps_overlay(), gsps_overlay_visible, and status_line to check the current value of self.status_line before clearing it and leave it untouched otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app.rs`:
- Around line 323-330: When toggling GSPS overlay in the block that checks
has_available_gsps_overlay(), don't unconditionally clear self.status_line;
instead only clear that field if it currently equals the specific "No GSPS
overlay available for the current image or group." message so other status text
remains. Update the code around has_available_gsps_overlay(),
gsps_overlay_visible, and status_line to check the current value of
self.status_line before clearing it and leave it untouched otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b20aa8c3-6bf4-4f6d-a8fd-fb72902f4ffc
📒 Files selected for processing (2)
src/app.rssrc/dicom.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/dicom.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary by CodeRabbit