Ctrl + Click and Drag to copy instance #2072
Conversation
WalkthroughThe changes enhance the Changes
Sequence DiagramsequenceDiagram
participant User
participant QtInstance
participant Context
User->>QtInstance: Press Ctrl + Mouse Click
QtInstance->>Context: duplicate_instance()
Context-->>QtInstance: Create new instance
User->>QtInstance: Move duplicated instance
QtInstance->>Context: Update instance points
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)sleap/gui/widgets/video.py (5)
The code correctly implements Ctrl-click detection for instance duplication, following the existing event handling patterns.
The implementation correctly handles Ctrl-click for duplication while maintaining the default selection behavior.
The code correctly restricts instance movement to only when selected and either Ctrl or Alt is pressed, preventing accidental movements.
The mouseReleaseEvent properly cleans up by removing the movable flag, updating points/box, and releasing the mouse grab.
Let's ensure there are no conflicting Ctrl+click handlers that might interfere with this new functionality. ✅ Verification successfulBased on the search results, I can generate the final response as we have sufficient information about Ctrl+click handlers in the codebase. No conflicting Ctrl+click handlers found The search results show that the Ctrl+click functionality for instance duplication is properly documented and doesn't conflict with other mouse event handlers. The documentation in
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other Ctrl+click handlers
echo "Searching for Qt.ControlModifier usage in mouse events:"
rg -A 5 "Qt\.ControlModifier.*mousePressEvent"
echo -e "\nSearching for Ctrl+click related comments:"
rg -i "ctrl.*click|control.*click"
Length of output: 711 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2072 +/- ##
===========================================
+ Coverage 75.43% 76.04% +0.60%
===========================================
Files 134 134
Lines 24749 24820 +71
===========================================
+ Hits 18670 18874 +204
+ Misses 6079 5946 -133 ☔ View full report in Codecov by Sentry. |
![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)
sleap/gui/widgets/video.py (1)
2128-2132: Simplify nested if statementsThe nested if statements can be combined using a logical AND operator for better readability.
- def mousePressEvent(self, event): - if event.buttons() == Qt.LeftButton: - if event.modifiers() == Qt.ControlModifier: - self._duplicate_instance() + def mousePressEvent(self, event): + if event.buttons() == Qt.LeftButton and event.modifiers() == Qt.ControlModifier: + self._duplicate_instance()🧰 Tools
🪛 Ruff (0.8.2)
2129-2130: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/widgets/video.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/widgets/video.py
2129-2130: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (2)
sleap/gui/widgets/video.py (2)
2169-2185: LGTM: Mouse event handlers are well implemented
The mouse move and release handlers properly manage the instance state and cleanup:
- Move handler correctly checks for both Ctrl key and movable flag
- Release handler properly cleans up all states and updates the instance
2128-2185: Verify interaction with existing shortcuts and mouse operations
The new Ctrl + Click and Drag feature should be tested for potential conflicts with:
- Other Ctrl + mouse combinations in the application
- Existing instance manipulation shortcuts
- Different operating systems' standard Ctrl + mouse behaviors
✅ Verification successful
Based on the search results, I can now generate the final response as there's enough information to verify potential conflicts:
No conflicts found with Ctrl + Click and Drag implementation
The codebase search reveals that the new Ctrl + Click and Drag feature is well-isolated and doesn't conflict with existing shortcuts:
- The only other Ctrl modifier usage is in
app.pyfor menu-related functionality - Existing mouse handlers in the same class handle different scenarios:
- Regular click and drag for moving nodes
- Shift modifier for marking points as complete
- Right-click for instance selection
The implementation follows Qt's event handling patterns and properly integrates with the existing mouse event chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other keyboard/mouse combinations that might conflict
# Look for other Ctrl key handlers
echo "Checking for other Ctrl key handlers..."
rg -A 5 "Qt\.ControlModifier"
# Look for other mouse event handlers that might conflict
echo "Checking for other mouse event handlers..."
rg -A 5 "mousePressEvent|mouseMoveEvent|mouseReleaseEvent"
Length of output: 11493
🧰 Tools
🪛 Ruff (0.8.2)
2129-2130: Use a single if statement instead of nested if statements
(SIM102)
sleap/gui/widgets/video.py
Outdated
| def _duplicate_instance(self): | ||
| """Duplicate the instance and add it to the scene.""" | ||
| # Add instance to the context | ||
| context = self.player.context | ||
| context.newInstance(copy_instance=self.instance) | ||
|
|
||
| # Find the new instance and its last label | ||
| lf = context.labels.find( | ||
| context.state["video"], context.state["frame_idx"], return_new=True | ||
| )[0] | ||
| new_instance = lf.instances[-1] | ||
|
|
||
| # Select the duplicated QtInstance object | ||
| self.player.state["instance"] = new_instance | ||
|
|
||
| # Refresh the plot | ||
| self.player.plot() | ||
|
|
||
| def on_selection_update(): | ||
| """Callback to set the new QtInstance to be movable.""" | ||
| # Find the QtInstance corresponding to the newly created instance | ||
| for qt_inst in self.player.view.all_instances: | ||
| if qt_inst.instance == new_instance: | ||
| self.player.view.updatedSelection.disconnect(on_selection_update) | ||
|
|
||
| # Set this QtInstance to be movable | ||
| qt_inst.setFlag(QGraphicsItem.ItemIsMovable) | ||
|
|
||
| # Optionally grab the mouse and change cursor, so user can immediately drag | ||
| qt_inst.setCursor(Qt.ClosedHandCursor) | ||
| qt_inst.grabMouse() | ||
| break | ||
|
|
||
| self.player.view.updatedSelection.connect(on_selection_update) | ||
| self.player.view.updatedSelection.emit() | ||
|
|
There was a problem hiding this comment.
Add error handling for context operations
The instance duplication logic should handle potential failures in context operations and instance creation. Also, ensure callback cleanup if the new instance is not found.
def _duplicate_instance(self):
"""Duplicate the instance and add it to the scene."""
+ if not self.player.context:
+ return
+
# Add instance to the context
context = self.player.context
- context.newInstance(copy_instance=self.instance)
+ try:
+ context.newInstance(copy_instance=self.instance)
+ except Exception as e:
+ print(f"Failed to duplicate instance: {e}")
+ return
# Find the new instance and its last label
lf = context.labels.find(
context.state["video"], context.state["frame_idx"], return_new=True
)[0]
+ if not lf or not lf.instances:
+ return
new_instance = lf.instances[-1]
# Select the duplicated QtInstance object
self.player.state["instance"] = new_instance
# Refresh the plot
self.player.plot()
+ # Track if callback was connected for cleanup
+ callback_connected = False
def on_selection_update():
"""Callback to set the new QtInstance to be movable."""
# Find the QtInstance corresponding to the newly created instance
for qt_inst in self.player.view.all_instances:
if qt_inst.instance == new_instance:
self.player.view.updatedSelection.disconnect(on_selection_update)
+ nonlocal callback_connected
+ callback_connected = True
# Set this QtInstance to be movable
qt_inst.setFlag(QGraphicsItem.ItemIsMovable)
# Optionally grab the mouse and change cursor, so user can immediately drag
qt_inst.setCursor(Qt.ClosedHandCursor)
qt_inst.grabMouse()
break
self.player.view.updatedSelection.connect(on_selection_update)
self.player.view.updatedSelection.emit()
+
+ # Clean up callback if instance was not found
+ if not callback_connected:
+ self.player.view.updatedSelection.disconnect(on_selection_update)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _duplicate_instance(self): | |
| """Duplicate the instance and add it to the scene.""" | |
| # Add instance to the context | |
| context = self.player.context | |
| context.newInstance(copy_instance=self.instance) | |
| # Find the new instance and its last label | |
| lf = context.labels.find( | |
| context.state["video"], context.state["frame_idx"], return_new=True | |
| )[0] | |
| new_instance = lf.instances[-1] | |
| # Select the duplicated QtInstance object | |
| self.player.state["instance"] = new_instance | |
| # Refresh the plot | |
| self.player.plot() | |
| def on_selection_update(): | |
| """Callback to set the new QtInstance to be movable.""" | |
| # Find the QtInstance corresponding to the newly created instance | |
| for qt_inst in self.player.view.all_instances: | |
| if qt_inst.instance == new_instance: | |
| self.player.view.updatedSelection.disconnect(on_selection_update) | |
| # Set this QtInstance to be movable | |
| qt_inst.setFlag(QGraphicsItem.ItemIsMovable) | |
| # Optionally grab the mouse and change cursor, so user can immediately drag | |
| qt_inst.setCursor(Qt.ClosedHandCursor) | |
| qt_inst.grabMouse() | |
| break | |
| self.player.view.updatedSelection.connect(on_selection_update) | |
| self.player.view.updatedSelection.emit() | |
| def _duplicate_instance(self): | |
| """Duplicate the instance and add it to the scene.""" | |
| if not self.player.context: | |
| return | |
| # Add instance to the context | |
| context = self.player.context | |
| try: | |
| context.newInstance(copy_instance=self.instance) | |
| except Exception as e: | |
| print(f"Failed to duplicate instance: {e}") | |
| return | |
| # Find the new instance and its last label | |
| lf = context.labels.find( | |
| context.state["video"], context.state["frame_idx"], return_new=True | |
| )[0] | |
| if not lf or not lf.instances: | |
| return | |
| new_instance = lf.instances[-1] | |
| # Select the duplicated QtInstance object | |
| self.player.state["instance"] = new_instance | |
| # Refresh the plot | |
| self.player.plot() | |
| # Track if callback was connected for cleanup | |
| callback_connected = False | |
| def on_selection_update(): | |
| """Callback to set the new QtInstance to be movable.""" | |
| # Find the QtInstance corresponding to the newly created instance | |
| for qt_inst in self.player.view.all_instances: | |
| if qt_inst.instance == new_instance: | |
| self.player.view.updatedSelection.disconnect(on_selection_update) | |
| nonlocal callback_connected | |
| callback_connected = True | |
| # Set this QtInstance to be movable | |
| qt_inst.setFlag(QGraphicsItem.ItemIsMovable) | |
| # Optionally grab the mouse and change cursor, so user can immediately drag | |
| qt_inst.setCursor(Qt.ClosedHandCursor) | |
| qt_inst.grabMouse() | |
| break | |
| self.player.view.updatedSelection.connect(on_selection_update) | |
| self.player.view.updatedSelection.emit() | |
| # Clean up callback if instance was not found | |
| if not callback_connected: | |
| self.player.view.updatedSelection.disconnect(on_selection_update) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sleap/gui/widgets/video.py (3)
2128-2129: Expand the docstring for clarity (optional).While the docstring is correct in describing the method, providing more detail on the “click + Ctrl” or “click + Alt” logic in this docstring could help maintain future readability.
2130-2133: Flatten nested conditionals (SIM102).Instead of nesting the “if event.buttons() == Qt.LeftButton” and “if event.modifiers() == Qt.ControlModifier”, you can merge them into a single condition. This eliminates a level of nesting and makes it more concise.
Proposed refactor:
-if event.buttons() == Qt.LeftButton: - if event.modifiers() == Qt.ControlModifier: - self._duplicate_instance() +if event.buttons() == Qt.LeftButton and event.modifiers() == Qt.ControlModifier: + self._duplicate_instance()🧰 Tools
🪛 Ruff (0.8.2)
2130-2131: Use a single
ifstatement instead of nestedifstatements(SIM102)
2134-2181: Consider adding more robust error handling for context operations.Although you already check whether “player.context” is None, unexpected errors might arise during “context.newInstance(copy_instance=self.instance)”. It may be worth wrapping this call in a try-except to handle anomalies gracefully and avoid partial state updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/widgets/video.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/widgets/video.py
2130-2131: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (5)
sleap/gui/widgets/video.py (5)
Line range hint 1890-1904: Looks good; maintains clarity.
The initialization of nodes only if they are visible (or if show_non_visible is set) aligns well with typical design patterns for optional rendering. No immediate correctness or maintainability concerns here.
Line range hint 1905-1914: Edge creation logic is appropriate.
Creating edges only when both nodes exist avoids potential null references or KeyErrors. The approach helps maintain consistent skeleton structure. Code seems correct and clear.
2182-2187: Functional correctness looks fine.
The overridden mouseMoveEvent method checks modifier states for Ctrl or Alt when the instance is already movable. This is correct for the intended feature: allowing instance movement only under these modifiers. No issues here.
2188-2190: Straightforward condition & call to parent.
This snippet ensures that the parent’s mouseMoveEvent is invoked only when the instance is movable and the correct modifier is pressed. Implementation is concise and accurate.
2191-2199: Appropriate finalization upon mouse release.
Disabling movability, updating instance points, and ungrabbing the mouse are all correct steps to ensure the final instance state is consistent once the user finishes the drag.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
sleap/gui/widgets/video.py (1)
2137-2189:⚠️ Potential issueAdd comprehensive error handling
While the implementation includes basic error handling, it could benefit from more comprehensive error handling as suggested in the previous review.
Apply this diff to improve error handling:
def duplicate_instance(self): """Duplicate the instance and add it to the scene.""" # Add instance to the context if self.player.context is None: return # Copy the instance and add it to the context context = self.player.context - context.newInstance(copy_instance=self.instance) + try: + context.newInstance(copy_instance=self.instance) + except Exception as e: + print(f"Failed to duplicate instance: {e}") + return # Find the new instance and its last label lf = context.labels.find( context.state["video"], context.state["frame_idx"], return_new=True )[0] - new_instance = lf.instances[-1] + if not lf or not lf.instances: + return + new_instance = lf.instances[-1] - if not new_instance: - return - # Select the duplicated QtInstance object self.player.state["instance"] = new_instance # Refresh the plot self.player.plot() - # Track if the new instance is connected for cleanu + # Track if the new instance is connected for cleanup callback_connected = False
🧹 Nitpick comments (1)
sleap/gui/widgets/video.py (1)
2131-2136: Simplify nested if statementsThe nested if statements can be simplified for better readability.
- def mousePressEvent(self, event): - """Custom event handler for mouse press.""" - if event.buttons() == Qt.LeftButton: - if event.modifiers() == Qt.ControlModifier: - self.duplicate_instance() + def mousePressEvent(self, event): + """Custom event handler for mouse press.""" + if event.buttons() == Qt.LeftButton and event.modifiers() == Qt.ControlModifier: + self.duplicate_instance()🧰 Tools
🪛 Ruff (0.8.2)
2133-2134: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/guides/gui.md(1 hunks)sleap/gui/widgets/video.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/widgets/video.py
2133-2134: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (3)
docs/guides/gui.md (1)
151-152: Documentation accurately describes the new functionality
The added documentation clearly explains the new Ctrl + Click and Drag feature for duplicating instances, and it's appropriately placed in the Mouse interactions section.
sleap/gui/widgets/video.py (2)
1572-1574: LGTM: Clean implementation of Ctrl + Click handling in QtNode
The implementation properly checks for the Control modifier and integrates well with the existing mouse event handling logic.
2190-2208: LGTM: Clean implementation of mouse event handlers
The mouse event handlers properly manage instance state and movement constraints based on modifier keys.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sleap/gui/widgets/video.py (2)
2131-2136: Simplify nested conditional statementsThe nested
ifstatements can be simplified using a single condition with theandoperator for better readability.- def mousePressEvent(self, event): - """Custom event handler for mouse press.""" - if event.buttons() == Qt.LeftButton: - if event.modifiers() == Qt.ControlModifier: - self.duplicate_instance() + def mousePressEvent(self, event): + """Custom event handler for mouse press.""" + if event.buttons() == Qt.LeftButton and event.modifiers() == Qt.ControlModifier: + self.duplicate_instance()🧰 Tools
🪛 Ruff (0.8.2)
2133-2134: Use a single
ifstatement instead of nestedifstatements(SIM102)
2187-2205: Improve documentation and readability of mouse event handlersThe mouse event handlers could benefit from better documentation and clearer variable names.
def mouseMoveEvent(self, event): - """Custom event handler to emit signal on event.""" + """Handle mouse movement for instance dragging. + + The instance can only be moved when: + 1. It is movable (ItemIsMovable flag is set) + 2. Either Ctrl or Alt modifier key is pressed + + Args: + event: The mouse event containing position and modifier information + """ is_move = self.flags() & QGraphicsItem.ItemIsMovable - is_ctrl_pressed = (event.modifiers() & Qt.ControlModifier) == Qt.ControlModifier - is_alt_pressed = (event.modifiers() & Qt.AltModifier) == Qt.AltModifier + can_move_with_ctrl = (event.modifiers() & Qt.ControlModifier) == Qt.ControlModifier + can_move_with_alt = (event.modifiers() & Qt.AltModifier) == Qt.AltModifier # Only allow moving if the instance is selected - if is_move and (is_ctrl_pressed or is_alt_pressed): + if is_move and (can_move_with_ctrl or can_move_with_alt): super().mouseMoveEvent(event) def mouseReleaseEvent(self, event): - """Custom event handler to emit signal on event.""" + """Handle mouse release after instance movement. + + Performs cleanup after instance movement: + 1. Disables movement flag + 2. Updates instance points + 3. Updates bounding box + 4. Releases mouse grab + + Args: + event: The mouse release event + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/widgets/video.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/widgets/video.py
2133-2134: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (1)
sleap/gui/widgets/video.py (1)
2131-2186: Verify shortcut conflicts and usage
Let's verify that:
- The Ctrl+Click shortcut doesn't conflict with existing shortcuts
- All instance duplication calls are handled consistently
✅ Verification successful
Let me gather more specific information about the Ctrl+Click handlers by checking the actual implementation.
Let me fix the regex and try again with proper multiline search:
Based on the comprehensive search results, I can now provide the final response:
No conflicts found with Ctrl+Click implementation
The implementation of Ctrl+Click for instance duplication is safe and consistent with the codebase:
- The only Ctrl+Click handler is in the new implementation
- Other mouse modifiers in the codebase serve different purposes:
- Alt+Click: for dragging instances
- Shift+Click: for marking points as complete
- No modifier: for regular selection
The feature is well-isolated and doesn't interfere with existing shortcuts or mouse interactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential shortcut conflicts and instance duplication usage
# Check for other Ctrl+Click handlers or shortcuts
echo "Checking for potential shortcut conflicts..."
rg -l "Qt\.ControlModifier.*mousePressEvent|mousePressEvent.*Qt\.ControlModifier"
# Check for all duplicate_instance calls
echo "Checking for duplicate_instance usage..."
rg -l "duplicate_instance"
Length of output: 327
Script:
#!/bin/bash
# Search for mousePressEvent implementations with context
echo "Checking mousePressEvent implementations..."
rg "def mousePressEvent.*\n.*\n.*\n.*\n.*\n" -A 10
# Search for any keyboard/mouse shortcut documentation
echo "Checking for shortcut documentation..."
fd "README|CONTRIBUTING|DOCUMENTATION" --exec rg -l "shortcut|mouse|click|key"
Length of output: 532
Script:
#!/bin/bash
# Search for mousePressEvent implementations with context
echo "Checking mousePressEvent implementations..."
rg "def mousePressEvent" -U -A 10
# Check the content of contributing guide for shortcuts
echo "Checking contributing guide for shortcuts..."
cat docs/CONTRIBUTING.md
# Search for Qt modifier usage in video.py
echo "Checking Qt modifier usage in video.py..."
rg "Qt\." sleap/gui/widgets/video.py -B 2 -A 2
Length of output: 18556
🧰 Tools
🪛 Ruff (0.8.2)
2133-2134: Use a single if statement instead of nested if statements
(SIM102)
sleap/gui/widgets/video.py
Outdated
| def duplicate_instance(self): | ||
| """Duplicate the instance and add it to the scene.""" | ||
| # Add instance to the context | ||
| if self.player.context is None: | ||
| return | ||
|
|
||
| # Copy the instance and add it to the context | ||
| context = self.player.context | ||
| context.newInstance(copy_instance=self.instance) | ||
|
|
||
| # Find the new instance and its last label | ||
| lf = context.labels.find( | ||
| context.state["video"], context.state["frame_idx"], return_new=True | ||
| )[0] | ||
| new_instance = lf.instances[-1] | ||
|
|
||
| # Select the duplicated QtInstance object | ||
| self.player.state["instance"] = new_instance | ||
|
|
||
| # Refresh the plot | ||
| self.player.plot() | ||
|
|
||
| # Track if the new instance is connected for clean up | ||
| callback_connected = False | ||
|
|
||
| def on_selection_update(): | ||
| """Callback to set the new QtInstance to be movable.""" | ||
| # Find the QtInstance corresponding to the newly created instance | ||
| for qt_inst in self.player.view.all_instances: | ||
| if qt_inst.instance == new_instance: | ||
| self.player.view.updatedSelection.disconnect(on_selection_update) | ||
| nonlocal callback_connected | ||
| callback_connected = True | ||
|
|
||
| # Set this QtInstance to be movable | ||
| qt_inst.setFlag(QGraphicsItem.ItemIsMovable) | ||
|
|
||
| # Optionally grab the mouse and change cursor, so user can immediately drag | ||
| qt_inst.setCursor(Qt.ClosedHandCursor) | ||
| qt_inst.grabMouse() | ||
| break | ||
|
|
||
| # Connect the callback to the updatedSelection signal | ||
| self.player.view.updatedSelection.connect(on_selection_update) | ||
| self.player.view.updatedSelection.emit() | ||
|
|
||
| # Clean up callback if QtInstance was not found | ||
| if not callback_connected: | ||
| self.player.view.updatedSelection.disconnect(on_selection_update) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and documentation
The instance duplication implementation could benefit from several improvements:
- The docstring should include Args/Returns sections and document possible exceptions.
- The callback function could be simplified by extracting the instance finding logic.
- Consider adding error handling for instance creation.
def duplicate_instance(self):
- """Duplicate the instance and add it to the scene."""
+ """Duplicate the instance and add it to the scene.
+
+ Creates a copy of the current instance, adds it to the context, and makes it
+ immediately movable for user interaction.
+
+ Returns:
+ None
+
+ Raises:
+ RuntimeError: If context is not available
+ """
# Add instance to the context
if self.player.context is None:
- return
+ raise RuntimeError("Cannot duplicate instance without context")
# Copy the instance and add it to the context
context = self.player.context
- context.newInstance(copy_instance=self.instance)
+ try:
+ context.newInstance(copy_instance=self.instance)
+ except Exception as e:
+ print(f"Failed to duplicate instance: {e}")
+ return
# Find the new instance and its last label
lf = context.labels.find(
context.state["video"], context.state["frame_idx"], return_new=True
)[0]
+ if not lf or not lf.instances:
+ print("Failed to find new instance")
+ return
new_instance = lf.instances[-1]
# Select the duplicated QtInstance object
self.player.state["instance"] = new_instance
# Refresh the plot
self.player.plot()
# Track if the new instance is connected for clean up
callback_connected = False
+ def find_qt_instance(new_instance):
+ """Find the QtInstance corresponding to the given instance."""
+ for qt_inst in self.player.view.all_instances:
+ if qt_inst.instance == new_instance:
+ return qt_inst
+ return None
def on_selection_update():
"""Callback to set the new QtInstance to be movable."""
- # Find the QtInstance corresponding to the newly created instance
- for qt_inst in self.player.view.all_instances:
- if qt_inst.instance == new_instance:
- self.player.view.updatedSelection.disconnect(on_selection_update)
- nonlocal callback_connected
- callback_connected = True
-
- # Set this QtInstance to be movable
- qt_inst.setFlag(QGraphicsItem.ItemIsMovable)
-
- # Optionally grab the mouse and change cursor, so user can immediately drag
- qt_inst.setCursor(Qt.ClosedHandCursor)
- qt_inst.grabMouse()
- break
+ qt_inst = find_qt_instance(new_instance)
+ if qt_inst:
+ self.player.view.updatedSelection.disconnect(on_selection_update)
+ nonlocal callback_connected
+ callback_connected = True
+
+ # Set this QtInstance to be movable
+ qt_inst.setFlag(QGraphicsItem.ItemIsMovable)
+
+ # Optionally grab the mouse and change cursor, so user can immediately drag
+ qt_inst.setCursor(Qt.ClosedHandCursor)
+ qt_inst.grabMouse()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def duplicate_instance(self): | |
| """Duplicate the instance and add it to the scene.""" | |
| # Add instance to the context | |
| if self.player.context is None: | |
| return | |
| # Copy the instance and add it to the context | |
| context = self.player.context | |
| context.newInstance(copy_instance=self.instance) | |
| # Find the new instance and its last label | |
| lf = context.labels.find( | |
| context.state["video"], context.state["frame_idx"], return_new=True | |
| )[0] | |
| new_instance = lf.instances[-1] | |
| # Select the duplicated QtInstance object | |
| self.player.state["instance"] = new_instance | |
| # Refresh the plot | |
| self.player.plot() | |
| # Track if the new instance is connected for clean up | |
| callback_connected = False | |
| def on_selection_update(): | |
| """Callback to set the new QtInstance to be movable.""" | |
| # Find the QtInstance corresponding to the newly created instance | |
| for qt_inst in self.player.view.all_instances: | |
| if qt_inst.instance == new_instance: | |
| self.player.view.updatedSelection.disconnect(on_selection_update) | |
| nonlocal callback_connected | |
| callback_connected = True | |
| # Set this QtInstance to be movable | |
| qt_inst.setFlag(QGraphicsItem.ItemIsMovable) | |
| # Optionally grab the mouse and change cursor, so user can immediately drag | |
| qt_inst.setCursor(Qt.ClosedHandCursor) | |
| qt_inst.grabMouse() | |
| break | |
| # Connect the callback to the updatedSelection signal | |
| self.player.view.updatedSelection.connect(on_selection_update) | |
| self.player.view.updatedSelection.emit() | |
| # Clean up callback if QtInstance was not found | |
| if not callback_connected: | |
| self.player.view.updatedSelection.disconnect(on_selection_update) | |
| def duplicate_instance(self): | |
| """Duplicate the instance and add it to the scene. | |
| Creates a copy of the current instance, adds it to the context, and makes it | |
| immediately movable for user interaction. | |
| Returns: | |
| None | |
| Raises: | |
| RuntimeError: If context is not available | |
| """ | |
| # Add instance to the context | |
| if self.player.context is None: | |
| raise RuntimeError("Cannot duplicate instance without context") | |
| # Copy the instance and add it to the context | |
| context = self.player.context | |
| try: | |
| context.newInstance(copy_instance=self.instance) | |
| except Exception as e: | |
| print(f"Failed to duplicate instance: {e}") | |
| return | |
| # Find the new instance and its last label | |
| lf = context.labels.find( | |
| context.state["video"], context.state["frame_idx"], return_new=True | |
| )[0] | |
| if not lf or not lf.instances: | |
| print("Failed to find new instance") | |
| return | |
| new_instance = lf.instances[-1] | |
| # Select the duplicated QtInstance object | |
| self.player.state["instance"] = new_instance | |
| # Refresh the plot | |
| self.player.plot() | |
| # Track if the new instance is connected for clean up | |
| callback_connected = False | |
| def find_qt_instance(new_instance): | |
| """Find the QtInstance corresponding to the given instance.""" | |
| for qt_inst in self.player.view.all_instances: | |
| if qt_inst.instance == new_instance: | |
| return qt_inst | |
| return None | |
| def on_selection_update(): | |
| """Callback to set the new QtInstance to be movable.""" | |
| qt_inst = find_qt_instance(new_instance) | |
| if qt_inst: | |
| self.player.view.updatedSelection.disconnect(on_selection_update) | |
| nonlocal callback_connected | |
| callback_connected = True | |
| # Set this QtInstance to be movable | |
| qt_inst.setFlag(QGraphicsItem.ItemIsMovable) | |
| # Optionally grab the mouse and change cursor, so user can immediately drag | |
| qt_inst.setCursor(Qt.ClosedHandCursor) | |
| qt_inst.grabMouse() | |
| # Connect the callback to the updatedSelection signal | |
| self.player.view.updatedSelection.connect(on_selection_update) | |
| self.player.view.updatedSelection.emit() | |
| # Clean up callback if QtInstance was not found | |
| if not callback_connected: | |
| self.player.view.updatedSelection.disconnect(on_selection_update) |
sleap/gui/widgets/video.py
Outdated
| # Refresh the plot | ||
| self.player.plot() | ||
|
|
||
| # Track if the new instance is connected for clean up | ||
| callback_connected = False | ||
|
|
||
| def on_selection_update(): | ||
| """Callback to set the new QtInstance to be movable.""" | ||
| # Find the QtInstance corresponding to the newly created instance | ||
| for qt_inst in self.player.view.all_instances: | ||
| if qt_inst.instance == new_instance: | ||
| self.player.view.updatedSelection.disconnect(on_selection_update) | ||
| nonlocal callback_connected | ||
| callback_connected = True | ||
|
|
||
| # Set this QtInstance to be movable | ||
| qt_inst.setFlag(QGraphicsItem.ItemIsMovable) | ||
|
|
||
| # Optionally grab the mouse and change cursor, so user can immediately drag | ||
| qt_inst.setCursor(Qt.ClosedHandCursor) | ||
| qt_inst.grabMouse() | ||
| break | ||
|
|
||
| # Connect the callback to the updatedSelection signal | ||
| self.player.view.updatedSelection.connect(on_selection_update) | ||
| self.player.view.updatedSelection.emit() | ||
|
|
||
| # Clean up callback if QtInstance was not found | ||
| if not callback_connected: | ||
| self.player.view.updatedSelection.disconnect(on_selection_update) | ||
|
|
There was a problem hiding this comment.
Not sure if this is better of worse, but my suggestion here would be:
| # Refresh the plot | |
| self.player.plot() | |
| # Track if the new instance is connected for clean up | |
| callback_connected = False | |
| def on_selection_update(): | |
| """Callback to set the new QtInstance to be movable.""" | |
| # Find the QtInstance corresponding to the newly created instance | |
| for qt_inst in self.player.view.all_instances: | |
| if qt_inst.instance == new_instance: | |
| self.player.view.updatedSelection.disconnect(on_selection_update) | |
| nonlocal callback_connected | |
| callback_connected = True | |
| # Set this QtInstance to be movable | |
| qt_inst.setFlag(QGraphicsItem.ItemIsMovable) | |
| # Optionally grab the mouse and change cursor, so user can immediately drag | |
| qt_inst.setCursor(Qt.ClosedHandCursor) | |
| qt_inst.grabMouse() | |
| break | |
| # Connect the callback to the updatedSelection signal | |
| self.player.view.updatedSelection.connect(on_selection_update) | |
| self.player.view.updatedSelection.emit() | |
| # Clean up callback if QtInstance was not found | |
| if not callback_connected: | |
| self.player.view.updatedSelection.disconnect(on_selection_update) | |
| qt_instance = QtInstance(instance=instance, player=self.player) | |
| self.player.addInstance(instance=qt_instance) | |
| # Set this QtInstance to be movable | |
| qt_instance.setFlag(QGraphicsItem.ItemIsMovable) | |
| # Optionally grab the mouse and change cursor, so user can immediately drag | |
| qt_instance.setCursor(Qt.ClosedHandCursor) | |
| qt_instance.grabMouse() |
There was a problem hiding this comment.
Thanks for the suggestion! I've gave this a try, but the GUI created two QtInstance objects in the GUI and only one of them was movable, which is not what we expected to happen.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
sleap/gui/widgets/video.py (1)
2137-2177:⚠️ Potential issueImprove error handling and callback cleanup
The duplicate_instance method needs better error handling and callback cleanup:
Apply this diff to improve robustness:
def duplicate_instance(self): - """Duplicate the instance and add it to the scene.""" + """Duplicate the instance and add it to the scene. + + Creates a copy of the current instance, adds it to the context, and makes it + immediately movable for user interaction. + + Returns: + None + + Raises: + RuntimeError: If context is not available + """ # Add instance to the context if self.player.context is None: - return + raise RuntimeError("Cannot duplicate instance without context") # Copy the instance and add it to the context context = self.player.context - context.newInstance(copy_instance=self.instance) + try: + context.newInstance(copy_instance=self.instance) + except Exception as e: + print(f"Failed to duplicate instance: {e}") + return # Find the new instance and its last label lf = context.labels.find( context.state["video"], context.state["frame_idx"], return_new=True )[0] + if not lf or not lf.instances: + print("Failed to find new instance") + return new_instance = lf.instances[-1] # Select the duplicated QtInstance object self.player.state["instance"] = new_instance # Refresh the plot self.player.plot() + # Track if the new instance is connected for cleanup + callback_connected = False def on_selection_update(): """Callback to set the new QtInstance to be movable.""" # Find the QtInstance corresponding to the newly created instance for qt_inst in self.player.view.all_instances: if qt_inst.instance == new_instance: self.player.view.updatedSelection.disconnect(on_selection_update) + nonlocal callback_connected + callback_connected = True # Set this QtInstance to be movable qt_inst.setFlag(QGraphicsItem.ItemIsMovable) # Optionally grab the mouse and change cursor, so user can immediately drag qt_inst.setCursor(Qt.ClosedHandCursor) qt_inst.grabMouse() break self.player.view.updatedSelection.connect(on_selection_update) self.player.view.updatedSelection.emit() + + # Clean up callback if instance was not found + if not callback_connected: + self.player.view.updatedSelection.disconnect(on_selection_update)
🧹 Nitpick comments (4)
sleap/gui/widgets/video.py (4)
1572-1574: Improve code clarity and structureThe code can be simplified and made more descriptive:
- # Ctrl-click to duplicate instance - elif event.modifiers() == Qt.ControlModifier: - self.parentObject().mousePressEvent(event) + # Forward Ctrl+Click to parent instance for duplication + elif event.modifiers() == Qt.ControlModifier and event.button() == Qt.LeftButton: + self.parentObject().mousePressEvent(event)
2131-2136: Simplify event handling logicThe mousePressEvent handler can be simplified:
- if event.buttons() == Qt.LeftButton: - if event.modifiers() == Qt.ControlModifier: - self.duplicate_instance() + if event.buttons() == Qt.LeftButton and event.modifiers() == Qt.ControlModifier: + self.duplicate_instance()🧰 Tools
🪛 Ruff (0.8.2)
2133-2134: Use a single
ifstatement instead of nestedifstatements(SIM102)
2178-2187: Improve move event handlingThe mouseMoveEvent logic can be simplified and better documented:
def mouseMoveEvent(self, event): - """Custom event handler to emit signal on event.""" + """Custom event handler for instance movement. + + Only allows movement when the instance is selected and either Ctrl or Alt + is pressed. + """ is_move = self.flags() & QGraphicsItem.ItemIsMovable - is_ctrl_pressed = (event.modifiers() & Qt.ControlModifier) == Qt.ControlModifier - is_alt_pressed = (event.modifiers() & Qt.AltModifier) == Qt.AltModifier + has_modifier = event.modifiers() & (Qt.ControlModifier | Qt.AltModifier) - # Only allow moving if the instance is selected - if is_move and (is_ctrl_pressed or is_alt_pressed): + if is_move and has_modifier: super().mouseMoveEvent(event)
2188-2196: Add documentation for mouse release handlingThe mouseReleaseEvent needs better documentation:
def mouseReleaseEvent(self, event): - """Custom event handler for mouse release.""" + """Custom event handler for mouse release. + + Handles cleanup after instance movement: + 1. Disables movement flag + 2. Updates instance points + 3. Updates bounding box + 4. Releases mouse grab + """ if self.flags() & QGraphicsItem.ItemIsMovable: self.setFlag(QGraphicsItem.ItemIsMovable, False) self.updatePoints(user_change=True) self.updateBox() self.ungrabMouse() super().mouseReleaseEvent(event)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/widgets/video.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/widgets/video.py
2133-2134: Use a single if statement instead of nested if statements
(SIM102)
Description
This PR adds a new feature where a user can hold Ctrl + click to copy instance and drag that copied instance in the current frame of the video.
Types of changes
Does this address any currently open issues?
#1846
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
Summary by CodeRabbit
These updates improve user interaction and efficiency when manipulating skeleton instances within the video player.