[WIP] Add shortcuts to brush tools #8519
Conversation
WalkthroughThe changes involve enhancements to the Changes
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![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
🧹 Outside diff range and nitpick comments (2)
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/brush-tools.tsx (2)
Line range hint
159-172: Ensure label color is always definedThe code assumes that
label.coloris always defined. To prevent potential runtime errors, consider adding a fallback color or handling the case wherelabel.colormight be undefined.Apply this diff to handle undefined label colors:
- color: label.color as string, + color: label?.color ?? defaultColor,Replace
defaultColorwith an appropriate fallback color value.
284-305: Redundantdisabledprop on continue buttonThe
disabledprop on the "Continue" button is unnecessarily dependent oneditableState, but the button is already conditionally rendered based on!editableState. This redundancy can be removed to simplify the code.Apply this diff to remove the redundant
disabledprop:<CVATTooltip title={`Continue ${normalizedKeyMap.SWITCH_REDRAW_MODE_STANDARD_CONTROLS}`}> <Button type='text' - disabled={!!editableState} className='cvat-brush-tools-continue' icon={<Icon component={PlusIcon} />} onClick={() => { // ... }} /> </CVATTooltip>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- cvat-ui/src/components/annotation-page/canvas/views/canvas2d/brush-tools.tsx (6 hunks)
- cvat-ui/src/components/header/settings-modal/shortcut-settings.tsx (1 hunks)
- cvat-ui/src/utils/mousetrap-react.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
cvat-ui/src/utils/mousetrap-react.tsx (1)
18-18: Consider documenting the purpose and usage of theweightproperty.The addition of the optional
weightproperty to theKeyMapIteminterface is a good enhancement that allows for more flexible key mapping configurations. However, to ensure proper usage and maintainability, consider the following suggestions:
Add a brief comment explaining the purpose and expected usage of the
weightproperty. This will help other developers understand how to use it correctly.Update any relevant documentation or README files to mention this new property and its implications on shortcut configurations.
Verify if any other files in the project need to be updated to utilize this new property, especially those related to shortcut management or display.
If there's a sorting mechanism for shortcuts, ensure it's updated to consider the
weightproperty.To ensure consistent usage of the new
weightproperty, let's check for any potential usage or sorting mechanisms:cvat-ui/src/components/header/settings-modal/shortcut-settings.tsx (1)
77-77: 💡 Codebase verification🛠️ Refactor suggestion
Missing
weightProperty inkeyMapItems Affects Sorting LogicThe
weightproperty used in the sorting logic is missing or improperly typed in all instances ofkeyMapacross the following files:
cvat-ui/src/utils/hooks.tscvat-ui/src/reducers/shortcuts-reducer.tscvat-ui/src/components/shortcuts.context.tsxcvat-ui/src/components/shortcuts-dialog/shortcuts-dialog.tsxcvat-ui/src/components/annotation-page/tag-annotation-workspace/tag-annotation-sidebar/shortcuts-select.tsxcvat-ui/src/components/layout-grid/layout-grid.tsxcvat-ui/src/components/labels-editor/skeleton-configurator.tsxImpact:
- The sorting based on
weightwill not function as intended, potentially leading to incorrect ordering of shortcut items.- This may also cause runtime errors if the
weightproperty is expected to be a number but is undefined.Recommended Actions:
Define the
weightProperty:
- Ensure that the
weightproperty is added to allkeyMapitems with appropriate type definitions.Update Type Definitions:
- Modify the
KeyMapIteminterface to include theweightproperty, ensuring type safety.Review Sorting Logic:
- Verify and test the sorting logic to confirm it behaves correctly once the
weightproperty is properly implemented.🔗 Analysis chain
Enhance sorting logic with proper type checking and performance considerations.
The addition of sorting based on the
weightproperty is a good improvement for customizing the order of shortcut items. However, there are a few points to consider:
- Type safety: Ensure that the
weightproperty is properly typed in theKeyMapIteminterface to prevent potential runtime errors.- Consistency: Verify that all items in the
keyMaphave theweightproperty defined, or provide a fallback mechanism for items without it.- Performance: For large lists, consider moving the sorting logic outside of the
useMemohook to optimize performance.Consider refactoring the code as follows:
// Add this type to your interfaces or types file interface WeightedKeyMapItem extends KeyMapItem { weight?: number; } // In your component const sortedFilteredKeyMap = useMemo(() => { return filteredKeyMap.sort(([, item1], [, item2]) => { const weight1 = (item1 as WeightedKeyMapItem).weight ?? 0; const weight2 = (item2 as WeightedKeyMapItem).weight ?? 0; return weight1 - weight2; }); }, [filteredKeyMap]); // Then use sortedFilteredKeyMap instead of filteredKeyMap in your items computationPlease run the following script to verify the presence and type of the
weightproperty across allkeyMapitems:This will help ensure that the
weightproperty is consistently implemented across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence and type of the 'weight' property in keyMap items # Test: Search for keyMap definitions and their item properties rg --type typescript -g '!*.test.ts' -g '!*.spec.ts' --multiline --multiline-dotall 'const\s+keyMap\s*=\s*{[^}]+}' -A 10Length of output: 255
Script:
#!/bin/bash # Description: Verify the presence and type of the 'weight' property in keyMap items # Corrected regex: Escaped braces to prevent parse errors rg --type typescript -g '!*.test.ts' -g '!*.spec.ts' --multiline --multiline-dotall 'const\s+keyMap\s*=\s*\{[^}]+\}' -A 10Length of output: 159
Script:
#!/bin/bash # Description: Verify the presence and type of the 'weight' property in keyMap items # Use file extensions to target TypeScript files rg -g '*.ts' -g '*.tsx' --multiline --multiline-dotall 'const\s+keyMap\s*=\s*\{[^}]+\}' -A 10Length of output: 95
Script:
#!/bin/bash # Description: Broadly search for any declarations or imports of 'keyMap' in TypeScript files # Search for any variable declarations (const, let, var) of keyMap rg -g '*.ts' -g '*.tsx' --ignore-file .gitignore '\b(const|let|var)\s+keyMap\b' # Additionally, search for imports of keyMap from other modules rg -g '*.ts' -g '*.tsx' --ignore-file .gitignore 'import\s+[^;]*\bkeyMap\b[^;]*;'Length of output: 1024
Script:
#!/bin/bash # Description: Verify the presence and type of the 'weight' property in keyMap items across identified files # List of files where keyMap is defined FILES=( "cvat-ui/src/utils/hooks.ts" "cvat-ui/src/reducers/shortcuts-reducer.ts" "cvat-ui/src/components/shortcuts.context.tsx" "cvat-ui/src/components/shortcuts-dialog/shortcuts-dialog.tsx" "cvat-ui/src/components/annotation-page/tag-annotation-workspace/tag-annotation-sidebar/shortcuts-select.tsx" "cvat-ui/src/components/layout-grid/layout-grid.tsx" "cvat-ui/src/components/labels-editor/skeleton-configurator.tsx" ) # Iterate over each file and search for the 'weight' property in keyMap for FILE in "${FILES[@]}"; do echo "Checking 'weight' property in $FILE:" rg --multiline --multiline-dotall 'keyMap\s*=\s*\{[^}]*\bweight\s*:\s*\d+' "$FILE" && echo "-> 'weight' property found and properly typed." || echo "-> 'weight' property missing or improperly typed." echo "" doneLength of output: 3767
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/brush-tools.tsx (5)
39-69: Component shortcuts are correctly defined and registeredThe
componentShortcutsobject is well-defined with appropriate names, descriptions, sequences, scopes, and weights for each shortcut. The registration of these shortcuts usingregisterComponentShortcuts(componentShortcuts);is correctly implemented.
93-105: Tool selection callbacks are properly implementedThe use of
useCallbackhooks for tool selection functions (setBrushTool,setEraserTool,setPolygonTool,setPolygonRemoveTool) optimizes performance by preventing unnecessary re-renders. The blocked tools are appropriately handled to prevent activation when they are disabled.
106-111: Shortcut handlers are correctly mappedThe
handlersobject accurately maps the component shortcuts to their respective callback functions, ensuring that keyboard shortcuts activate the correct tools.
261-266: GlobalHotKeys component is properly integratedThe
GlobalHotKeyscomponent is correctly set up with thekeyMapandhandlers, ensuring that the keyboard shortcuts are active within this component.
370-381: Ensure consistent label selection handlingWhen changing the label using the
LabelSelector, it's good practice to verify that the selected label exists and handle any potential errors if it doesn't. Although the current implementation checks iflabelIDis an integer, additional validation can enhance robustness.Run the following script to check for any references to non-existent label IDs:
✅ Verification successful
Label ID Validation Passed
All label IDs used in the
LabelSelectorare properly defined and validated. No undefined label IDs were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all label IDs used in the code correspond to existing labels. # Test: Search for label IDs in the codebase and cross-reference with defined labels. # Expect: All label IDs used are defined in the labels list. # Get all label IDs from the code rg --type js --pcre2 "(?<=(labelID\s*:\s*))\d+" -o -r '$0' --no-filename | sort -u > used_label_ids.txt # Get all defined label IDs rg --type js "id\s*:\s*\d+" -o -r '$0' --no-filename | sort -u > defined_label_ids.txt # Compare the two lists comm -23 used_label_ids.txt defined_label_ids.txt > undefined_label_ids.txt if [ -s undefined_label_ids.txt ]; then echo "Undefined label IDs found:" cat undefined_label_ids.txt else echo "All label IDs are properly defined." fi # Clean up rm used_label_ids.txt defined_label_ids.txt undefined_label_ids.txtLength of output: 547
Script:
#!/bin/bash # Description: Verify that all label IDs used in the code correspond to existing labels. # Test: Search for label IDs in the codebase and cross-reference with defined labels. # Expect: All label IDs used are defined in the labels list. # Get all label IDs from the code by capturing the value after 'labelID: ' rg --type js '"labelID"\s*:\s*(\d+)' -o -r '$1' --no-filename | sort -u > used_label_ids.txt # Get all defined label IDs by capturing the value after 'id: ' rg --type js '"id"\s*:\s*(\d+)' -o -r '$1' --no-filename | sort -u > defined_label_ids.txt # Compare the two lists to find used label IDs that are not defined grep -Fxv -f defined_label_ids.txt used_label_ids.txt > undefined_label_ids.txt if [ -s undefined_label_ids.txt ]; then echo "Undefined label IDs found:" cat undefined_label_ids.txt else echo "All label IDs are properly defined." fi # Clean up rm used_label_ids.txt defined_label_ids.txt undefined_label_ids.txtLength of output: 386
| <CVATTooltip title='Brush size [Hold Alt + Right Mouse Click + Drag Left/Right]'> | ||
| <InputNumber | ||
| className='cvat-brush-tools-brush-size' | ||
| value={brushSize} | ||
| min={MIN_BRUSH_SIZE} | ||
| onChange={(value: number | null) => { | ||
| if (typeof value === 'number' && Number.isInteger(value) && value >= MIN_BRUSH_SIZE) { | ||
| setBrushSize(value); | ||
| } | ||
| }} | ||
| /> | ||
| </CVATTooltip> | ||
| ) : null} | ||
| { ['brush', 'eraser'].includes(currentTool) ? ( | ||
| <Select value={brushForm} onChange={(value: 'circle' | 'square') => setBrushForm(value)}> | ||
| <Select.Option value='circle'>Circle</Select.Option> | ||
| <Select.Option value='square'>Square</Select.Option> | ||
| </Select> | ||
| ) : null} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility of brush size and form selectors
Consider adding labels to the InputNumber and Select components for better accessibility and user experience. This helps screen readers to identify the purpose of the input fields.
Apply this change to add labels:
<CVATTooltip title='Brush size [Hold Alt + Right Mouse Click + Drag Left/Right]'>
+ <label htmlFor='brush-size'>Brush Size:</label>
<InputNumber
+ id='brush-size'
className='cvat-brush-tools-brush-size'
// ...
/>
</CVATTooltip>
+<label htmlFor='brush-form'>Brush Shape:</label>
<Select
+ id='brush-form'
value={brushForm}
onChange={(value: 'circle' | 'square') => setBrushForm(value)}
>
<Select.Option value='circle'>Circle</Select.Option>
<Select.Option value='square'>Square</Select.Option>
</Select>📝 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.
| <CVATTooltip title='Brush size [Hold Alt + Right Mouse Click + Drag Left/Right]'> | |
| <InputNumber | |
| className='cvat-brush-tools-brush-size' | |
| value={brushSize} | |
| min={MIN_BRUSH_SIZE} | |
| onChange={(value: number | null) => { | |
| if (typeof value === 'number' && Number.isInteger(value) && value >= MIN_BRUSH_SIZE) { | |
| setBrushSize(value); | |
| } | |
| }} | |
| /> | |
| </CVATTooltip> | |
| ) : null} | |
| { ['brush', 'eraser'].includes(currentTool) ? ( | |
| <Select value={brushForm} onChange={(value: 'circle' | 'square') => setBrushForm(value)}> | |
| <Select.Option value='circle'>Circle</Select.Option> | |
| <Select.Option value='square'>Square</Select.Option> | |
| </Select> | |
| ) : null} | |
| <CVATTooltip title='Brush size [Hold Alt + Right Mouse Click + Drag Left/Right]'> | |
| <label htmlFor='brush-size'>Brush Size:</label> | |
| <InputNumber | |
| id='brush-size' | |
| className='cvat-brush-tools-brush-size' | |
| value={brushSize} | |
| min={MIN_BRUSH_SIZE} | |
| onChange={(value: number | null) => { | |
| if (typeof value === 'number' && Number.isInteger(value) && value >= MIN_BRUSH_SIZE) { | |
| setBrushSize(value); | |
| } | |
| }} | |
| /> | |
| </CVATTooltip> | |
| ) : null} | |
| { ['brush', 'eraser'].includes(currentTool) ? ( | |
| <> | |
| <label htmlFor='brush-form'>Brush Shape:</label> | |
| <Select | |
| id='brush-form' | |
| value={brushForm} | |
| onChange={(value: 'circle' | 'square') => setBrushForm(value)} | |
| > | |
| <Select.Option value='circle'>Circle</Select.Option> | |
| <Select.Option value='square'>Square</Select.Option> | |
| </Select> | |
| </> | |
| ) : null} |
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
LabelSelectorcomponent for improved label selection.Improvements
Refactor
weightproperty to the key mapping interface for better customization.