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

Improve rate_up/down tooltips, pitch vs. speed #12590

Merged
merged 4 commits into from
Mar 12, 2024
Merged

Improve rate_up/down tooltips, pitch vs. speed #12590

merged 4 commits into from
Mar 12, 2024

Conversation

glocq
Copy link
Contributor

@glocq glocq commented Jan 18, 2024

The tooltip help regarding the playback rate change buttons stated that those raise/lower pitch.

If keylock is disabled, this is true, though incomplete, since the buttons also affect playback speed.
If keylock is enabled, this is false, since pitch is left unchanged, and only playback speed changes.

This change replaces the tooltip text about pitch change with text about playback speed. It also copies the clarification:
"(affects both the tempo and the pitch). If keylock is enabled, only the tempo is affected."
over from the Speed Control fader tooltip, to prevent further confusion.

The tooltip help regarding the playback rate change buttons stated that
those raise/lower pitch.

If keylock is disabled, this is true, though incomplete, since the
buttons also affect playback speed.
If keylock is enabled, this is false, since pitch is left unchanged, and
only playback speed changes.

This commit replaces the tooltip text about pitch change with text about
playback speed. It also copies the clarification:
"(affects both the tempo and the pitch). If keylock is enabled, only the
tempo is affected."
over from the Speed Control fader tooltip, to prevent further confusion.
@JoergAtGithub
Copy link
Member

Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

@glocq
Copy link
Contributor Author

glocq commented Jan 18, 2024

Hi, I have signed the contributor agreement.

@daschuer
Copy link
Member

I can unfortunately not confirm your signing. Something has probably failed. Can you please repeat the signing?
Sorry for the hassle.

@glocq
Copy link
Contributor Author

glocq commented Jan 19, 2024

Sorry, I just tried again, hope it ended up working?

@daschuer
Copy link
Member

Thank you. I can confirm your signature.

@ronso0
Copy link
Member

ronso0 commented Jan 25, 2024

@glocq Please take a look at the failing pre-commit hook. The linebreaks ned to be adjusted.
See https://github.com/mixxxdj/mixxx/blob/main/CONTRIBUTING.md#git-workflow -> pre-commit

@glocq
Copy link
Contributor Author

glocq commented Jan 28, 2024

Thank you for the heads up, sorry I didn't get the chance to update my code yet. Will do so in the next days.

@ronso0 ronso0 changed the title Correct inaccurate tooltip help Improve rate_up/down tooltips, pitch vs. speed Mar 12, 2024
src/skin/legacy/tooltips.cpp Outdated Show resolved Hide resolved
src/skin/legacy/tooltips.cpp Outdated Show resolved Hide resolved
src/skin/legacy/tooltips.cpp Outdated Show resolved Hide resolved
src/skin/legacy/tooltips.cpp Outdated Show resolved Hide resolved
Co-authored-by: ronso0 <ronso0@mixxx.org>
@glocq
Copy link
Contributor Author

glocq commented Mar 12, 2024

Hi @ronso0 , sorry I completely forgot about this pull request. I accepted the changes you did implement, however I still think I have some style changes to make. I am trying to install pre-commit right now. Regarding your suggestion to make a common QString, that sounds good, I am not sure I would be the best person to implement it though.

@ronso0
Copy link
Member

ronso0 commented Mar 12, 2024

yeah, pre-commit wants to break lines.
You can see its proposal here https://github.com/mixxxdj/mixxx/actions/runs/8254032169/job/22577721774?pr=12590#step:6:121

Re the string, just take a look at the example I mentioned, it's pretty easy.

@glocq
Copy link
Contributor Author

glocq commented Mar 12, 2024

An update: I did run pre-commit, it says the check "don't commit to branch" failed. I am not sure what this even means :/ Obviously there are commits to my branch?

@ronso0
Copy link
Member

ronso0 commented Mar 12, 2024

what does git status print?

@glocq
Copy link
Contributor Author

glocq commented Mar 12, 2024

Ah sorry, didn't read your answer about breaking lines before talking about my not understanding the output of pre-commit.
git status says everything is clean and up to date, but I will try breaking lines as pre-commit suggests, that might solve the issue.

@glocq
Copy link
Contributor Author

glocq commented Mar 12, 2024

I had indeed missed the example QString. I implemented the change you suggested, ran pre-commit and it did not complain. Sorry for this pull request being so long-winded for such a small change. I hope the changes are adequate now.

@ronso0
Copy link
Member

ronso0 commented Mar 12, 2024

git status says everything is clean and up to date

The exact output might have clues why no-commit-to-branch failed. IIUC that only kicks in if you want to commit/push to a branch that's named exactly like the Mixxx upstream branch, e.g. 2.4 or main.

Anyway, issue resolved I guess : )

@ronso0
Copy link
Member

ronso0 commented Mar 12, 2024

Sorry for this pull request being so long-winded for such a small change.

Oh, don't worry, at all, we had much longer PRs for much less changed lines...

@ronso0
Copy link
Member

ronso0 commented Mar 12, 2024

Changes LGTM, just waiting for CI

@glocq
Copy link
Contributor Author

glocq commented Mar 12, 2024

Thank you for your help!

@ronso0 ronso0 merged commit 24b415c into mixxxdj:main Mar 12, 2024
13 checks passed
@daschuer daschuer added this to the 2.5-beta milestone Mar 12, 2024
@glocq glocq deleted the glocq-tooltipcorrection branch March 13, 2024 06:07
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.

4 participants