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

DlgTrackInfo: add track color selector #11436

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 3, 2023

supersedes #11390

Uses WColorPickerAction
This requires two buttons, each with the color picker that's used in the track menu and hotcue popup:

  • colored button: 'fusion' style to allow coloring the button with a simple stylesheet
  • 'no color' button with default style theme that looks like the other buttons in the dialog (unless it's focused, then the OS theme's indicator may distort the appearance)

ToDo

  • add checkmark to selected color button (like in the track menu, should actually already be styled by skin/default.qss 🤷 ) edit: IIRC this requires to allow custom styles for the entire dialog / color picker which could cause trouble (inheritance of WLibraryTableView styles, conflicts with system style) so maybe we need to fix this in c++, WColorPicker/~Action

Screenshot_2023-04-03_14-42-57
Screenshot_2023-04-03_14-42-42
image

@ronso0
Copy link
Member Author

ronso0 commented Apr 4, 2023

Done.
I rebased this onto #11439 for complete testing

@ronso0
Copy link
Member Author

ronso0 commented Apr 5, 2023

Ready!

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Small UI nitpick, the color selector style changes between no color and colored. It looks like the colored style is using the fusion style and no color is using native. The spacing also changes (notice different position of the Color Label on the left), I guess because of the same reason.
Screenshot from 2023-04-21 11-54-12
Screenshot from 2023-04-21 11-54-42

src/library/dlgtrackinfo.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Apr 22, 2023

I used two buttons (Fusion + native style) in order to make the (no color) button blend in, and because --at that time-- I didn't figure how to reset the background color (no color would render as black if used as argument for the stylesheet).

However, I figured the color reset (simply button->setStyleSheet("");) and maybe it'll work with the button in plain 'Fusion' style, too, since it's labeled "(no color)" anyway. The (keyboard) focus style seems to be adopted from the OS style in my case.
It works with just one button now.
Please test!

@ronso0
Copy link
Member Author

ronso0 commented Apr 22, 2023

no color would render as black if used as argument for the stylesheet

Maybe that's due to this snippet in res/skins/default.qss ??

WColorPicker QPushButton[noColor="true"] {
  background-color: black;
}

edit late night comment, it's entirely unrelated.

@Swiftb0y
Copy link
Member

Works well and looks good. The fusion style looks a little out of place but thats part of the color picker widget, right?
I'll take a look at the code shortly.

@ronso0
Copy link
Member Author

ronso0 commented Apr 23, 2023

The Fusion style is required to paint the color button via qss. Else (native theme) painting the button background requires qss hacks like "border: none" which obviously means we'd need to style the entire button ourselves.

The code is mostly from DlgReplaceCueColor.

@ronso0
Copy link
Member Author

ronso0 commented Apr 23, 2023

No idea why SoundSourceProxyTest.regressionTestCachingReaderChunkJumpForward fails on Windows.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Functionality works, but I'd prefer if you could slightly restructure the code.

src/library/dlgtrackinfo.cpp Outdated Show resolved Hide resolved
Comment on lines 527 to 532
void DlgTrackInfo::slotColorPicked(const mixxx::RgbColor::optional_t& newColor) {
m_pColorPicker->setSelectedColor(newColor);
btnColorPicker->menu()->close();

m_trackRecord.setColor(newColor);

if (newColor) {
btnColorPicker->setText("");
Copy link
Member

Choose a reason for hiding this comment

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

Can you separate the logic for changing the color picker style and setting from the logic that sets the track color? Otherwise initializing the color picker with data would automatically also result in setting the track color again. At least I think its better to separate the UI logic action from the database action.

Suggested change
void DlgTrackInfo::slotColorPicked(const mixxx::RgbColor::optional_t& newColor) {
m_pColorPicker->setSelectedColor(newColor);
btnColorPicker->menu()->close();
m_trackRecord.setColor(newColor);
if (newColor) {
btnColorPicker->setText("");
void DlgTrackInfo::trackColorDialogSetColor(const mixxx::RgbColor::optional_t& newColor) {
m_pColorPicker->setSelectedColor(newColor);
btnColorPicker->menu()->close();
if (newColor) {
btnColorPicker->setText("");

src/library/dlgtrackinfo.cpp Outdated Show resolved Hide resolved
src/library/dlgtrackinfo.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Apr 24, 2023

Can you separate the logic for changing the color picker style and setting from the logic that sets the track color? Otherwise initializing the color picker with data would automatically also result in setting the track color again. At least I think its better to separate the UI logic action from the database action.

Sure, makes sense. We could also move button->close menu into the lambda but I think it's okay now.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM.

@Swiftb0y Swiftb0y merged commit 1ef229c into mixxxdj:2.4 Apr 24, 2023
@ronso0
Copy link
Member Author

ronso0 commented Apr 24, 2023

Thanks for your review!

@ronso0 ronso0 deleted the dlgtrackinfo-color branch April 24, 2023 09:19
@ronso0 ronso0 linked an issue Apr 24, 2023 that may be closed by this pull request
@daschuer
Copy link
Member

A duplicated name slipped though

[4/94] Automatic MOC and UIC for target mixxx-lib
AutoUic: /home/daniel/workspace/mixxx2/src/library/dlgtrackinfo.ui: Warning: The name 'horizontalSpacer_5' (QSpacerItem) is already in use, defaulting to 'horizontalSpacer_51'.

@ronso0
Copy link
Member Author

ronso0 commented Apr 25, 2023

Oh. I just pushed a fix.

@Swiftb0y
Copy link
Member

Sorry, thanks for the the prompt fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make track color visible in track properties dialog
3 participants