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

Fixed bpm.tapButton in common-controller-script.js #2594

Merged
merged 12 commits into from Apr 17, 2020
Merged

Fixed bpm.tapButton in common-controller-script.js #2594

merged 12 commits into from Apr 17, 2020

Conversation

Swiftb0y
Copy link
Member

@Swiftb0y Swiftb0y commented Mar 23, 2020 •
edited

closes #2587

I'm sorry for this PR chaos. I made some mistakes that caused git to not cooperate.

Holzhaus
Holzhaus requested changes Mar 25, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks.

Can someone else confirm that this works? My controller's mapping is only in master and I don't want to remove master-only features temporarily just for testing this PR. I can confirm that the behaviour is currently broken and was quite confusing when creating my controller mapping.

res/controllers/common-controller-scripts.js Outdated Show resolved Hide resolved
res/controllers/common-controller-scripts.js Outdated Show resolved Hide resolved
res/controllers/common-controller-scripts.js Outdated Show resolved Hide resolved
@Holzhaus Holzhaus added this to the 2.2.4 milestone Mar 25, 2020
@goddisignz
Copy link
Contributor

I just tested it and the missed presses seem to be detected very well. However, when I press the button two times quickly, the bpm is set to a high value. From 150 due to an accidental double click up to 500 because of a bounced button. Afterwards, it is hard to get the bpm back down to the desired tempo.

@Swiftb0y
Copy link
Member Author

Thanks for testing. The double press should be caught technically. Can you try to increase the lower value on Line 439 and see if the issue still occurs?

@Holzhaus
Copy link
Member

Maybe we should require tapping at least 4 times?

@Swiftb0y
Copy link
Member Author

Sometimes you do need that instant feedback so I'd rather not wait 4 taps. I also don't see how it would solve the reported problem.

@goddisignz
Copy link
Contributor

goddisignz commented Mar 30, 2020 •
edited

Thanks for testing. The double press should be caught technically. Can you try to increase the lower value on Line 439 and see if the issue still occurs?

0.6 seems fine to me.

@Swiftb0y
Copy link
Member Author

@Holzhaus I think 0.6 is still tolerable, wdyt?

@Holzhaus
Copy link
Member

Holzhaus commented Apr 3, 2020

I cannot test because I'd have to backport my controller mapping, but I trust that you tested this thoroughly. Is this ready to merge?

@Holzhaus
Copy link
Member

Holzhaus commented Apr 3, 2020

This has been broken since Mixxx 1.10, right? As this regression has been in a released version, it deserves a changelog entry. Can you add one above the controller mapping entries?

@Swiftb0y
common-controller-scripts: change tap threshold
906b995 
res/controllers/common-controller-scripts: increased treshold
for detecting duplicate taps as the previous threshold was too low
and sometimes failed to identify taps as duplicates
@Swiftb0y
Merge remote-tracking branch 'upstream/2.2' into tapButtonFix
97574d7
@Swiftb0y
add changelog entry for script.tabButton fix
59353f4
@Swiftb0y
Copy link
Member Author

Swiftb0y commented Apr 3, 2020 •
edited

I cannot test because I'd have to backport my controller mapping, but I trust that you tested this thoroughly. Is this ready to merge?

I am not able to test the the last 5 or so commits, so I'd appreciate if @goddisignz could take another look before we merge it.

Holzhaus
Holzhaus requested changes Apr 3, 2020
View reviewed changes
CHANGELOG Outdated Show resolved Hide resolved
res/controllers/common-controller-scripts.js Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member

@goddisignz Can you verify that this works well? I'll go ahead and merge then.

@goddisignz
Copy link
Contributor

Hi,
it took me a while to checkout the changes and compile everything but I just tested it. Pressing the tap button twice as fast multiple times is detected well. Skipping single or multiple beats also.
First time I can also say: LGTM

@Swiftb0y
Copy link
Member Author

Great, thanks for testing

@Holzhaus
Copy link
Member

Cool, in that case let's merge this.

@Holzhaus Holzhaus merged commit 163e3b7 into mixxxdj:2.2 Apr 17, 2020
@mixxxbot mixxxbot mentioned this pull request Aug 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Holzhaus Holzhaus Holzhaus requested changes

Assignees
No one assigned
Labels
controller mappings
Projects
None yet
Milestone
2.2.4
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Swiftb0y @goddisignz @Holzhaus