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.

Sign up for GitHub

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

Jump to bottom

Enable mouse scroll wheel control of sliders #3582

Merged
merged 1 commit into from
Sep 22, 2023
Merged

Enable mouse scroll wheel control of sliders #3582

merged 1 commit into from
Sep 22, 2023

Conversation

stoneman
Copy link
Contributor

@stoneman stoneman commented Sep 16, 2023 •
edited

This pull request adds the ability to manipulate the PID, filter, and motor sliders with the mouse's scroll wheel.

As demonstrated in the videos below, this is a convenient way of adjusting the sliders and, in the case of the motor sliders, enables you to easily set two motors to exactly the same value (which is almost impossible when clicking on them). The content pane can still be scrolled when the mouse is not directly over an active slider control.

Recording.2023-09-16.220335.mp4
Recording.2023-09-16.211940.mp4

Since it is not a good experience having the sliders move about the screen while you are scrolling them, I have moved the PID/filter warning boxes from above the sliders to below them. This prevents the sliders from being pushed downwards when the warning appears.

I have also styled them to match the "notes" that they now sit on top of.

image

image

@stoneman stoneman force-pushed the mouse-scroll branch 2 times, most recently from e93dfd7 to 0d58c36 Compare September 16, 2023 20:55
@haslinghuis haslinghuis added this to the 10.10.0 milestone Sep 16, 2023
@github-actions

This comment has been minimized.

Sign in to view
@blckmn
Copy link
Member

blckmn commented Sep 16, 2023 •
edited

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> PASS
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> FAIL
  • approver count at least three -> FAIL

@nerdCopter
Copy link
Member

nerdCopter commented Sep 20, 2023 •
edited

will this lead to accidental reconfiguration when simply wanting to page up/down. maybe ctrl+wheel?

@chmelevskij
Copy link
Member

chmelevskij commented Sep 21, 2023 •
edited

will this lead to accidental reconfiguration when simply wanting to page up/down. maybe ctrl+wheel?

Feels like this can be the case. Also, when you reduce the size of the window this could possibly cause issues with the scrolling as well.

My 2 cents on this around the UX. General, from experience, ppl get confused when scroll is hijacked by anything other than scroll.

IMO better UI/UX here would be to use modifier key like shift or ctrl/cmd to enable the scroll behaviour instead. (Though shift scroll is scrolling sideways I htink

chmelevskij
chmelevskij reviewed Sep 21, 2023
View reviewed changes
src/js/tabs/motors.js Outdated Show resolved Hide resolved
@stoneman
Copy link
Contributor Author

Since browsers use shift for horizontal scroll and cmd/ctrl for zoom, I've gone with opt/alt. Does that seem reasonable?

@nerdCopter
Copy link
Member

  • reasonable since ctrl activates zoom on web-app version.
  • is here a way to activate when the mouse is anywhere in the <TD>? i.e. when the mouse is a few pixels above or below the slider, then vertical scrolling still happens.

@github-actions

This comment has been minimized.

Sign in to view
@stoneman stoneman marked this pull request as draft September 21, 2023 13:44
@stoneman
Copy link
Contributor Author

stoneman commented Sep 21, 2023 •
edited

is here a way to activate when the mouse is anywhere in the <TD>?

Yes, great idea - I have done so.

@stoneman stoneman marked this pull request as ready for review September 21, 2023 13:50
@stoneman
Copy link
Contributor Author

stoneman commented Sep 21, 2023 •
edited

Note that for alt/opt+scroll wheel to work with the motor sliders, I've had to add Alt to the list of keys which do not disable motor testing.

@github-actions

This comment has been minimized.

Sign in to view
@nerdCopter
Copy link
Member

Note that for alt/opt+scroll wheel to work with the motor sliders, I've had to add Alt to the list of keys which do not disable motor testing.

fde5df0a

  • PID sliders work as expected ❤️
  • motors do not, i'm on Linux and ALT still disables

@stoneman
Copy link
Contributor Author

motors do not, i'm on Linux and ALT still disables

I've changed the ignored keys array / check to use numeric values which should solve this.

Having noticed the values of the existing ignoredKeys, I've discovered that the sliders can be controlled with arrow keys, page up/down, home, and end. Is this documented anywhere? If so, then I think I should add alt+scroll wheel to those docs.

@github-actions

This comment has been minimized.

Sign in to view
@nerdCopter
Copy link
Member

nerdCopter commented Sep 21, 2023 •
edited

  • ALT on motors tab in Linux works!
  • i did not know about arrows/home/end (for PIDs) -- i presume it a side-effect of Chrome/Chromium (NW.js), but not certain.

@nerdCopter
Copy link
Member

nerdCopter commented Sep 21, 2023 •
edited

i dont know if it's required, but maybe rebase on upstream/master. i'll approve this PR as well. 🚀

@nerdCopter
Copy link
Member

does anyone have any objections to error messages being moved? this is my only concern, but i do see the reason.

@stoneman
Copy link
Contributor Author

Could add a help tip like this...

image

image

...but adding that info to the help for some of the pid/filter sliders might be a bit much...

image

🤔

@stoneman
Copy link
Contributor Author

rebased

@github-actions

This comment has been minimized.

Sign in to view
@nerdCopter
Copy link
Member

nerdCopter commented Sep 21, 2023 •
edited

Could add a help tip like this...

i don't think it needs to be verbalized in help-tips, personally.
can be mentioned on release notes maybe.

@stoneman
Copy link
Contributor Author

pushed a change to reduce code duplication and also to call slider.trigger('change'); so that setDirty(true) is called. (I noticed that the profile/rate profile drop downs were getting disabled when clicking on a slider but not when scrolling them)

@stoneman
Copy link
Contributor Author

Pushed another change to reduce duplication ...and now I see that the duplication check is complaining about src/js/deubg.js, which I haven't changed 🙈 Guess I should just ignore that check

@github-actions

This comment has been minimized.

Sign in to view
haslinghuis
haslinghuis reviewed Sep 21, 2023
View reviewed changes
src/js/tabs/motors.js Outdated Show resolved Hide resolved
haslinghuis
haslinghuis reviewed Sep 21, 2023
View reviewed changes
src/js/tabs/motors.js Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@stoneman
Copy link
Contributor Author

@haslinghuis It was because the alt keys were not being ignored on Linux but I guess that's because I used Alt when I should have used AltLeft and AltRight. I've reverted/amended the change.

@github-actions
Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@stoneman
Copy link
Contributor Author

CI failed due to error An unexpected error occurred: "https://registry.yarnpkg.com/abab/-/abab-2.0.6.tgz: Request failed \"502 Bad Gateway\"". I don't see the "re-run job" option.

@github-actions
Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@github-actions
Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@nerdCopter
Copy link
Member

nerdCopter commented Sep 22, 2023 •
edited

795c4aec working on Linux. (left and right Alt)

@haslinghuis haslinghuis merged commit 71e1240 into betaflight:master Sep 22, 2023
9 checks passed
@stoneman stoneman deleted the mouse-scroll branch September 24, 2023 09:58
chmelevskij pushed a commit to chmelevskij/betaflight-configurator that referenced this pull request Apr 27, 2024
@stoneman @chmelevskij
Enable mouse scroll wheel control of sliders (betaflight#3582)
5df1b4e 
Enable mouse scroll wheel for sliders
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chmelevskij chmelevskij chmelevskij left review comments

@haslinghuis haslinghuis haslinghuis approved these changes

@nerdCopter nerdCopter nerdCopter approved these changes

Assignees

@stoneman stoneman

Labels
RN: UI
Projects
Finalizing Firmware 4.5 Release
Status: COMPLETED
Milestone
10.10.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@stoneman @blckmn @nerdCopter @chmelevskij @haslinghuis