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

Hotcue RGB colors #2520

Merged
merged 164 commits into from
Mar 16, 2020
Merged

Hotcue RGB colors #2520

merged 164 commits into from
Mar 16, 2020

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Feb 26, 2020 •
edited
Loading

This is yet another iteration of the hotcue color PR #2345 (and #2398 and #2399).

To Do (as separate PRs):

  • Make palette and default color(s) editable via preferences (replaces auto_hotcue_colors option)
  • Add mass migration tool

@Holzhaus Holzhaus added this to the 2.3.0 milestone Feb 26, 2020
@Holzhaus Holzhaus self-assigned this Feb 26, 2020
@Holzhaus
Copy link
Member Author

@uklotzde Please have a look if 4c82eee looks good so far. I hope everything is correct ;-)

@Holzhaus Holzhaus changed the title Hotcue RGB colors [WIP] Hotcue RGB colors Feb 26, 2020
@Holzhaus Holzhaus force-pushed the hotcue-rgb-colors branch 3 times, most recently from 9f3a4db to ebbab57 Compare February 27, 2020 12:42
@Holzhaus
Copy link
Member Author

By the way, should the Palette Editor and Mass Migration stuff go into a separate PR? I think that this PR is already big enough right now...

This was referenced Feb 27, 2020
Closed
Closed
Closed
Merged
@Holzhaus Holzhaus changed the title [WIP] Hotcue RGB colors Hotcue RGB colors Feb 27, 2020
@Holzhaus Holzhaus changed the title Hotcue RGB colors [WIP] Hotcue RGB colors Feb 27, 2020
@Holzhaus Holzhaus mentioned this pull request Feb 27, 2020
Merged
@uklotzde
Copy link
Contributor

TODO (pass-by-value): Replace all occurrences of const mixxx::RgbColor::optional_t& and const mixxx::RgbColor& by mixxx::RgbColor::optional_t and mixxx::RgbColor respectively.

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

Holzhaus commented Mar 14, 2020 •
edited
Loading

Sorry if it seems like I'm rehashing a lot of stuff that y'all already talked about

Actually we didn't really talk about palette editor/hotcue auto assign stuff that much. #2530 hasn't been reviewed yet. We talked about this a bit in December, but that's it: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/color.20palettes

@daschuer
Copy link
Member

I am just in progress to create a WHotcueButton, that has a context menu and handles the color connection implicit.

@Holzhaus
Copy link
Member Author

Shall we merge this now and and do the hotcue button as a separate PR or do you want to file the PR against my repo and get it merged it into this branch first?

@ronso0
Copy link
Member

ronso0 commented Mar 15, 2020

Shall we merge this now and and do the hotcue button as a separate PR or do you want to file the PR against my repo and get it merged it into this branch first?

From what I understand there'll be quite a few skin changes required to implement WHotcueButton.
Let's do it separately and not expand this PR even further.

@ywwg
Copy link
Member

ywwg commented Mar 15, 2020

L G T M thank you!

uklotzde
uklotzde suggested changes Mar 15, 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.

Just some minor comments. As soon as the DB migration is finalized we should merge this PR. Bugfixes and tuning could be done later.

Due to the size of this PR (not your fault) any new review is time consuming. Following every single, new commit is impractical. As long as we don't introduce serious regressions I'm (almost) fine with the current state.

res/schema.xml Show resolved Hide resolved
src/preferences/configobject.h Outdated
@@ -187,6 +187,8 @@ template <class ValueType> class ConfigObject {
return m_settingsPath;
}

QList<ConfigKey> getKeysWithGroup(QString group);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thats's why the mutex is declared as mutable. I don't see any reasons why these functions cannot be declared as const.

@Holzhaus
Copy link
Member Author

@uklotzde Done.

uklotzde
uklotzde reviewed Mar 15, 2020
View reviewed changes
res/schema.xml Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member Author

Merge? Then I could use my COVID-19 vacation tomorrow to implement the hotcue color replace dialog.

@uklotzde
Copy link
Contributor

@daschuer Merge?

@ywwg
Copy link
Member

ywwg commented Mar 16, 2020

just waiting on @daschuer approval and then merge

@Holzhaus
Copy link
Member Author

Holzhaus commented Mar 16, 2020 via email •
edited
Loading

@daschuer
Copy link
Member

Thank you. The issue is not only the alpha channel, we will have also an issue if the double value overflows the 32 bit int.

daschuer
daschuer approved these changes Mar 16, 2020
View reviewed changes
src/engine/controls/cuecontrol.cpp
@@ -22,19 +23,39 @@ static const double CUE_MODE_NUMARK = 3.0;
static const double CUE_MODE_MIXXX_NO_BLINK = 4.0;
static const double CUE_MODE_CUP = 5.0;

constexpr double kNoColorControlValue = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Ok, I will issue that in my follow up PR.

src/util/color/colorpalette.h
explicit ColorPalette(QString name, QList<mixxx::RgbColor> colorList)
: m_name(name),
m_colorList(colorList) {
DEBUG_ASSERT(m_colorList.size() != 0);
Copy link
Member

Choose a reason for hiding this comment

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

We have RELEASE_ASSERT

@daschuer daschuer merged commit e16b6a6 into mixxxdj:master Mar 16, 2020
@ywwg
Copy link
Member

ywwg commented Mar 16, 2020

woooooo!

@ehendrikd ehendrikd mentioned this pull request Mar 17, 2020
Merged
@Holzhaus
Copy link
Member Author

I am just in progress to create a WHotcueButton, that has a context menu and handles the color connection implicit.

Can you add that to the list of to-do items on the Project board, so we don't forget about it? ;-)

@ywwg
Copy link
Member

ywwg commented Mar 17, 2020

Can you add that to the list of to-do items on the Project board, so we don't forget about it? ;-)

+1! love having things written down in easy-to-reference places

@Holzhaus Holzhaus mentioned this pull request Mar 21, 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

@ywwg ywwg ywwg approved these changes

@daschuer daschuer daschuer approved these changes

Assignees

@Holzhaus Holzhaus

Labels
library needs review
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Holzhaus @uklotzde @Be-ing @daschuer @ywwg @ronso0 @ferranpujolcamins