Library controls color selector #13077

aqw42 · 2024-04-09T22:18:59Z

Add [Library], track_color_selector

Tested on Debian :
-> Using developer mode, messing with the controls values
-> Using a MIDI controller, mapping each of the controls to a push button or a rotary encoder
Everything worked fine.

New controls list (note to myself for the future manual addition)

[ChannelN], track_color_prev - push button
[ChannelN], track_color_next - push button
[ChannelN], track_color_selector - encoder
[Library], track_color_prev - push button
[Library], track_color_next - push button
[Library], track_color_selector - encoder <-- new

src/library/librarycontrol.cpp

Swiftb0y · 2024-04-16T19:18:12Z

src/library/librarycontrol.cpp

+    while (steps != 0) {
+        if (steps > 0) {
+            pActiveView->assignNextTrackColor();
+            steps--;
+        } else {
+            pActiveView->assignPreviousTrackColor();
+            steps++;
        }


Is this possible without the while? It's not very efficient and could cause visual artifacts.

Thanks for your review.
colorPalette.nextColor() only handles color codes, the actual QT event is thrown with track->setColor().
So I think no visual artifacts or performance issue can be caused by this.
Another code using this have been push last week (see #13066)

I think @Swiftb0y is referring to the Track::colorUpdated(color) signal that is fired for each color change whic causes repaint calls everywhere that track (or many tracks) is loaded (tracks view, decks), and with the selector color changes can indeed occur at high rates.
Didn't think of that during my previous review 👍

So ideally the color selector would be moved to the track itself, find the desired color and assign it right away.
IIUC that only refers to the situation when we have select steps buffered, e.g. from MIDI, and LibraryControl is going to do one by one. Because 'quick' color switch may also happen when triggering color_next/_prev quickly.

causes repaint calls everywhere that track (or many tracks)

okay, WTrackTableView::assignNextTrackColor acts only on the first selected track.

So ideally the color selector would be moved to the track itself

nope, to ColorPalette::nextColor: find the Nth next/prev color (incl. palette wrap-around), return, apply to track.

causes repaint calls everywhere

Yes. thats what I was referring to.

IIUC that only refers to the situation when we have select steps buffered,

That seems like an ugly workaround. Ideally the API would just be changed so you can just set n'th next color directly.

from MIDI, and LibraryControl is going to do one by one.

If the user quickly cycles through colors by pressing the next/prev button quickly, thats fine. Especially since there is quite a lot of delay between the individual presses (which is not true for a programmed loop).

To clarify:
"IIUC that only refers to the situation when we have select steps buffered"
was meant like
"IIUC that only affects the situation when we have select steps buffered" ...

Okay, I was looking at the wrong patch when I first replied (the one in basetrackplayer)
So I updated this one following your advice, what do you think ?

Test result :
With this version, setting [library], track_color_selector to 5 display one time this message :
"debug [Main] BaseTrackCache(0x562e25698fb0) updateIndexWithQuery took 0 ms"
With the old version (while loop in librarycontrol), the same manipulation triggers it once too.
Both are functionally the same.
Is there any other debug message I could use to check the repaint call ?

Swiftb0y · 2024-04-17T17:17:41Z

src/widget/wtracktableview.cpp

+        while (steps != 0) {
+            if (steps > 0) {
+                color = colorPalette.nextColor(color);
+                steps--;
+            } else {
+                color = colorPalette.previousColor(color);
+                steps++;
+            }
+        }


this solution still does a lot of searching. we can go one step further

Suggested change

while (steps != 0) {

if (steps > 0) {

color = colorPalette.nextColor(color);

steps--;

} else {

color = colorPalette.previousColor(color);

steps++;

}

}

const int currentColorIdx = colorPalette.indexOf(color);

const int newColorIndex = rem_euclid(currentColorIdx + steps, colorPalette.size());

color = colorPalette.at(newColorIndex);

rem_euclid is a euclidean remainder/modulo which you'll have to implement in util/math.h. There are plenty of solutions to this online that are reasonably fast (remember % is terribly slow, solutions using three modulos in a row are likely suboptimal).

You might also want to consider to implement this as nextNthColor in ColorPalette directly.

Good suggestion !
But i'm not sure the modulo efficiency is relevant here

Here probably not, no, but if we add it as a general function to math, we should still make sure its within reasonable limits.

What about simply moving this to colorpalette so it is available elsewhere?
ColorPalette::getNthColor()

I simply moved the while loop to getNthColor, as suggested by @ronso0 because there are too many individual cases that are already handled in nextColor and previousColor (for example index -1 is no color, and you need to cycle through it too).

I felt that trying to be overefficient here would damage readability and not gain execution time given the nature of the color change (only manual changes of a few steps can happen).

I called this function in the per deck track_color_selector too.

Test results:
The steps maximum value is 99, min is -99. (is it hardcoded in the control definition ?)
It takes 0ms to execute and there are no visual artifacts.
I made sure we really cycle through the palette, even with big values.
Works for Library and Channel.

yup, the visual artifact problem should be solved, 0ms doesn't mean its very efficient though (milliseconds is wayy too large of a timescale). At least add a TODO at the loop location please.

ronso0 · 2024-06-06T12:20:26Z

src/library/librarycontrol.cpp

+    LibraryView* pActiveView = m_pLibraryWidget->getActiveView();
+    if (!pActiveView) {


FWIW I have a WIP branch that makes use of WLibrary::getCurrentTrackTableView(). This allows to remove many virtual methods from libraryview.h and library features (DlgMissing etc.) which essentially just redirect calls to their own WTrackTableView (which you didn't do here, which means the color controls don't work in AutoDJ, Missing and Hidden?).

I'm sorry, my knowledge of the codebase is clearly insufficient to understand this comment.

I cannot find the getCurrentTrackTableView method anywhere.
Why would AutoDJ need these features ?
How does he access them ?

Can you clarify or point me to the relevant files ?

sorry, I was knee-deep into it when I wrote this.

Like the color select slots, many control slots in LibraryControl call methods of the 'active view'. This can be anything that inherits LibraryView: a pure WTrackTableView, a html root view (top-level item of Crates and Playlists), or some controls row + table view (Auto DJ, Recording, Analysis).

So, until now, a virtual function in libraryview.h needs to be overriden not just in WTrackTableView but in every view/feature where a certain control should be availble. "Until now" because I opened #13335 earlier today which makes use of WLibrary::getCurrentTrackTableView() to get rid of all the virtuals and overrides (see what I removed there).
That's why AutoDJ doesn't need it, but you probably want the color controls to work in the AutoDJ and Recording table, too.

I cannot find the getCurrentTrackTableView method anywhere.

Probably because your branch is based on an older version of main that doesn't have it, yet, not sure.

FWIW I've been using the Github code search a ot lately, very handy:
https://github.com/search?q=repo%3Amixxxdj%2Fmixxx%20%22searchterm%22&type=code
I have a search accelerator (shortcut) and I wrap the term in quotes " (%22) so I use search terms with whitespaces.

ronso0 · 2024-06-18T08:19:28Z

#13335 has been merged, please fix the conflicts (switch to getCurrentTrackTableView())

github-actions · 2024-09-17T00:07:48Z

This PR is marked as stale because it has been open 90 days with no activity.

ronso0 · 2024-09-20T15:03:23Z

@aqw42 do you intend to finish this, or should someone else jump in?

If yes:

#13335 has been merged, please fix the conflicts (switch to getCurrentTrackTableView())

Thanks!

aqw42 · 2024-09-21T15:01:05Z

@aqw42 do you intend to finish this, or should someone else jump in?

If yes:

#13335 has been merged, please fix the conflicts (switch to getCurrentTrackTableView())

Thanks!

I'm on it, thanks for the reminder

Also unified other color controls implementation with deck track color controls

added the last control : selector

Fix codestyle issue

ronso0 · 2024-09-23T08:59:58Z

src/library/librarycontrol.cpp

-
-void LibraryControl::slotTrackColorNext(double v) {
-    if (!m_pLibraryWidget || v <= 0) {
+    LibraryView* pActiveView = m_pLibraryWidget->getCurrentTrackTableView();


Please change to WTrackTableView

Suggested change

LibraryView* pActiveView = m_pLibraryWidget->getCurrentTrackTableView();

WTrackTableView* pTrackTableView = m_pLibraryWidget->getCurrentTrackTableView();

and use pTrackTableView below.

Switch to ptracktableview

JoergAtGithub · 2024-09-28T10:27:05Z

Please resolve the merge conflict, otherwise the CI builds will not start.

src/widget/wtracktableview.cpp

ronso0 · 2024-09-29T14:53:27Z

@aqw42 Please ammend the last fixup commit in to the 'merge branch main' commits.
All commits should build, so we can use git bisect for debugging.

So please also check all the previous commits build.
Thank you!

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

src/widget/wtracktableview.h

src/library/libraryview.h

src/library/librarycontrol.cpp

Swiftb0y · 2024-09-29T15:52:46Z

src/util/color/colorpalette.cpp

+    // TODO : Use rem_euclid modulo function instead of a loop
+    while (steps) {


we should address this in this PR IMO. The repeated calls to nextColor/previousColor which in turn call indexOf are highly suboptimal (in terms of wasted computations).

aqw42 · 2024-10-01T07:13:58Z

@Swiftb0y these comments are mostly unimportant or details that we already discussed 4 month ago.
I feel that you are making me lose my time, please resolve them yourself, as you already did in your suggestions.

ronso0 · 2024-10-01T10:37:44Z

@aqw42 I don't think this is the appropriate tone, considering the delay of 4 months stems from the fact that you missed my reminder in June. (and none of the contributors, me included, gave this high prio considering more important 2.4/2.5 fixes that had to be done).

Let's fix/polish this and get it merged soonish.

Swiftb0y · 2024-10-01T13:37:30Z

these comments are mostly unimportant or details that we already discussed 4 month ago.

sure these comments are certainly nitpicks to a degree, but I feel like they're important enough to raise. The fact that I brought them up even though we've already discussed them should emphasize their importance as well as the fact that they have not yet been addressed. Trust me, I want to do anything but to waste your time, but largescale software development requires careful consideration of each change, otherwise the result will become a large mess that nobody wants to work on anymore.

So please bear with me when I ask for a little bit of polish. It helps you to improve your code and us all to improve mixxx. Thank you.

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

src/library/libraryview.h

src/widget/wtracktableview.h

ronso0 · 2024-10-03T14:15:42Z

Just some cleanup..

ronso0 · 2024-10-05T14:01:26Z

@Swiftb0y Have your concerns all been addressed?

github-actions bot added the library label Apr 9, 2024

ronso0 requested changes Apr 9, 2024

View reviewed changes

src/library/librarycontrol.cpp Outdated Show resolved Hide resolved

aqw42 requested a review from ronso0 April 10, 2024 18:28

ronso0 mentioned this pull request Apr 14, 2024

Code Style: Add brances around single line blocks. #13097

Merged

Swiftb0y reviewed Apr 16, 2024

View reviewed changes

github-actions bot added the ui label Apr 17, 2024

Swiftb0y reviewed Apr 17, 2024

View reviewed changes

github-actions bot added the controller backend label Jun 6, 2024

ronso0 mentioned this pull request Jun 6, 2024

mixxx_controls: added track color controls mixxxdj/manual#638

Open

ronso0 reviewed Jun 6, 2024

View reviewed changes

aqw42 force-pushed the library-controls-color-selector branch from 23ad535 to 7bd68a8 Compare June 6, 2024 17:06

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Sep 17, 2024

daschuer marked this pull request as draft September 17, 2024 09:28

github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Sep 19, 2024

aqw42 added 6 commits September 21, 2024 17:09

Added library color selector

fde6cf9

Also unified other color controls implementation with deck track color controls

ControlPickerMenu: Add library track color control

0b6e276

added the last control : selector

Track color controls: Fix code style issue

32a8bb1

Moved the color selector loop to wtracktableview

9e6d17d

Unused argument warnign fix

09ddf9f

Color Palette: Added getNthColor method

9826bbe

aqw42 force-pushed the library-controls-color-selector branch from 7bd68a8 to 6d46304 Compare September 22, 2024 12:25

Add TODO

561ab07

Fix codestyle issue

aqw42 force-pushed the library-controls-color-selector branch from 6d46304 to 561ab07 Compare September 22, 2024 12:42

aqw42 marked this pull request as ready for review September 22, 2024 13:45

ronso0 mentioned this pull request Sep 23, 2024

Add stem controls #13086

Merged

ronso0 requested changes Sep 23, 2024

View reviewed changes

Update librarycontrol.cpp

a091a4f

Switch to ptracktableview

ronso0 reviewed Sep 29, 2024

View reviewed changes

src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved

Merge branch 'main' into library-controls-color-selector

e70da63

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

aqw42 force-pushed the library-controls-color-selector branch from cd29890 to e70da63 Compare September 29, 2024 15:30

Swiftb0y reviewed Sep 29, 2024

View reviewed changes

Polishing

be3b34c

Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>

ronso0 requested changes Oct 3, 2024

View reviewed changes

src/library/libraryview.h Outdated Show resolved Hide resolved

src/widget/wtracktableview.h Outdated Show resolved Hide resolved

Removed virtual method selectColor in libraryview

7a5e9ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Library controls color selector #13077

Library controls color selector #13077

aqw42 commented Apr 9, 2024 •

edited by ronso0

Loading

Swiftb0y Apr 16, 2024 •

edited

Loading

aqw42 Apr 16, 2024

ronso0 Apr 16, 2024

ronso0 Apr 17, 2024

ronso0 Apr 17, 2024

ronso0 Apr 17, 2024

Swiftb0y Apr 17, 2024

ronso0 Apr 17, 2024

aqw42 Apr 17, 2024

aqw42 Apr 17, 2024

Swiftb0y Apr 17, 2024 •

edited

Loading

aqw42 Apr 17, 2024

Swiftb0y Apr 17, 2024

ronso0 Apr 17, 2024

aqw42 Apr 17, 2024 •

edited

Loading

aqw42 Apr 17, 2024

Swiftb0y Apr 18, 2024 •

edited

Loading

ronso0 Jun 6, 2024

aqw42 Jun 6, 2024

ronso0 Jun 6, 2024

ronso0 Jun 6, 2024

ronso0 commented Jun 18, 2024

github-actions bot commented Sep 17, 2024

ronso0 commented Sep 20, 2024

aqw42 commented Sep 21, 2024

ronso0 Sep 23, 2024

JoergAtGithub commented Sep 28, 2024

ronso0 commented Sep 29, 2024

Swiftb0y Sep 29, 2024

aqw42 commented Oct 1, 2024 •

edited

Loading

ronso0 commented Oct 1, 2024

Swiftb0y commented Oct 1, 2024

ronso0 commented Oct 3, 2024

ronso0 commented Oct 5, 2024

		LibraryView* pActiveView = m_pLibraryWidget->getActiveView();
		if (!pActiveView) {

	LibraryView* pActiveView = m_pLibraryWidget->getCurrentTrackTableView();
	WTrackTableView* pTrackTableView = m_pLibraryWidget->getCurrentTrackTableView();

		// TODO : Use rem_euclid modulo function instead of a loop
		while (steps) {

Library controls color selector #13077

Are you sure you want to change the base?

Library controls color selector #13077

Conversation

aqw42 commented Apr 9, 2024 • edited by ronso0 Loading

Swiftb0y Apr 16, 2024 • edited Loading

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

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

Swiftb0y Apr 17, 2024 • edited Loading

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

aqw42 Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Apr 18, 2024 • edited Loading

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 Jun 18, 2024

github-actions bot commented Sep 17, 2024

ronso0 commented Sep 20, 2024

aqw42 commented Sep 21, 2024

Choose a reason for hiding this comment

JoergAtGithub commented Sep 28, 2024

ronso0 commented Sep 29, 2024

Choose a reason for hiding this comment

aqw42 commented Oct 1, 2024 • edited Loading

ronso0 commented Oct 1, 2024

Swiftb0y commented Oct 1, 2024

ronso0 commented Oct 3, 2024

ronso0 commented Oct 5, 2024

aqw42 commented Apr 9, 2024 •

edited by ronso0

Loading

Swiftb0y Apr 16, 2024 •

edited

Loading

Swiftb0y Apr 17, 2024 •

edited

Loading

aqw42 Apr 17, 2024 •

edited

Loading

Swiftb0y Apr 18, 2024 •

edited

Loading

aqw42 commented Oct 1, 2024 •

edited

Loading