Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring: tools blocker state outside of canvas #8293

Merged
merged 8 commits into from
Aug 12, 2024
Merged

Conversation

bsekachev
Copy link
Member

@bsekachev bsekachev commented Aug 12, 2024

Motivation and context

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

Summary by CodeRabbit

  • New Features

    • Enhanced interaction handling with new state management for tool operations.
    • Improved responsiveness in components through event tracking for keyboard actions.
  • Bug Fixes

    • Simplified event handling logic across various components, reducing unnecessary complexity.
  • Refactor

    • Removed outdated callback functions related to tool blocking from multiple interfaces and classes, streamlining interaction models.
  • Documentation

    • Updated interfaces and component declarations to reflect the removal of deprecated properties.

Copy link
Contributor

coderabbitai bot commented Aug 12, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The recent changes across multiple files streamline interaction handling and simplify the management of state changes related to tool blocking. Key modifications include the removal of callback functions and unnecessary properties, leading to a more efficient and cleaner codebase. These adjustments suggest a shift towards a more direct handling of user interactions, enhancing overall responsiveness and user experience within the application.

Changes

Files Change Summary
cvat-canvas/src/typescript/canvasModel.ts, cvat-core/src/core-types.ts, cvat-core/src/ml-model.ts Removed the onChangeToolsBlockerState callback from various interfaces and classes, simplifying interaction management.
cvat-canvas/src/typescript/interactionHandler.ts Simplified event handling logic by removing the ctrlKey parameter from shouldRaiseEvent and eliminating certain event listeners.
cvat-ui/src/components/.../opencv-control.tsx, cvat-ui/src/components/.../tools-control.tsx Added latestPostponedEvent property to manage keyboard events and improve user interaction during tool operations.
cvat-ui/src/utils/opencv-wrapper/intelligent-scissors.ts, cvat-ui/src/utils/opencv-wrapper/opencv-wrapper.ts Removed onChangeToolsBlockerState from the IntelligentScissorsParams interface and adjusted intelligentScissorsFactory method to be parameterless.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant OpenCVControlComponent
    participant ToolsControlComponent

    User->>OpenCVControlComponent: Interacts (Key event)
    OpenCVControlComponent->>OpenCVControlComponent: Check state (isActivated)
    OpenCVControlComponent->>User: Respond (if activated)

    User->>ToolsControlComponent: Interacts (Key event)
    ToolsControlComponent->>ToolsControlComponent: Store event (if tools blocked)
    ToolsControlComponent->>User: Respond (when tools are available)
Loading

🐇 In code we prance, with changes bright,
Simplifying paths, making things right.
No more callbacks, clean and clear,
A smoother journey, let’s give a cheer!
With interactions swift, and tools that don't block,
We hop along together, around the clock! 🥕


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
cvat-ui/src/utils/opencv-wrapper/intelligent-scissors.ts (1)

Impact of onChangeToolsBlockerState Removal:

The onChangeToolsBlockerState property is still being used in the following files, which may lead to issues if it's expected to be part of the IntelligentScissorsParams interface:

  • cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/tools-control.tsx
  • cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/opencv-control.tsx

Please review these locations to ensure that the removal of onChangeToolsBlockerState from the interface is correctly reflected throughout the codebase.

Analysis chain

Line range hint 1-15:
Verify the impact of removing onChangeToolsBlockerState from the interface.

The removal of onChangeToolsBlockerState from the IntelligentScissorsParams interface simplifies the parameter structure. Ensure that any dependent code has been updated to accommodate this change.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the removed `onChangeToolsBlockerState` property in the codebase.

# Test: Search for the removed property in the codebase. Expect: No occurrences.
rg --type ts 'onChangeToolsBlockerState'

Length of output: 787

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 10a5f66 and 5879fec.

Files selected for processing (8)
  • cvat-canvas/src/typescript/canvasModel.ts (1 hunks)
  • cvat-canvas/src/typescript/interactionHandler.ts (13 hunks)
  • cvat-core/src/core-types.ts (1 hunks)
  • cvat-core/src/ml-model.ts (3 hunks)
  • cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/opencv-control.tsx (5 hunks)
  • cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/tools-control.tsx (10 hunks)
  • cvat-ui/src/utils/opencv-wrapper/intelligent-scissors.ts (4 hunks)
  • cvat-ui/src/utils/opencv-wrapper/opencv-wrapper.ts (2 hunks)
Files skipped from review due to trivial changes (1)
  • cvat-core/src/core-types.ts
Additional comments not posted (15)
cvat-core/src/ml-model.ts (1)

Line range hint 1-75:
Verify the impact of removing the callback function.

The removal of changeToolsBlockerStateCallback and its setter method indicates a change in how the MLModel class interacts with UI state changes. Ensure that any dependent code has been updated to handle this change.

cvat-ui/src/utils/opencv-wrapper/intelligent-scissors.ts (1)

56-57: Verify the impact of removing onChangeToolsBlockerState from the class.

The removal of onChangeToolsBlockerState from the IntelligentScissorsImplementation class and its constructor simplifies the class's instantiation. Ensure that any dependent code has been updated to accommodate this change.

Verification successful

Removal of onChangeToolsBlockerState is non-impactful.

The onChangeToolsBlockerState property is not used in conjunction with IntelligentScissorsImplementation in the codebase. The removal of this property from the class does not affect other parts of the codebase where onChangeToolsBlockerState is defined or used.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the removed `onChangeToolsBlockerState` property and constructor parameter in the codebase.

# Test: Search for the removed property and constructor parameter. Expect: No occurrences.
rg --type ts 'onChangeToolsBlockerState'

Length of output: 787


Script:

#!/bin/bash
# Description: Verify if `IntelligentScissorsImplementation` is used in the files where `onChangeToolsBlockerState` appears.

# Test: Search for the usage of `IntelligentScissorsImplementation` in the codebase.
rg --type ts 'IntelligentScissorsImplementation'

Length of output: 486

cvat-ui/src/utils/opencv-wrapper/opencv-wrapper.ts (2)

17-17: Verify the impact of removing the parameter from intelligentScissorsFactory.

The removal of the parameter from intelligentScissorsFactory simplifies the method's signature. Ensure that any dependent code has been updated to accommodate this change.

Verification successful

Parameter removal successfully integrated for intelligentScissorsFactory.

The usage of intelligentScissorsFactory in the codebase matches the updated method signature without any parameters, indicating that the change has been correctly propagated. No further action is needed.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the `intelligentScissorsFactory` method with the removed parameter in the codebase.

# Test: Search for the method usage with the removed parameter. Expect: No occurrences.
rg --type ts 'intelligentScissorsFactory'

Length of output: 502


294-294: Verify the impact of modifying the segmentation getter.

The modification of the segmentation getter to instantiate IntelligentScissorsImplementation without arguments reflects changes in the Segmentation interface. Ensure that any dependent code has been updated to accommodate this change.

cvat-canvas/src/typescript/interactionHandler.ts (3)

Line range hint 66-86:
Simplified event raising logic looks good.

The removal of the ctrlKey parameter and associated logic simplifies the method and aligns with the refactoring goals.


127-127: Event triggering logic is streamlined.

The use of the updated shouldRaiseEvent method simplifies the conditions for triggering interactions.


206-206: Rectangle interaction logic is simplified.

The use of the updated shouldRaiseEvent method streamlines the event triggering process after drawing stops.

cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/opencv-control.tsx (3)

141-141: Introduction of latestPostponedEvent enhances interaction management.

Tracking postponed events allows for improved handling of interactions when the tools blocker state changes.


377-396: Tools blocker state logic is simplified and responsive.

The removal of the event parameter and the use of latestPostponedEvent streamline the state management process.


401-405: Key event handling improves interactivity.

The onKeyUp method effectively manages 'Control' key events to trigger tools blocker state changes.

cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/tools-control.tsx (5)

222-222: Addition of latestPostponedEvent property is appropriate.

This property is used to store events that are postponed due to tool blocking, aligning with the refactoring goals.


276-277: Event listeners added in componentDidMount are correctly implemented.

Adding keydown and keyup listeners ensures the component can handle keyboard interactions appropriately.


342-343: Event listeners removed in componentWillUnmount are correctly implemented.

This ensures proper cleanup and prevents memory leaks when the component is unmounted.


554-560: Logic for handling postponed events in interactionListener is well-implemented.

The method correctly checks the toolsBlockerState and defers events when necessary, aligning with the refactoring goals.


597-604: onChangeToolsBlockerState method implementation is effective.

The method correctly updates the tools blocker state and processes postponed events, enhancing the component's responsiveness to user interactions.

@bsekachev bsekachev merged commit 53bcbf3 into develop Aug 12, 2024
47 checks passed
Copy link

@bsekachev bsekachev deleted the bs/refactoring branch August 13, 2024 09:23
@cvat-bot cvat-bot bot mentioned this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant