[GSoC'24] Registeration of Shortcuts via modules and dummy conflict detector #8007
Conversation
…e level shortcut registeration
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates to the CVAT UI introduce a comprehensive system for managing keyboard shortcuts across various components. The changes include the addition of a new action type Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant Hook as useRegisterShortcuts
participant Store as CVAT Store
participant Actions as Shortcuts Actions
participant Reducer as Shortcuts Reducer
Component->>Hook: Define componentShortcuts
Hook->>Store: Get CVAT Store
Hook->>Actions: Dispatch registerShortcuts
Actions->>Reducer: Handle REGISTER_SHORTCUT
Reducer->>Store: Update key mappings
Store-->>Component: Shortcuts registered
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 (
|
tahamukhtar20
changed the title
…registerShortcuts to accept more than one shortcuts
…t the mousetrap to how the code was before with nessessary changes.
tahamukhtar20
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (30)
- cvat-ui/src/actions/shortcuts-actions.ts (1 hunks)
- cvat-ui/src/components/annotation-page/attribute-annotation-workspace/attribute-annotation-sidebar/attribute-annotation-sidebar.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/canvas/views/canvas3d/canvas-wrapper3D.tsx (1 hunks)
- cvat-ui/src/components/annotation-page/review-workspace/controls-side-bar/issue-control.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/controls-side-bar.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/cursor-control.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/group-control.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/join-control.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/merge-control.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/slice-control.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/split-control.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/labels-list.tsx (1 hunks)
- cvat-ui/src/components/annotation-page/standard3D-workspace/controls-side-bar/controls-side-bar.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/tag-annotation-workspace/tag-annotation-sidebar/tag-annotation-sidebar.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/top-bar/left-group.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/top-bar/player-buttons.tsx (2 hunks)
- cvat-ui/src/components/annotation-page/top-bar/player-navigation.tsx (2 hunks)
- cvat-ui/src/components/customizable-components/index.tsx (1 hunks)
- cvat-ui/src/components/header/header.tsx (2 hunks)
- cvat-ui/src/components/labels-editor/skeleton-configurator.tsx (2 hunks)
- cvat-ui/src/components/layout-grid/layout-grid.tsx (1 hunks)
- cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/objects-list.tsx (2 hunks)
- cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx (2 hunks)
- cvat-ui/src/cvat-store.ts (2 hunks)
- cvat-ui/src/reducers/shortcuts-reducer.ts (2 hunks)
- cvat-ui/src/utils/enums.tsx (1 hunks)
- cvat-ui/src/utils/hooks.ts (2 hunks)
- cvat-ui/src/utils/mousetrap-react.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
- cvat-ui/src/utils/enums.tsx
Additional comments not posted (49)
cvat-ui/src/actions/shortcuts-actions.ts (2)
11-11: The addition ofREGISTER_SHORTCUTtoShortcutsActionsTypesaligns well with the PR's objectives to enhance shortcut management.
18-20: The implementation ofregisterShortcutsaction creator is correct and follows the established patterns in the codebase.cvat-ui/src/cvat-store.ts (1)
24-45: The modification to accept arootReducerCreatorfunction increateCVATStoreenhances flexibility in configuring the rootReducer, which is beneficial for scenarios like dynamic module loading.cvat-ui/src/components/customizable-components/index.tsx (1)
25-25: The registration of theSAVE_JOBshortcut within the component is implemented correctly, ensuring that the shortcut is active only when the component is rendered.cvat-ui/src/utils/mousetrap-react.tsx (1)
14-14: The addition of theviewproperty to theKeyMapIteminterface enhances the clarity and functionality of shortcut definitions by specifying the context in which they are applicable.cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/cursor-control.tsx (2)
16-16: The use ofuseRegisterShortcutsandViewTypeis correctly implemented and aligns with the PR's objectives to enhance shortcut management.Also applies to: 39-39
30-37: The definition ofcomponentShortcutsfor the cursor control is clear and well-documented, enhancing the usability and accessibility of the component.cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/join-control.tsx (2)
14-14: The use ofuseRegisterShortcutsandViewTypeis correctly implemented and aligns with the PR's objectives to enhance shortcut management.Also applies to: 38-38
29-36: The definition ofcomponentShortcutsfor the join control is clear and well-documented, enhancing the usability and accessibility of the component.cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/split-control.tsx (2)
16-16: The use ofuseRegisterShortcutsandViewTypeis correctly implemented and aligns with the PR's objectives to enhance shortcut management.Also applies to: 40-40
31-37: The definition ofcomponentShortcutsfor the split control is clear and well-documented, enhancing the usability and accessibility of the component.cvat-ui/src/components/annotation-page/review-workspace/controls-side-bar/issue-control.tsx (2)
14-15: The imports forViewTypeanduseRegisterShortcutsare correctly placed and used.
30-39: The shortcut registration for 'Open an issue' is implemented correctly. Ensure that the shortcut 'n' does not conflict with other global shortcuts.Verification successful
To verify if the shortcut 'n' conflicts with other global shortcuts, we need to search for the sequence 'n' in all TypeScript and JavaScript files. Let's adjust the script to search for the pattern in these file types.
To verify if the shortcut 'n' conflicts with other global shortcuts, we need to correct the syntax error in the script and re-run it.
To verify if the shortcut 'n' conflicts with other global shortcuts, we need to correct the syntax error in the script by ensuring all quotes are properly matched and re-run it.
To verify if the shortcut 'n' conflicts with other global shortcuts, we need to correct the syntax error by using double quotes for the entire pattern and re-run the script.
The search results indicate that the shortcut 'n' is only used in the
issue-control.tsxfile and does not conflict with other global shortcuts.
- cvat-ui/src/components/annotation-page/review-workspace/controls-side-bar/issue-control.tsx:
sequences: ['n']Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the shortcut 'n' conflicts with other global shortcuts. # Test: Search for the shortcut 'n' in other components. Expect: No other global usage of 'n'. rg --type tsx $'sequences: \['n'\]'Length of output: 62
Script:
#!/bin/bash # Description: Verify if the shortcut 'n' conflicts with other global shortcuts. # Test: Search for the shortcut 'n' in other components. Expect: No other global usage of 'n'. rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' 'sequences: \[\'n\'\]'Length of output: 93
Script:
#!/bin/bash # Description: Verify if the shortcut 'n' conflicts with other global shortcuts. # Test: Search for the shortcut 'n' in other components. Expect: No other global usage of 'n'. rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' 'sequences: \[\'n\'\]'Length of output: 93
Script:
#!/bin/bash # Description: Verify if the shortcut 'n' conflicts with other global shortcuts. # Test: Search for the shortcut 'n' in other components. Expect: No other global usage of 'n'. rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' 'sequences: \[\'n\'\]'Length of output: 93
Script:
#!/bin/bash # Description: Verify if the shortcut 'n' conflicts with other global shortcuts. # Test: Search for the shortcut 'n' in other components. Expect: No other global usage of 'n'. rg --type-add "ts:*.{ts,tsx}" --type-add "js:*.{js,jsx}" "sequences: \['n'\]"Length of output: 203
cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/merge-control.tsx (2)
15-16: Imports foruseRegisterShortcutsandViewTypeare correctly implemented.
31-40: Shortcut registration for 'Merge mode' is correctly implemented. Verify that the shortcut 'm' does not conflict with other shortcuts.Verification successful
Shortcut 'm' for 'Merge mode' does not conflict with other shortcuts.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the shortcut 'm' conflicts with other shortcuts. # Test: Search for the shortcut 'm' in other components. Expect: No other usage of 'm'. rg --type tsx $'sequences: \['m'\]'Length of output: 62
Script:
#!/bin/bash # Description: Verify if the shortcut 'm' conflicts with other shortcuts. # Test: Search for the shortcut 'm' in all TypeScript and JavaScript files. Expect: No other usage of 'm'. rg --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' $'sequences: \[\'m\'\]'Length of output: 209
cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/slice-control.tsx (1)
15-16: Correct implementation of imports foruseRegisterShortcutsandViewType.cvat-ui/src/reducers/shortcuts-reducer.ts (1)
60-78: The handling of theREGISTER_SHORTCUTaction is correctly implemented. Ensure that the error handling for shortcut conflicts is robust and user-friendly.cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/group-control.tsx (3)
15-16: Ensure that the new imports are used appropriately in the file.
36-49: The shortcut registration for 'Group mode' and 'Reset group' is well-defined and aligns with the new system architecture.
51-51: TheuseRegisterShortcutshook is correctly placed outside of the component to avoid unnecessary re-registrations on re-renders.cvat-ui/src/utils/hooks.ts (1)
137-141: The implementation ofuseRegisterShortcutscorrectly utilizes the Redux store to dispatch actions, which is a clean and effective way to handle global state changes related to shortcuts.cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/labels-list.tsx (3)
16-17: Ensure that the new imports are used appropriately in the file.
19-26: The shortcut registration for 'Switch label' is well-defined and aligns with the new system architecture.
28-28: TheuseRegisterShortcutshook is correctly placed outside of the component to avoid unnecessary re-registrations on re-renders.cvat-ui/src/components/annotation-page/top-bar/left-group.tsx (3)
19-20: Ensure that the new imports are used appropriately in the file.
40-53: The shortcut registration for 'Undo action' and 'Redo action' is well-defined and aligns with the new system architecture.
55-55: TheuseRegisterShortcutshook is correctly placed outside of the component to avoid unnecessary re-registrations on re-renders.cvat-ui/src/components/annotation-page/standard3D-workspace/controls-side-bar/controls-side-bar.tsx (1)
32-33: Ensure proper testing of theuseRegisterShortcutshook integration.Also applies to: 65-65
cvat-ui/src/components/annotation-page/top-bar/player-navigation.tsx (2)
47-61: Validate the shortcut key sequences for potential conflicts or overlaps.Verification successful
The search for shortcut key sequences did not yield any conflicting results. The only relevant match found was a single instance of
shortcuts.sequencesin the codebase, which does not indicate any conflicts.
- No conflicts or overlaps were found with the existing shortcut key sequences.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the shortcut key sequences do not conflict with existing ones. # Test: Search for all shortcut key sequences in the project to check for conflicts. rg --type typescript "sequences:" "src/"Length of output: 73
Script:
#!/bin/bash # Description: Verify that the shortcut key sequences do not conflict with existing ones. # Test: Search for all shortcut key sequences in the project to check for conflicts. rg --type ts "sequences:" "src/"Length of output: 75
Script:
#!/bin/bash # Description: Locate TypeScript files and verify that the shortcut key sequences do not conflict with existing ones. # Step 1: Locate TypeScript files to determine the correct directory structure. fd --extension ts # Step 2: Search for all shortcut key sequences in the located directories to check for conflicts. fd --extension ts --exec rg "sequences:"Length of output: 6719
Script:
#!/bin/bash # Description: Locate TypeScript files and verify that the shortcut key sequences do not conflict with existing ones. # Step 1: Locate TypeScript files to determine the correct directory structure. fd --extension ts # Step 2: Search for all shortcut key sequences in the located directories to check for conflicts. fd --extension ts --exec rg "sequences"Length of output: 6812
22-23: Ensure proper testing of theuseRegisterShortcutshook integration.Also applies to: 63-63
cvat-ui/src/components/annotation-page/tag-annotation-workspace/tag-annotation-sidebar/tag-annotation-sidebar.tsx (1)
31-32: Ensure proper testing of theuseRegisterShortcutshook integration.Also applies to: 92-92
cvat-ui/src/components/annotation-page/top-bar/player-buttons.tsx (1)
51-101: Validate the shortcut key sequences for potential conflicts or overlaps.cvat-ui/src/components/annotation-page/attribute-annotation-workspace/attribute-annotation-sidebar/attribute-annotation-sidebar.tsx (3)
26-27: The import ofuseRegisterShortcutsandViewTypealigns with the PR's objective to enhance shortcut management. Ensure that these utilities are used consistently across other components if applicable.
55-110: Introduced a comprehensive set of keyboard shortcuts for various actions within the attribute annotation sidebar. This is a significant enhancement for accessibility and efficiency. Ensure that these shortcuts do not conflict with existing global shortcuts and are documented in the user guide.
112-112: TheuseRegisterShortcutshook is correctly used to register the defined shortcuts. It's crucial to ensure that this hook is called in the appropriate lifecycle method or effect to avoid potential issues with re-registration on every render.cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/controls-side-bar.tsx (1)
16-17: The import ofuseRegisterShortcutsandViewTypeis consistent with the changes in other components, supporting the PR's theme of enhanced shortcut management.cvat-ui/src/components/header/header.tsx (3)
44-46: The import ofuseRegisterShortcuts,useIsMounted, andViewTypeis consistent with the changes in other components, supporting the PR's theme of enhanced shortcut management.
76-89: Introduced keyboard shortcuts for toggling the shortcuts and settings modals. This is a useful feature for enhancing accessibility. Ensure these shortcuts are documented and do not conflict with other global shortcuts.Verification successful
The search results indicate that the newly introduced shortcuts (
f1andf2) do not conflict with any existing shortcuts in the codebase. Therefore, the new shortcuts for toggling the shortcuts and settings modals are unique and should not cause any issues.
- No conflicts found with the
f1andf2shortcuts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the uniqueness of newly introduced shortcuts. # Test: Search for potential conflicts in shortcut definitions across the project. rg --type tsx $'sequences:'Length of output: 54
Script:
#!/bin/bash # Description: Verify the uniqueness of newly introduced shortcuts. # Test: Search for potential conflicts in shortcut definitions across the project. rg --type typescript $'sequences:'Length of output: 68
Script:
#!/bin/bash # Description: Verify the uniqueness of newly introduced shortcuts. # Test: Search for potential conflicts in shortcut definitions across the project. rg 'sequences:'Length of output: 9252
91-91: TheuseRegisterShortcutshook is correctly used to register the defined shortcuts. It's crucial to ensure that this hook is called in the appropriate lifecycle method or effect to avoid potential issues with re-registration on every render.cvat-ui/src/containers/annotation-page/standard-workspace/objects-side-bar/objects-list.tsx (3)
32-33: Integration ofuseRegisterShortcutshook.The import and usage of
useRegisterShortcutsfromutils/hooksaligns with the PR's objective to manage shortcut registration through a centralized hook. This modular approach enhances maintainability and reusability.
72-169: Comprehensive shortcut definitions for object management.The
componentShortcutsobject comprehensively defines various shortcuts for object manipulation within the annotation workspace. Each shortcut is well-documented with a name, description, sequences, and a view type. This structured approach not only enhances user interaction but also maintains consistency across the application.
171-171: Proper usage of theuseRegisterShortcutshook.The
useRegisterShortcutshook is correctly utilized to register the defined shortcuts at the component level. This ensures that the shortcuts are active and available when the component is rendered, aligning with the lifecycle of the React component.cvat-ui/src/components/annotation-page/single-shape-workspace/single-shape-sidebar/single-shape-sidebar.tsx (2)
32-33: Integration ofViewTypeenum anduseRegisterShortcutshook.This change aligns with the PR's objective to enhance shortcut management. Ensure that the
ViewTypeenum anduseRegisterShortcutsfunction are properly defined and exported in their respective modules.
134-151: Definition and registration of component-specific shortcuts.The shortcuts for 'Draw mode' and 'Cancel' are well-defined with clear names, descriptions, and sequences. This should enhance user accessibility and efficiency. Ensure that the
ViewType.ALLis appropriate for these shortcuts and that it doesn't lead to unintended behavior in other parts of the application.cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx (2)
45-46: Integration ofViewTypeenum anduseRegisterShortcutshook.This change is consistent with the PR's goal to manage shortcuts more effectively. Verify that the
ViewTypeenum anduseRegisterShortcutsfunction are correctly implemented and available for use as intended.
103-113: Definition and registration of a component-specific shortcut for switching tools blocker state.The shortcut for 'Switch algorithm blocker' is defined with a clear name, description, and sequence. This addition should help in managing the interaction tools more efficiently. Confirm that the
ViewType.ALLsetting is suitable for this shortcut and does not interfere with other functionalities.cvat-ui/src/components/annotation-page/canvas/views/canvas3d/canvas-wrapper3D.tsx (1)
37-38: Ensure proper shortcut registration and usage.The shortcut definitions for camera controls are well-defined with clear names and descriptions. The use of
ViewType.ALLensures these shortcuts are globally available, which is appropriate for these actions. The registration of these shortcuts usinguseRegisterShortcutsis correctly implemented.Also applies to: 42-102, 105-105
cvat-ui/src/components/labels-editor/skeleton-configurator.tsx (1)
24-25: Ensure proper shortcut registration and usage.The shortcut for canceling skeleton edge drawing is well-defined with a clear name and description. The use of
ViewType.ALLensures this shortcut is globally available, which is appropriate for this action. The registration of this shortcut usinguseRegisterShortcutsis correctly implemented.Also applies to: 52-61, 61-61
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (1)
62-63: Integrate new imports effectively.The imports for
ViewTypeanduseRegisterShortcutsare correctly placed and follow the project's conventions for organizing imports.
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx
Outdated
Show resolved
Hide resolved
cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/cursor-control.tsx
Outdated
Show resolved
Hide resolved
...ui/src/components/annotation-page/standard-workspace/controls-side-bar/controls-side-bar.tsx
Outdated
Show resolved
Hide resolved
.../src/components/annotation-page/standard3D-workspace/controls-side-bar/controls-side-bar.tsx
Outdated
Show resolved
Hide resolved
...s/annotation-page/tag-annotation-workspace/tag-annotation-sidebar/tag-annotation-sidebar.tsx
Outdated
Show resolved
Hide resolved
cvat-ui/src/components/annotation-page/top-bar/player-buttons.tsx
Outdated
Show resolved
Hide resolved
|
There are plenty failed end to end tests with this pull request It is expected that nothing is changed for a user with this patch. So, I do not think you need to fix test. Probably there are some issues with the implementation. |
|
Example: |
|
...attribute-annotation-workspace/attribute-annotation-sidebar/attribute-annotation-sidebar.tsx
Show resolved
Hide resolved
|
Hello, Not all the shortcuts are transformed with this patch (because not all were initially defined in shortcuts reducer). For example in file On one hand, if we want to avoid shortcuts conflicts, using common design, now we have to declare these shortcuts globally as well (register in storage) (probably with specific scope (e.g. On the other hand, I do not think we want to be able to modify such kind of shortcuts in the future (and even list them in total shortcuts list). So, maybe we should introduce Alternatively, we may unify these shortcuts, like: and only provide correct handlers in attribute annotation sidebar component. From first look, I like the second option more. As I remember, there is similar problem in tag annotation mode (this file: cvat-ui/src/components/annotation-page/tag-annotation-workspace/tag-annotation-sidebar/shortcuts-select.tsx). In this component, I believe it make sense to be able to see and configure these shortcuts (again, with specific scope, e.g. |
...tation-page/attribute-annotation-workspace/attribute-annotation-sidebar/attribute-editor.tsx
Outdated
Show resolved
Hide resolved
...ponents/annotation-page/tag-annotation-workspace/tag-annotation-sidebar/shortcuts-select.tsx
Outdated
Show resolved
Hide resolved
|
LGTM, let's wait for CI. We are going to make a new release soon, after that this PR will be merged. |
|
Yes sure, I will make those changes in the next pull request, the UI one. |
|
…cuts adjusted according to different Scopes, with the addition of Local Storage (#8186)
…d Shortcuts adjusted according to different Scopes, with the addition of Local Storage (cvat-ai#8186)
|
I think this maybe removed the ability to change the "active class" with hotkeys on a per task basis? Without being able to change the class names that these are applied to, these hotkeys are really not useful. It means I can only use hotkeys for 10 of my > 200 classes, and a completely randomly selected subset at that.
This implementation, seems to have broken the functionality that was requested below (which is more useful IMO) |
Part of: #747
Motivation and context
This PR is part of the GSOC project: Keyboard Shortcut Customization
How has this been tested?
It has been tested by checking if the existing shortcuts work the same way as before.
Checklist
developbranch- [ ] I have updated the documentation accordingly- [ ] I have added tests to cover my changes- [ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.