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

feat: Add next and previous hotkeys #5350

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Skier23
Copy link

@Skier23 Skier23 commented Jan 24, 2024

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Backend (Database)
  • Backend (API)
  • Frontend

Describe the reason for change

This provides the much needed missing support for going to previous and next tasks using keyboard hotkeys. The current process is impossible to use without a mouse when reviewing already annotated tasks and this pr will allow the re-reviewing process of tasks to be able to be an entire keyboard based operation which significantly speeds up the process
#2311 (comment)

What does this fix?

(if this is a bug fix)

What is the new behavior?

Adds a hotkey for next and previous tasks

What is the current behavior?

No hotkey for next and previous existed

What libraries were added/updated?

N/A

Does this change affect performance?

No

Does this change affect security?

No

What alternative approaches were there?

N/A

What feature flags were used to cover this change?

feat

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

Frontend Annotation Process
(for bug fixes/features, be as precise as possible. ex. Authentication, Annotation History, Review Stream etc.)

Copy link

netlify bot commented Jan 24, 2024

👷 Deploy request for label-studio-docs-new-theme pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 0460ace

Copy link

netlify bot commented Jan 24, 2024

👷 Deploy request for heartex-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 0460ace

@Skier23
Copy link
Author

Skier23 commented Jan 24, 2024

Built and tested it works as expected. Fulfills this request: #2311 (comment)

@Skier23 Skier23 changed the title Add next and previous hotkeys feat: Add next and previous hotkeys Jan 24, 2024
@Skier23
Copy link
Author

Skier23 commented Jan 24, 2024

Further testing revealed that the next and previous hotkeys when on the review page, aren't going to the expected next and previous images. I'm guessing the previous and next methods are not giving the correct result when in the review window but I don't know the code thoroughly enough to know exactly whats going on.

@Skier23
Copy link
Author

Skier23 commented Jan 24, 2024

I just included an alternate way to do this which is by modifying dm.focus-next and dm.focus-previous to always go to the next/previous item instead of just changing focus to the next/previous item (which seemingly doesn't do anything now). This could be an even simpler way to do this. One potential downside of it though is that approach wouldn't work for going previous and next on tasks when in the label multiple tasks labeling view once this is fixed: #5357 @jombooth

Also that second approach as is (or what was there before it with the selecting) has a bug where it doesnt scroll the page down once it gets passed the choices on the page so it doesnt load new items and eventually can't continue going next because of it. Also if you manually scroll the page down it appears to go back to the first item on the page.

edit: I found the source of that second bug mentioned above. This hack works around it but this is where that bug occurs at:
https://github.com/HumanSignal/label-studio/pull/5350/files#diff-843ceacdc988004db8884789b11629a7f380644a2b587c921f950d15333a3df0

Regarding that first bug, its not too hard to workaround since if you use the previous button, itll go back to the previous one before it resets the page so its not a huge deal. (with the fix/workaround to the second bug)

@Skier23
Copy link
Author

Skier23 commented Jan 25, 2024

I've been reviewing existing annotations on my end using these changes and its been working well btw. (primarily using the focus-next and previous changes). The all-keyboard approach is over 2x as fast when reviewing data for me at least

@Skier23
Copy link
Author

Skier23 commented Jun 2, 2024

@Serhii-beep @jombooth just checking to see about getting this incorporated in some form? I'm still running on my local build of this branch and I can no longer upgrade label studio until this gets incorporated.

@amymrou
Copy link

amymrou commented Jun 3, 2024

/jira create

Workflow run
Jira issue TRIAG-589 is created

@sajarin sajarin added hotkeys Hotkeys / shortcuts UX enhancements Community Community Feature Requests, Open Issues, Bugs Reported, or Comments community:fix labels Jun 5, 2024
Copy link
Contributor

@juliosgarbi juliosgarbi left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I have some comments before approval. Could you take a look at the comments and address or fix them if necessary?

web/libs/editor/src/core/settings/keymap.json Outdated Show resolved Hide resolved
web/libs/editor/src/stores/AppStore.js Outdated Show resolved Hide resolved
@Skier23
Copy link
Author

Skier23 commented Jul 14, 2024

Thanks for the contribution. I have some comments before approval. Could you take a look at the comments and address or fix them if necessary?

No problem. I've pushed changes to address the pr comments and merged and resolved conflicts.

@Skier23 Skier23 requested a review from juliosgarbi July 14, 2024 17:24
@Skier23
Copy link
Author

Skier23 commented Jul 14, 2024

@juliosgarbi I've rebuilt and tested locally after the changes on my end and after merging with the latest from development and everything works correctly in testing.

@moritzwilksch
Copy link

amazing, looking forward to this landing! can we get someone to approve the CI run?

@robot-ci-heartex
Copy link
Collaborator

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@EdjeElectronics
Copy link

Hey hey! Has this feature been merged and built yet? Next and previous hotkeys would be super handy!

@Skier23
Copy link
Author

Skier23 commented Oct 25, 2024

Hey hey! Has this feature been merged and built yet? Next and previous hotkeys would be super handy!

The pr has been open for 10 months now but looks like they haven't gotten to merging it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community:fix Community Community Feature Requests, Open Issues, Bugs Reported, or Comments feat hotkeys Hotkeys / shortcuts UX enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants