add beats_translate_move ControlEncoder #12376

mxmilkiib · 2023-11-30T12:15:03Z

ronso0

Thanks for working on this, adn congratulations for your first PR!

For every code change to be included in Mixxx we need contributor's permissions.

Please 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.

I've left some comments / suggestions.

src/engine/controls/bpmcontrol.cpp

ronso0 · 2023-11-30T12:57:20Z

src/engine/controls/bpmcontrol.cpp

+        const double sampleOffset = frameInfo().sampleRate;
+        if (v <= 0) {
+            const double sampleOffset = sampleOffset * 0.01;
+        } else {
+            const double sampleOffset = sampleOffset * -0.01;
+        }


A variable must only be declared once.
Also if you declare it const (constant) it can't be altered later on.

One way:

Suggested change

const double sampleOffset = frameInfo().sampleRate;

if (v <= 0) {

const double sampleOffset = sampleOffset * 0.01;

} else {

const double sampleOffset = sampleOffset * -0.01;

}

double sampleOffset = frameInfo().sampleRate;

if (v < 0) {

sampleOffset *= -0.01;

} else {

sampleOffset *= 0.01;

}

and another, more compact with Short Hand If...Else (Ternary Operator) (initialized with

const double sampleOffset = frameInfo().sampleRate * (v < 0 ? -0.01 : 0.01);

But actually we're not only interested in the direction but also want the value of the input v, in case the controller accumulated multiple ticks internally and sent 61 or 68 for example.

const double sampleOffset = frameInfo().sampleRate * v * 0.01);

Nice compactification, I'll switch to that

Oh, superfluous ) at the end.

ronso0 · 2023-11-30T12:59:22Z

src/skin/legacy/tooltips.cpp

+    add("beats_translate_move")
+            << tr("Adjust Beats")
+            << tr("When tapped, moves the beatgrid left or right by a small amount.");
+


Good you thought about that, though tooltips are only for GUI controls, and since this new control is made for controllers we don't need a need a tooltip (there are no GUI encoders, only pots with absoute values).

ronso0 · 2023-11-30T19:01:03Z

About code reviews: please keep them open, it's up to the reviewer to check and mark as resolved (besides no one will see your reply if you resolve a conversation ;)

ronso0 · 2023-11-30T19:08:33Z

Please also check the CI runners (at the bottom of this page) once they are finished.

Worth reading to avoid cleanup work later on:
https://github.com/mixxxdj/mixxx/wiki/Contribution-Guidelines
and this one about automatic ode checking (pre-commit)
https://github.com/mixxxdj/mixxx/wiki/Using%20Git#set-up-automatic-code-checking

Thanks for signing the CLA!
You'll find your name in the contributor list in the About dialog in the next release.

mxmilkiib · 2023-11-30T20:06:09Z

Thanks for all the feedback!

On the current build fail due to code style, I'm confused;

https://github.com/mixxxdj/mixxx/actions/runs/7051273531/job/19194088455?pr=12376#step:6:133

Should I change the code to the format in green? But that would make it different from the rest of the formatting (well, in the first case, the formatting of all the related lines above).

https://github.com/mxmilkiib/mixxx/blob/beats_translate_move/src/controllers/controlpickermenu.cpp#L232

https://github.com/mxmilkiib/mixxx/blob/beats_translate_move/src/engine/controls/bpmcontrol.cpp#L90

ronso0 · 2023-11-30T20:12:55Z

Yeah, pre-commit only checks changed lines, and the existing lines were created long before we started using pre-commit.
Yep, the green part is how pre-commit wants it to be, but I'm doubtful if that is correct. Actually we want the lines to break at about 80 chars, and the suggestion is much longer.

I propose to break after every comma, that way it's readable in narrow editors and pre-commit should be happy, too.

mxmilkiib · 2023-11-30T21:36:53Z

I concur. My desire would be to format the entire file the same in fell swoop. Does this need to be thirded or go throiugh a consensus process?

ronso0 · 2023-11-30T21:56:07Z

I understand this desire, though we try to avoid needless mass-formatting. Usually, after being merged, it creates conflicts in other PRs.

mxmilkiib · 2023-11-30T22:39:53Z

🙏

What should I do re the second (given it goes over 80 chars)? There is an irony, in that the suggested style for both is the opposite of the other.

ronso0 · 2023-12-01T00:31:14Z

Same for bpmcontrol.cpp I'd say, break after every comma.

mxmilkiib · 2023-12-01T00:33:52Z

Does the code style linter need updating then?

ronso0 · 2023-12-01T01:01:46Z

Actually it's configured well, but every now and then it has a bad day or something and its suggestions are... weird.
We don't obey it 100% but it makes life much easier.

ronso0 · 2023-12-01T01:05:12Z

You can also ignore certain checks locally (once, for the current command) if they keep you from commiting, for example disable clang-format if it insists on the long line after you broke it in pieces:
SKIP=clang-format git [commit|push|whatever]

mxmilkiib · 2023-12-01T02:04:18Z

Cool beans.

Well, sorry for such an ugly git commit history.

What might the next step be? I take it the failing Windows build isn't related?

daschuer · 2023-12-01T06:48:03Z

Does the code style linter need updating then?

We apply clang-format twice, to respect user line breaks. First run: Unbreak lines with breaks not following the rules. Second Run: break all lines the exceeds 100 at <80.

mixxx/.clang-format

Line 7 in 0f0deb1

# A ColumnLimit > 0 causes clang-format to unbreaks all short lines,

ronso0 · 2023-12-01T10:19:56Z

yeah, but it failed here. Its proposal is 126 chars + indentation.

src/engine/controls/bpmcontrol.cpp

ronso0 · 2023-12-01T10:59:03Z

One more comment.
And the linebreaks please, then I'll take a final look.

mxmilkiib · 2023-12-01T20:16:23Z

How could I make the line

        const mixxx::audio::FrameDiff_t frameOffset = sampleOffset / mixxx::kEngineChannelCount;

not be 97 characters wide?

Assuming that's the main offender.

Edit: it's not, given it doesn't show as diffed, it's just "shifted". The other longer line I cna break on a comma.

src/engine/controls/bpmcontrol.cpp

mxmilkiib · 2023-12-01T23:24:49Z

a bad day or something

There's got to be logic behind why it's trying to make a 130 character wide statement.

ronso0 · 2023-12-01T23:46:12Z

Sure. However, these nonsense suggestions are so rare that obviously nobody is motivated enough to debug the script. Not worth spending time on it atm, manual fixing is... snap.. done. Or just ignore and merge anyway.

mixxx/.clang-format

Line 7 in 0f0deb1

# A ColumnLimit > 0 causes clang-format to unbreaks all short lines,

mixxx/tools/clang_format.py

Line 15 in 6acfe63

# We recommend a maximum line length of 80, but do allow up to 100 characters

Btw I'm not sure why you keep merging the main branch.

mxmilkiib · 2023-12-02T00:43:20Z

Ok.

I'm not sure why I shouldn't?

ronso0 · 2023-12-02T13:18:07Z

I'm not sure why I shouldn't?

IME we only do that to get code that was merged since opeing a PR / starting a branch, especially for long-standing PRs, in order to get other fixes and/or fix merge conflicts.
You can do that whenever you like, I was just wondering what you expect from it.

The line-break issue still needs to be resolved, one connect arg per line. If you can't resist just change the calls above and below, as well, I don't mind. Not many (and only trivial) merge conflicts to be expected with this PR.

ronso0 · 2023-12-02T17:08:06Z

The commit messages look like you used the Github web interface.
If yes, do you also work on this branch locally?

I'm asking because now would be a good moment to squash all commits, and that's not possible in the web UI, only via command line or desktop client (never used that).

mxmilkiib · 2023-12-02T17:15:09Z

Yes. I'm having a mental health crisis atm, various things and a dream of a dead sometime lover. Please take over this, I can't deal with it anymore.

ronso0 · 2023-12-02T17:16:45Z

src/engine/controls/bpmcontrol.h

@@ -141,6 +142,7 @@ class BpmControl : public EngineControl {
    ControlPushButton* m_pAdjustBeatsSlower;
    ControlPushButton* m_pTranslateBeatsEarlier;
    ControlPushButton* m_pTranslateBeatsLater;
+    ControlEncoder* m_pTranslateBeatsMove;


Interesting that CI can build this at all.
Locally, I can't because ControlEncoder is an unknown type, it's neither forward-declared here (preferred) nor is its header included via #include "control/controlencoder.h" here or in the bpmcontrol.cpp.

Btw, I now remember I touched a lot this code in #12104 incl. the formatting of the connect calls ; )

@daschuer could that be a side effect of PCH (which I have disabled locally)?
Anyhow, CI should catch this.

ronso0 · 2023-12-02T17:27:56Z

Okay, I'm sorry to hear that.

I'll adopt this PR then.
Thank you for getting this started and for your work!

ronso0 · 2023-12-02T17:28:52Z

Please don't delete your branch, I'll push to it and someone else needs to do the final review and merge.

ronso0 · 2023-12-02T21:17:29Z

@mixxxdj/developers May someone please take a look?
I adopted this and added another commit, therefore I won't merge myself. Thanks!

JoergAtGithub · 2023-12-04T20:51:28Z

src/engine/controls/bpmcontrol.cpp

-            pTrack->trySetBeats(*translatedBeats);
-        }
-    }
+    slotTranslateBeatsMove(v);


Suggested change

slotTranslateBeatsMove(v);

if (v <= 0) {

return;

}

slotTranslateBeatsMove(-1.0);

Otherwise the code is identical to slotTranslateBeatsLater and the behavior of the function would be different, than before.

🤦
Thank you. I pushed the fix:
later: +1
earlier: -1

ywwg · 2023-12-07T00:53:22Z

src/engine/controls/bpmcontrol.cpp

@@ -127,7 +134,6 @@ BpmControl::BpmControl(const QString& group,
    // Measures distance from last beat in percentage: 0.5 = half-beat away.
    m_pThisBeatDistance = new ControlProxy(group, "beat_distance", this);
    m_pSyncMode = new ControlProxy(group, "sync_mode", this);
-    m_pSyncEnabled = new ControlProxy(group, "sync_enabled", this);


was this dropped because it's unused?

ronso0 · 2023-12-07T09:28:04Z

src/engine/controls/bpmcontrol.cpp

@@ -87,6 +87,13 @@ BpmControl::BpmControl(const QString& group,
    connect(m_pTranslateBeatsLater, &ControlObject::valueChanged,
            this, &BpmControl::slotTranslateBeatsLater,
            Qt::DirectConnection);
+    m_pTranslateBeatsMove = new ControlEncoder(ConfigKey(group, "beats_translate_move"), false);
+    m_pTranslateBeatsMove->setKbdRepeatable(true);


oh, that's useless with ControlEncoder. I'll remove it.

Co-authored-by: ronso0 <ronso0@mixxx.org>

ywwg · 2023-12-07T16:32:17Z

looks good, thank you @mxmilkiib and @ronso0 !

ywwg · 2023-12-07T16:32:55Z

@ronso0 and @JoergAtGithub are your notes addressed?

JoergAtGithub · 2023-12-07T16:37:39Z

LGTM now! Thank you!

github-actions bot added the engine label Nov 30, 2023

mxmilkiib marked this pull request as draft November 30, 2023 12:32

ronso0 requested changes Nov 30, 2023

View reviewed changes

ronso0 changed the title ~~Draft: adding a beats_translate_move~~ add beats_translate_move ControlEncoder Dec 1, 2023

mxmilkiib marked this pull request as ready for review December 1, 2023 02:12

ronso0 requested changes Dec 1, 2023

View reviewed changes

src/engine/controls/bpmcontrol.cpp Show resolved Hide resolved

ronso0 reviewed Dec 1, 2023

View reviewed changes

src/engine/controls/bpmcontrol.cpp Outdated Show resolved Hide resolved

ronso0 reviewed Dec 2, 2023

View reviewed changes

ronso0 force-pushed the beats_translate_move branch from a69215e to 67d2daa Compare December 2, 2023 21:16

JoergAtGithub requested changes Dec 4, 2023

View reviewed changes

ronso0 force-pushed the beats_translate_move branch from 67d2daa to 97c1596 Compare December 5, 2023 00:00

ronso0 mentioned this pull request Dec 5, 2023

CI test: type declaration #12397

Draft

mxmilkiib mentioned this pull request Dec 6, 2023

MIDI learn should support a single knob for opposing functions (e.g. track select) #7557

Open

ywwg reviewed Dec 7, 2023

View reviewed changes

ronso0 reviewed Dec 7, 2023

View reviewed changes

mxmilkiib and others added 2 commits December 7, 2023 10:29

add beats_translate_move to trigger beats_translate_earlier/_later

081015d

Co-authored-by: ronso0 <ronso0@mixxx.org>

BpmControl: improve headers, remove unused member

63602be

ronso0 force-pushed the beats_translate_move branch from 97c1596 to 63602be Compare December 7, 2023 09:29

ywwg requested review from ronso0 and JoergAtGithub December 7, 2023 16:32

JoergAtGithub merged commit 42762b3 into mixxxdj:main Dec 7, 2023
9 of 11 checks passed

ronso0 mentioned this pull request Dec 10, 2023

Add a beats_translate_move? Like beats_translate_earlier and beats_translate_later, but can be used with a single CC SelectKnob style bind #12337

Closed

mxmilkiib mentioned this pull request Jan 5, 2024

Update Changelog for 2.4 #12507

Merged

add beats_translate_move ControlEncoder #12376

add beats_translate_move ControlEncoder #12376

Conversation

mxmilkiib commented Nov 30, 2023

ronso0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Nov 30, 2023

ronso0 commented Nov 30, 2023

mxmilkiib commented Nov 30, 2023 • edited Loading

ronso0 commented Nov 30, 2023

mxmilkiib commented Nov 30, 2023

ronso0 commented Nov 30, 2023

mxmilkiib commented Nov 30, 2023 • edited Loading

ronso0 commented Dec 1, 2023

mxmilkiib commented Dec 1, 2023

ronso0 commented Dec 1, 2023

ronso0 commented Dec 1, 2023

mxmilkiib commented Dec 1, 2023

daschuer commented Dec 1, 2023 • edited Loading

ronso0 commented Dec 1, 2023

ronso0 commented Dec 1, 2023

mxmilkiib commented Dec 1, 2023 • edited Loading

mxmilkiib commented Dec 1, 2023

ronso0 commented Dec 1, 2023

mxmilkiib commented Dec 2, 2023

ronso0 commented Dec 2, 2023

ronso0 commented Dec 2, 2023

mxmilkiib commented Dec 2, 2023

Choose a reason for hiding this comment

ronso0 Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

ronso0 commented Dec 2, 2023

ronso0 commented Dec 2, 2023

ronso0 commented Dec 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg commented Dec 7, 2023

ywwg commented Dec 7, 2023

JoergAtGithub commented Dec 7, 2023

mxmilkiib commented Nov 30, 2023 •

edited

Loading

mxmilkiib commented Nov 30, 2023 •

edited

Loading

daschuer commented Dec 1, 2023 •

edited

Loading

mxmilkiib commented Dec 1, 2023 •

edited

Loading

ronso0 Dec 2, 2023 •

edited

Loading