Add ability to remove clear markers from plots. #574
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdated an ecolab submodule reference; added a duplicate Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Plot Widget UI
participant TS as PlotWidget (TS API)
participant CPP as PlotWidget (C++)
User->>UI: change marker selections / choose "[clear selection]"
UI->>UI: getMarkersSelection() → filter out "[clear selection]"
UI->>UI: compare markers (horizontal → vertical)
alt markers changed
UI->>TS: addPlotPt(...)
TS->>CPP: addPlotPt(...)
CPP->>CPP: scalePlot()
note right of CPP #D6EAF8: label pens\nthen call removePensFrom(pen)
CPP->>TS: pen state cleared (via backend binding)
end
UI->>UI: requestRedraw()
UI->>TS: request canvas redraw
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to remove clear markers from plot widgets. The changes enable users to clear marker selections through the UI and properly update the plot display when markers are removed.
- Implements marker removal logic in both backend (C++) and frontend (TypeScript)
- Updates UI to support clearing marker selections via a new "[clear selection]" option
- Adds visual styling to distinguish the clear selection option
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| model/plotWidget.cc | Adds call to removePensFrom() to clear markers after processing pen assignments |
| gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.ts | Filters out clear selection markers and adds logic to detect marker removal and trigger plot updates |
| gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.scss | Adds CSS styling for the clear selection option with blue color and bold font |
| gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.html | Updates marker selection dropdowns to include clickable "[clear selection]" option |
| gui-js/libs/shared/src/lib/backend/minsky.ts | Adds TypeScript binding for the removePensFrom() C++ method |
| ecolab | Updates subproject commit reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.ts
Outdated
Show resolved
Hide resolved
gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.ts
Outdated
Show resolved
Hide resolved
gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.html
Outdated
Show resolved
Hide resolved
![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)
gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.html (1)
127-128: Consider making the placeholder option disabled for clarity.The "[clear selection]" option is currently selectable but gets filtered out in the TypeScript code. This might confuse users who select it expecting an action. Consider adding
disabledto make the intent clearer, or handle the selection explicitly to clear all markers.Apply this diff if you prefer a disabled placeholder:
- <option class="selectTitle" title="Click to clear selection" - value="[clear selection]">Vertical Markers</option> + <option class="selectTitle" disabled + value="[clear selection]">Vertical Markers</option>And similarly for horizontal markers:
- <option class="selectTitle" title="Click to clear selection" - value="[clear selection]">Horizontal Markers</option> + <option class="selectTitle" disabled + value="[clear selection]">Horizontal Markers</option>Also applies to: 132-133
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ecolab(1 hunks)gui-js/libs/shared/src/lib/backend/minsky.ts(1 hunks)gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.html(1 hunks)gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.scss(1 hunks)gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.ts(2 hunks)model/plotWidget.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
model/plotWidget.cc (1)
gui-js/libs/shared/src/lib/backend/minsky.ts (1)
removePensFrom(1657-1657)
🪛 GitHub Actions: CI Tests - Debug
ecolab
[error] 655-655: CppWrapperType: 'memset' used on non-trivial type. Consider value-initialization or assignment instead. (-Werror=class-memaccess)
[error] 115-121: ObjectPtrBase constructor: 'm_id' will be initialized after 'std::shared_ptr' leading to reorder issues. (-Werror=reorder)
[error] 395-395: GraphBase::purge(): comparison between signed/unsigned integers. (-Werror=sign-compare)
[error] 201-201: Allocator nested type name collision: 'using Allocator=Allocator' redefines Allocator symbol in this scope. (-Werror=changes-meaning)
🪛 GitHub Actions: CI Tests - Release
ecolab
[error] 655-655: CppWrapperType constructor uses memset on a non-trivial type. This triggers -Werror=class-memaccess.
[error] 115-115: graphcode::ObjectPtrBase::m_id will be initialized after base class member; -Werror=reorder.
[error] 119-121: Constructor initialization order issue flagged by -Werror=reorder in ObjectPtrBase.
[error] 220-220: Using alias 'Allocator' via 'using Allocator=Allocator' changes meaning of 'Allocator'; -Werror=changes-meaning.
[error] 395-395: Comparison of int and unsigned int in purge(); -Werror=sign-compare.
[warning] 536-536: Note: 'struct CppWrapperType' declared here (potentially related to previous memset issue).
[error] 35-35: C++ compilation encountered multiple -Werror errors; overall build failed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript, 22.04)
🔇 Additional comments (4)
ecolab (1)
1-1: Discard this review comment—the compilation errors cited do not exist in the codebase.The four specific compilation errors claimed in this review cannot be verified:
- CppWrapperType memset at line 655: Not found in ecolab
- ObjectPtrBase constructor at lines 115–121: Not found in ecolab
- Allocator alias collision at lines 220/201: Not found in ecolab
- sign-compare in purge() at line 395: Not found in ecolab
Additionally, the commit hashes referenced in the review's scratchpad (073dc08c58... and 08a6cd27b...) do not exist in the ecolab repository history. No CI pipeline output or build logs supporting these errors are present in the repository.
The ecolab submodule itself is legitimate, but the compilation error claims are unfounded.
Likely an incorrect or invalid review comment.
gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.scss (1)
7-10: LGTM!The CSS styling clearly distinguishes the clear-selection placeholder from regular marker options.
model/plotWidget.cc (1)
527-527: LGTM!The call to
removePensFrom(pen)correctly cleans up pens used for marker labeling before proceeding with plot rendering.gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.ts (1)
184-185: LGTM!The filter correctly excludes the "[clear selection]" placeholder when collecting selected markers.
gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.ts
Show resolved
Hide resolved
If the placeholder is disabled, it cannot be clicked. All I wanted was somewhere to click to remove all selections, which seems like a design flaw of multiselectable selects |
…get-options.component.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.ts (1)
228-245: Good fix for thefor...ofloops; removal detection works correctly.The
for...ofloops now correctly iterate over marker values instead of array indices, addressing the previous review concerns. The logic successfully detects when markers are removed from either horizontal or vertical marker sets and triggersaddPlotPt()to synchronize backend state before redraw.As you noted earlier, the code intentionally only detects removals (old markers not present in new) and doesn't check for additions (new markers not present in old). If you later decide addition detection is needed, you could add a second pass:
// After line 243, add: if (!markersUpdated) { for (let marker of markers) if (!this.horizontalMarkers.includes(marker)) { markersUpdated = true; break; } }Optional: Consider renaming
markersUpdatedtomarkersRemovedfor clarity, since it specifically tracks removals rather than all changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ubuntu-20.04
🔇 Additional comments (1)
gui-js/libs/ui-components/src/lib/plot-widget-options/plot-widget-options.component.ts (1)
184-185: LGTM! Placeholder filtering works correctly.The logic correctly filters out the "[clear selection]" placeholder value, ensuring only actual marker names are collected from the multi-select dropdown.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
…cecoder/minsky into bugfix-1874-remove-markers
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
This change is
Summary by CodeRabbit
New Features
Improvements
Chores