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

lp1100882: Add support for track colors #2470

Merged
merged 33 commits into from
Feb 17, 2020
Merged

lp1100882: Add support for track colors #2470

merged 33 commits into from
Feb 17, 2020

Conversation

Holzhaus
Copy link
Member

This adds a new column for color-coding tracks. Since opening a color
picker when left-clicking a cell might be clunky and does now allow
changing multiple tracks at once, a right-click context menu entry was
used instead.

Launchpad issue: https://bugs.launchpad.net/mixxx/+bug/1100882

2020-01-28-162201_698x735_scrot

@ronso0
Copy link
Member

ronso0 commented Jan 28, 2020

Nice!
As stated in the bug discussion, a rainbow palette would suit better here IMO. We don't need those distinct colors like for the hotcues but a set of gradual colors to fit moods, energy or whatever.

You're right, for multiple seleted files the menu is better, but for single tracks the delayed double-click would be much quicker. Is it complicated to implement this as well?

@Holzhaus
Copy link
Member Author

Nice!
As stated in the bug discussion, a rainbow palette would suit better here IMO. We don't need those distinct colors like for the hotcues but a set of gradual colors to fit moods, energy or whatever.

Yes, but IMO it makes sense to postpone this until we have support for multiple palettes, which will probably be part of the QColor migration I think. Otherwise we might end up with a lot more work.

You're right, for multiple seleted files the menu is better, but for single tracks the delayed double-click would be much quicker. Is it complicated to implement this as well?

I think this should be possible, I'll have to implement an editor class for the ColorDelegate.

uklotzde
uklotzde reviewed Jan 28, 2020
View reviewed changes
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

I would suggest that we make the color optional, not only in the database but also in the domain model. Let's not repeat the same mistake twice that happened with the cue colors. This time we have the chance to apply the most flexible and versatile approach from the beginning.

res/schema.xml Outdated Show resolved Hide resolved
src/library/colordelegate.cpp Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Jan 29, 2020

Thanks for starting to work on this long requested feature.

How could we let users filter the library table by track color?

@uklotzde uklotzde mentioned this pull request Jan 29, 2020
Merged
@ronso0
Copy link
Member

ronso0 commented Jan 29, 2020

How could we let users filter the library table by track color?

Requiring exactly spelled colornames would probably too errorprone (or take too long?)
when a user types color: can we show a drop-down with all colors of the active palette (Icon + name) like in Track Properties > Cuepoints?

@Be-ing
Copy link
Contributor

Be-ing commented Jan 30, 2020

Here's an idea, maybe a bad one: use the track color for waveform (scrolling + overview) background color.

@Holzhaus Holzhaus changed the title lp1100882: Add support for track colors [WIP] lp1100882: Add support for track colors Jan 30, 2020
@Holzhaus
Copy link
Member Author

Holzhaus commented Jan 30, 2020 •
edited
Loading

I marked this as WIP. Apart from figuring out how to store the colors (#2472), this also lacks a way to reset the track color to uncolored/transparent.

@Holzhaus Holzhaus added this to the 2.3.0 milestone Feb 5, 2020
@Holzhaus Holzhaus mentioned this pull request Feb 6, 2020
Merged
@RafaelFrance
Copy link
Contributor

Very nice feature.

@uklotzde
Copy link
Contributor

Since this PR is marked WiP you may rebase it and clean up the commit history.

@uklotzde
Copy link
Contributor

uklotzde commented Feb 11, 2020 •
edited
Loading

This feature could serve as a blueprint for the cue colors. It doesn't suffer from legacy issues and doesn't have to deal with backwards compatibility. We can explore how cue colors should have been implemented from the start. Maybe we find a migration path to align both solutions as far as possible.

@Holzhaus
Copy link
Member Author

Holzhaus commented Feb 11, 2020 •
edited
Loading

Please have a look at ed3eca7 and let me know what you think. I think we should add a fromQVariant helper function as well.

@Holzhaus Holzhaus changed the title [WIP] lp1100882: Add support for track colors lp1100882: Add support for track colors Feb 11, 2020
@Holzhaus
Copy link
Member Author

Feel free to review now. Let me know if anything is missing. Ideally we'd be able to have another palette for track colors but I don't want to mess around that because it would cause lots of merge conflicts with the cue color PR(s).

@Holzhaus
Copy link
Member Author

I hope this is ready to merge now ;-)

@ronso0
Copy link
Member

ronso0 commented Feb 14, 2020

nice!
did you already think about how to search track colors via Search box as @Be-ing mentioned?

@Holzhaus
Copy link
Member Author

@uklotzde I applied the requested changes. If there's nothing else to do feel free to merge.

@ronso0 I don't know what the best way to search for colors. I think we can add support for that later on.

@uklotzde
Copy link
Contributor

AppVeyor CI failure is unrelated.

LGTM. Really nice work that shows how a new feature should be integrated, i.e. with a solid foundation and clean abstractions. The resulting code is readable and maintainable.

More opinions since this will trigger a database schema update?

@uklotzde
Copy link
Contributor

Let's go!!

@uklotzde uklotzde merged commit 2f64246 into mixxxdj:master Feb 17, 2020
@ehendrikd ehendrikd mentioned this pull request Feb 20, 2020
Merged
@daschuer
Copy link
Member

When trying to edit the color in the library table I got:

Critical [Main]: DEBUG ASSERT: "false" in function void BaseSqlTableModel::setTrackValueForColumn(TrackPointer, int, QVariant) at /home/daniel/workspace/qt5mixxx/src/library/basesqltablemodel.cpp:1039
Warning [Main]: Column "" is not editable!

@daschuer daschuer mentioned this pull request Feb 24, 2020
Merged
@Holzhaus
Copy link
Member Author

When trying to edit the color in the library table I got:

Critical [Main]: DEBUG ASSERT: "false" in function void BaseSqlTableModel::setTrackValueForColumn(TrackPointer, int, QVariant) at /home/daniel/workspace/qt5mixxx/src/library/basesqltablemodel.cpp:1039
Warning [Main]: Column "" is not editable!

Is this a non-issue? From the conversation at #2515 I discern that this was caused by a local DB schema version conflict (maybe from some PR) and is not an actual problem present in master, right?

@daschuer
Copy link
Member

I have just started with a new .mixxx folder and it turns out that the issue is gone.
However it re-occurs after enable the in line library edit.
A normal text editor opens when trying to edit the color library column.

@uklotzde
Copy link
Contributor

Oh, I never tried this. After switching to a text edit box Mixxx becomes inaccessible with a two-sided resize arrow as cursor. Probably a Wayland issue?

We should disable inline editing of this column.

@uklotzde
Copy link
Contributor

Not only inaccessible but unresponsive with a debug assertion:

Critical [Main]: DEBUG ASSERT: "false" in function void 
BaseSqlTableModel::setTrackValueForColumn(TrackPointer, int, QVariant) at /tmp/mixxx/src/library/basesqltablemodel.cpp:1043

Exact line numbers may differ from master!

@Be-ing
Copy link
Contributor

Be-ing commented Feb 25, 2020

In the track table, tracks' color is changed by highlighting the track, at least in Tango. Is this intended? I found it confusing when first trying this new feature.

@Holzhaus Holzhaus mentioned this pull request Feb 25, 2020
Merged
@Holzhaus
Copy link
Member Author

Holzhaus commented Feb 27, 2020 •
edited
Loading

@ronso0

Nice!
As stated in the bug discussion, a rainbow palette would suit better here IMO. We don't need those distinct colors like for the hotcues but a set of gradual colors to fit moods, energy or whatever.

Are you interested in designing a color palette for this? With the changes in #2520 it is now possible to have separate palettes for hotcues and tracks. Here's an example where I used the track color palette from Serato DJ Pro:

Patch: 0001-util-color-colorpalette-Add-support-for-Serato-s-tra.patch
See https://github.com/Holzhaus/serato-tags/blob/master/docs/colors.md#track-colors for reference.

2020-02-27-164817_390x610_scrot

@ronso0
Copy link
Member

ronso0 commented Feb 27, 2020

Yup, I'll take a look next week after I'll have built master with the new features.

@ronso0 ronso0 mentioned this pull request Mar 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uklotzde uklotzde uklotzde approved these changes

@Be-ing Be-ing Be-ing left review comments

Assignees

@Holzhaus Holzhaus

Labels
library ui
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Holzhaus @ronso0 @Be-ing @RafaelFrance @uklotzde @daschuer