Repository navigation
Fix joined ravels not synchronizing state when joining existing lock groups - #570
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 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. WalkthroughAdds an initialBroadcast() call when a ravel joins a RavelLockGroup so the group's current state is propagated to the newly joined ravel. Adds a test verifying two registered ravels share the lock group and that caliper-related state is broadcast on join. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Model
participant RavelA
participant RavelB
participant LockGroup
Test->>Model: create & register RavelA
Test->>Model: create & register RavelB
Test->>RavelA: create lock group (colour)
RavelA->>LockGroup: associate (create)
Test->>RavelA: set caliper/state
Note right of LockGroup: LockGroup holds current state
Test->>RavelB: joinLockGroup(colour)
RavelB->>LockGroup: associate with existing group
LockGroup->>LockGroup: initialBroadcast()
LockGroup-->>RavelB: sync caliper/state
LockGroup-->>RavelA: (no-op or confirm)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
|
|
||
| // Verify both ravels are in the same lock group | ||
| EXPECT_TRUE(a->lockGroup==b->lockGroup); | ||
| EXPECT_EQ(2, lockGroup->ravels().size()); |
There was a problem hiding this comment.
This test does not exercise the joinLockGroup method, nor does it test that the fix works, which would be by setting calipers on one Ravel and checking that the two Ravel states are identical.
There was a problem hiding this comment.
Updated the test to actually call joinLockGroup() method and verify state synchronization. The test now:
- Adds ravels to the model so
joinLockGroup()can find them viarecursiveDo - Creates a lock group with ravel
aand sets caliper state on it - Calls
b->joinLockGroup(colour)to join the existing group - Verifies both group membership and that caliper settings (displayFilterCaliper, minLabel, maxLabel) are synchronized between the ravels
Fixed in commit 72b963e.
…zation Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
|
@CodeRabbit review |
There was a problem hiding this comment.
Pull Request Overview
Fixes a bug where ravels joining existing lock groups via the context menu were not synchronizing their state with other group members. The solution adds a single line to broadcast the lock group's state to all members after a ravel joins.
- Adds
initialBroadcast()call injoinLockGroup()method to synchronize state - Updates test to verify state synchronization using caliper settings
- Ensures consistent behavior with existing
lockRavelsInSelection()functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| model/ravelWrap.cc | Adds state synchronization call after joining lock group |
| test/testLockGroup.cc | Adds comprehensive test for join functionality and state broadcast |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/testLockGroup.cc (1)
99-138: Test correctly verifies the state broadcast fix.The test properly exercises
joinLockGroup()and verifies that caliper state (displayFilterCaliper, minLabel, maxLabel) is synchronized from ravelato ravelbwhenbjoins the lock group. This addresses the previous review feedback and validates the fix in model/ravelWrap.cc.Optional: Consider asserting that state was successfully set before the join.
While the current guards (
if (!aState.handleStates.empty())) prevent test failures when ravels lack handles, you could add an assertion after line 121 to ensure the test actually exercises the synchronization logic:// After line 121 ASSERT_FALSE(aState.handleStates.empty()) << "Test requires ravels with handles";This ensures the test doesn't pass vacuously if ravels unexpectedly have no handles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
model/ravelWrap.cc(1 hunks)test/testLockGroup.cc(1 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). (6)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (1)
model/ravelWrap.cc (1)
596-596: LGTM! State synchronization fix is correct.The addition of
initialBroadcast()after adding the ravel to the lock group ensures that the newly joined ravel receives the group's current state (calipers, slice labels, etc.). This follows the same pattern used inCanvas::lockRavelsInSelection()and correctly addresses the issue where joined ravels were not synchronizing state.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/testLockGroup.cc (1)
134-135: Remove unused variable.Variable
bStateis declared on line 134 but never used. Line 135 callsb->getState()directly instead.Apply this diff to remove the dead code:
- // Verify state was broadcast - check if b got a's caliper settings - auto bState=b->getState(); - EXPECT_TRUE(b->getState()==aState); + // Verify state was broadcast - check if b got a's caliper settings + EXPECT_TRUE(b->getState()==aState);Alternatively, use the variable:
// Verify state was broadcast - check if b got a's caliper settings auto bState=b->getState(); - EXPECT_TRUE(b->getState()==aState); + EXPECT_TRUE(bState==aState);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/testLockGroup.cc(3 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). (4)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (2)
test/testLockGroup.cc (2)
20-20: LGTM! Fixture changes support model registration.The include and fixture modifications correctly enable the new test to register ravels with the model, which is necessary for
joinLockGroup()to locate them viarecursiveDo.Also applies to: 35-36
99-136: Test correctly verifies the fix.The test properly exercises
joinLockGroup()and validates state synchronization through caliper settings, addressing the previous review feedback. The test structure is sound: it registers ravels with the model, creates a lock group with distinguishable state, callsjoinLockGroup(), and verifies both group membership and state broadcast.
Ravel::joinLockGroup()doesn't callinitialBroadcast()after adding ravel to lock grouplockGroup->initialBroadcast()call injoinLockGroup()method (line 596 in model/ravelWrap.cc)joinLockGroup()method and verify state synchronization with calipersCanvas::lockRavelsInSelection()Summary
The issue was that when a Ravel joins an existing lock group via the context menu (using
joinLockGroup()), the state from that lock group was not being broadcast to the newly joined ravel. This meant the ravel would join the group but not synchronize its state with the other ravels in the group.The fix adds a single line calling
initialBroadcast()after adding the ravel to the lock group, which propagates the group's state to all members including the newly joined ravel. This matches the behavior oflockRavelsInSelection()which already callsinitialBroadcast()after creating/updating lock groups.The test now:
joinLockGroup()can find themjoinLockGroup()on the second ravel to join the groupOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
This change is
Summary by CodeRabbit
Bug Fixes
Tests