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

Fix inverted scroll wheel waveform zoom direction #4195

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

raphabigal
Copy link
Contributor

Fixes bug #1876005

The feature to zoom the waveform using the scroll wheel was inverted, when compared to standard behaviour from each OS. This fixes the reported bug.
Due to it having stayed that way for quite a while, some people might actually already be used to the way it is now, so I also added a preference to keep it inverted.

image

@raphabigal raphabigal changed the base branch from main to 2.3 August 11, 2021 21:47
@github-actions github-actions bot added the ui label Aug 11, 2021
@raphabigal raphabigal marked this pull request as ready for review August 11, 2021 21:49
@uklotzde
Copy link
Contributor

uklotzde commented Aug 11, 2021

We won't introduce any new preference options in 2.3.x, so this fix or change should go into 2.4.0. Changing the behavior of a released version is also not possible, because controller mappings already use this feature and must be updated. According to the changes controller mappings are probably unaffected.

@uklotzde uklotzde changed the base branch from 2.3 to main August 11, 2021 22:36
@uklotzde
Copy link
Contributor

I suggest to omit the extra config option and simply change the behavior permanently in 2.4.0. We already have too many config options and users would only get more confused and overwhelmed.

@ronso0 / @daschuer ?

@Be-ing
Copy link
Contributor

Be-ing commented Aug 11, 2021

Users should be able to trust that we will not break their workflows at all in stable releases. We already made that mistake with the microphone ducking knob in 2.2 and the result was a lot of confused users on the forum. Let's not repeat that mistake.

I suggest to omit the extra config option and simply change the behavior permanently in 2.4.0. We already have too many config options and users would only get more confused and overwhelmed.

I am in favor of less options.

@uklotzde uklotzde added this to the 2.4.0 milestone Aug 11, 2021
@ronso0
Copy link
Member

ronso0 commented Aug 12, 2021

If we change it it should work the same everywhere. A config option just causes confusion.
I can confirm the scroling behaviour would be consistent with that in pdf & image viewers and the file manager for me (Ubuntu 20.04, xfce4).

@raphabigal Please remove the last commit, rebase and force-push. Thanks.

@ronso0
Copy link
Member

ronso0 commented Aug 12, 2021

@raphabigal
Since this is your first contribution we need you to sign the contributor agreement and thereby allows Mixxx to include your changes.
https://docs.google.com/forms/d/e/1FAIpQLScC9QG327sjLL0eWftmfGUasxFFLxg6LCXJ2xHDYRzFIRqyiw/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
Use a nickname or your real name, as you prefer to be listed in the contributor list in the About dialog.

Thanks!

Changed the operators used when zooming through the scroll wheel
so that the zoom will follow the default behaviour from each OS correctly.
@raphabigal raphabigal force-pushed the invert-scroll-zoom-waveform branch from 64c5f92 to d69f3d6 Compare August 12, 2021 18:20
@raphabigal
Copy link
Contributor Author

Thanks for reviewing! I've removed the latest commit and signed the contributor agreement.

@ronso0
Copy link
Member

ronso0 commented Aug 12, 2021

Great, thank you.
Ready to merge @Be-ing @uklotzde ?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1124981149

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 9 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 25.994%

Files with Coverage Reduction New Missed Lines %
src/engine/cachingreader/cachingreader.cpp 9 71.38%
Totals Coverage Status
Change from base Build 1122868421: -0.01%
Covered Lines: 20013
Relevant Lines: 76991

💛 - Coveralls

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM
I am actually used to the old waveform zoom direction, but I am able to learn the new more intuitive one for new users. No need for s preferences option.

@ronso0 ronso0 merged commit e2d4f1f into mixxxdj:main Aug 12, 2021
@Swiftb0y Swiftb0y mentioned this pull request Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants