mixxxdj / mixxx

Mixxx is Free DJ software that gives you everything you need to perform live mixes.
http://mixxx.org
Other
4.25k stars 1.24k forks source link

feat: static color coding for key column #13390

Open danferns opened 1 week ago

danferns commented 1 week ago

Hi! I just got the core of this feature working and I'm seeking feedback.

A small rectangle is drawn next to the Key label in the Library. The color of the rectangle is based on the Key of the track.

image


I'm concerned about the method used for finding the track's key. Right now, I'm using KeyUtils::guessKeyFromText on the Key string, but I don't think that'll work if the user sets custom Key Notation.

I want to use the value in ColumnCache::COLUMN_LIBRARYTABLE_KEY_ID. But I haven't been able to figure out how to access this column from the QModelIndex& index that is passed into KeyDelegate::paintItem. I'm not sure if the column is even available.

Is there a place where I can see how the index is structured?


Once complete, this will resolve #7810.

Swiftb0y commented 1 week ago

Once complete, this will resolve https://github.com/mixxxdj/mixxx/issues/7810.

I don't think so. That issue requests to color the key depending on how compatible it is to the currently playing track.

Swiftb0y commented 1 week ago

I'm concerned about the method used for finding the track's key. Right now, I'm using KeyUtils::guessKeyFromText on the Key string, but I don't think that'll work if the user sets custom Key Notation.

Probably not. You should store the underlying mixxx::track::io::key::ChromaticKey in the cell when its text gets set. You can use a custom item role for that (I think, I'm not much into Qt MV programming tbh).

danferns commented 1 week ago

Thank you so much @Swiftb0y for the feedback! Preferences for key color palettes is definitely in the plans. I will apply the requested changes and try the item role method.

daschuer commented 12 hours ago

Can you update the screenshot in the initial description of this PR? This way it is more significant if one is later following links from the Changelog.

daschuer commented 12 hours ago

Is the draft state still correct or is it ready for final review and tests?

danferns commented 11 hours ago

Is the draft state still correct or is it ready for final review and tests?

Yes, I am not done yet. I still have to read the Key Colors setting and only render the rectangle if it is enabled.

Also I'd like to squash a couple of commits before review, at least the one which was failing to build.

danferns commented 9 hours ago

I want to read the setting using ConfigKey("[Config]", "KeyColorsEnabled") inside KeyUtils::keyToColor (if key colors are disabled, return an invalid color). But to do that, I would need access to the pConfig variable inside keyutils.cpp.

I see that the pConfig is passed around in class constructors. Is there a way to access it directly?

Should I go about this in some other way entirely?

Swiftb0y commented 8 hours ago

I don't think accessing the config in the KeyUtils is a good idea. Can you share a little more insight on what you're trying to accomplish? Then we can brainstorm together.

danferns commented 7 hours ago

Can you share a little more insight on what you're trying to accomplish?

I have added a preference setting to enable/disable Key Colors. It is called KeyColorsEnabled and I put it under the [Config] group. It is defined inside ColorPaletteSettings.

Now, I want to display the key colors only when this setting is on.

Initially, I was trying to do this preference check inside KeyDelegate. But then I noticed that the KeyUtils::keyToString function is already aware of the user's chosen notation (it doesn't need to be passed in explicitly). So I thought maybe KeyUtils::keyToColor should also do the same, and access the user settings on it's own.

Should the check instead be done inside KeyDelegate?

Swiftb0y commented 7 hours ago

Thanks for the explanation. I think that check should stay inside KeyDelegate. The static data in KeyUtils is ugly and its usefulness is questionable IMO.

danferns commented 6 hours ago

Got it, thank you!