Added Three Different Cases for Adding a New Instance #1859
Conversation
WalkthroughThe modifications enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VideoWidget
participant ContextMenu
participant Command
User ->> VideoWidget: Right-click for context menu
VideoWidget ->> ContextMenu: Display options
ContextMenu ->> User: Show "Default" option
User ->> ContextMenu: Select "Default"
ContextMenu ->> VideoWidget: Trigger newInstance(init_method="best", location=scene_pos)
VideoWidget ->> Command: create_new_instance(location=scene_pos)
Command ->> Command: Adjust node positions based on location
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1859 +/- ##
===========================================
+ Coverage 73.30% 74.34% +1.03%
===========================================
Files 134 135 +1
Lines 24087 24755 +668
===========================================
+ Hits 17658 18405 +747
+ Misses 6429 6350 -79 ☔ View full report in Codecov by Sentry. |
…ion of the cursor
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sleap/gui/commands.py (4 hunks)
- sleap/gui/widgets/video.py (1 hunks)
Additional comments not posted (5)
sleap/gui/widgets/video.py (1)
370-373: LGTM! Ensure the usage ofscene_posandnewInstancemethod in the codebase.The code changes are approved.
However, verify the usage of
scene_posandnewInstancemethod to ensure correctness and consistency.Verification successful
The usage of
scene_posandnewInstancemethod is correct and consistent in the codebase.The code changes are verified and approved:
scene_posis properly used as a parameter to thenewInstancemethod in multiple instances withinsleap/gui/widgets/video.py.- The
newInstancemethod insleap/gui/commands.pycorrectly handles thelocationparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `scene_pos` and `newInstance` method in the codebase. # Test: Search for the usage of `scene_pos` in the codebase. Expect: Proper handling of `scene_pos`. rg --type python -A 5 $'scene_pos' # Test: Search for the usage of `newInstance` method in the codebase. Expect: Proper handling of `newInstance` with `location` parameter. rg --type python -A 5 $'newInstance'Length of output: 131
Script:
#!/bin/bash # Description: Verify the usage of `scene_pos` and `newInstance` method in the codebase. # Test: Search for the usage of `scene_pos` in the codebase. Expect: Proper handling of `scene_pos`. rg -A 5 'scene_pos' # Test: Search for the usage of `newInstance` method in the codebase. Expect: Proper handling of `newInstance` with `location` parameter. rg -A 5 'newInstance'Length of output: 6976
sleap/gui/commands.py (4)
2916-2916: Verify the addition of thelocationparameter.The addition of the
locationparameter to the function signature appears correct. Ensure that all calls tocreate_new_instancein the codebase are updated to handle this new parameter.
2988-2988: Verify the handling of thelocationparameter.The handling of the
locationparameter appears correct. The logic for setting node positions based on thelocationoffset is sound. Ensure thatlocationis properly passed and utilized in all relevant scenarios.
3015-3015: Verify the addition of thelocationparameter.The addition of the
locationparameter to the function signature appears correct. Ensure that all calls toset_visible_nodesin the codebase are updated to handle this new parameter.
3030-3038: Verify the logic forlocationbased node positioning.The logic for adjusting node positions based on the
locationoffset appears correct. Ensure that thelocationparameter is properly passed and utilized in all relevant scenarios.
There was a problem hiding this comment.
The current implementation will add (a scaled) +10 offset even if we are in the third case
- Right click and click "Default" to create instance should make the instance based on the location of the click
or the unmentioned fourth case
- Neither cases 1-3
We want to add an unscaled +10 offset on in the first case:
- Ctrl + I should create instances slightly away from the copied instance
And also handle the fourth case better (i.e. 0 offset). See suggestions/comments
| reference_node = next( | ||
| (node for node in copy_instance if not node.isnan()), None | ||
| ) |
There was a problem hiding this comment.
Finds first non nan reference node
| if x_new_offset < 0: | ||
| x_new = 0 | ||
| visible = False | ||
| elif x_new_offset > new_size_width: | ||
| x_new = new_size_width | ||
| visible = False | ||
| else: | ||
| x_new = x_new_offset | ||
| if y_new_offset < 0: | ||
| y_new = 0 | ||
| visible = False | ||
| elif y_new_offset > new_size_height: | ||
| y_new = new_size_height | ||
| visible = False | ||
| else: | ||
| y_new = y_new_offset |
There was a problem hiding this comment.
Moves out of bounds nodes to closest in bounds position. Also marks those nodes that were going to be out of bounds as not visible.
Description
Types of changes
Does this address any currently open issues?
This PR is an expansion of PR #1816.
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
locationfor node positions, enhancing control over instance initialization and visibility settings.location.