Default cue color

[ferranlala]

This PR introduces a new skin node ControllersDefaultCueColor.

This node is used to let controllers use the same default color than skins for cue points with no assigned color.

TODO

• Add callback so controller scripts can be notified when the default color changed
• Add ControllersDefaultCueColor to all skins
• Docs

[ferranlala]

How should we communicate to the controller scripts that the skin has been reloaded? Via a CO or shall we provide a method to register a callback without being tied to a CO?

[Swiftb0y]

I am in favor of a simple CO. The infrastructure is already there and it's easy to integrate into JS. The CO should just contain the value of the current colorID for that skin. Controllers can then easily bind to it and adjust their color map accordingly.

[ferranlala]

I've added a "[Skin] reloaded" CO that is triggered when the skin is reloaded (not triggered on mixxx startup). With this CO scripts can know when to check for a new default color for the cuepoints with NO COLOR.

[Swiftb0y]

So how exactly is that new color getting communicated? Or how does the mapping retrieve it exactly?

[ferranlala]

Yes, sorry I should have detailed this.

You get the "No Color" color with the regular color js functions, this is with predefinedColorFromId(iId) or predefinedColorsList().

Before this PR, the "No Color" always returned the black color.

Now, if the skin declares a default color for it with the ControllersDefaultCueColor node, when you get the "No Color" with the color functions you will get the color defined by the skin.

So, unless you were treating the "No color" in a special way, you don't need to change anything. You will always get the appropriate color values for it.

You only need to listen to the "[Skin] reloaded" CO, query the "No Color" again in case it changes and update your controller leds.

Makes sense? Let me know if the explanation is still not clear enough.

[ferranlala]

@Swiftb0y If you have an RGB controller I would appreciate if you could do a quick test of this. I don't have such a controller :(

[Swiftb0y]

I think I know what you mean but I'm still not quite sure yet. The behavior of the hotcue_X_color_id stays the same? So we use the predefinedColorFromId(iId) function to translate from the 'No Color' ID (so 0) to the ID of the set color?
I do have an RGB controller but I won't have access to it for the next two weeks.

[ferranlala]

function updateCueColors() {
    var colorId = engine.getValue("[Channel1]",  "hotcue_1_color_id");
    var cueColor = engine.predefinedColorFromId(colorId);
    // Use cueColor.red, .green ...
    // to send a message to your controller
}

With this, you get the color values for whatever PredefiendColor is set to hotcue_1. If hotcue_1 happens to be set to "No Color", the cueColor variable will have the default color defined by the skin. In the current master branch it's always black.

Additionally you need to add something like the snippet below to make sure you update the default color for "No Color" hotcues when the skin changes:

engine.makeConnection("[Skin]", "reloaded", updateCueColors); 

[Swiftb0y]

The Issue is that we have to restrict the whole feature to the predefined color palette. Most controllers don't support or don't have a way documented to set arbitrary colors for their pads. So to keep it compatible with all controllers, the 'no color' color must be an 'alias' to a color from the palette.

[ferranlala]

I see your point. I think we can add yet another skin option (not replacing ControllersDefaultCueColor, but additional to it) so skin designers can provide a fallback predefined color. Then it's up to the controller script maker to choose what makes more sense for their controller. I will do this.

[Swiftb0y]

I would add another CO ([Skin], "default_cue_color_id") that contains the id for the fallback palette color. The callback for that CO should then only be called if the value actually changes. This is IMO the best approach for the mapping system but my experience is limited so I would like to know @Be-ing thinks as well.

[ferranlala]

New implementation. As @Swiftb0y suggested, there's a new [Skin], fallback_cue_color_id CO that indicates the preferred predefined color that controllers should use to render the NOCOLOR cues. This fallback predefined colod is defined by the skins. Skins define this value with the new ControllersFallbackCueColor node.

Cases I tested:

• Start Mixxx with controller disabled and no mapping applied. Select the mapping and enable controller.
• Start Mixxx with a controller enabled and the mapping already configured.
• Change skin (scripts must listen to [Skin] reloaded CO
• Disable and then re-enable the controller
• Change controller mapping

In all cases I asserted that the fallback_cue_color_id CO and the predefined color as obtained with color.predefinedColorFromId(0) have the appropriate values.

All tests are ok except the last one, but it's because the script's init function is not called when changing mapping. This is an existing problem out of the scope of this PR.

[Swiftb0y]

Looks good. I'm gonna try to test it with my controller in the following days.

[daschuer]

This function does not store the pointer, so it should take a const reference.
This is not part of this PR, but the change suits to simplify the using code.

[daschuer]

this can live on stack after changing jsColorFrom()

[daschuer]

I would prefer to remove the default parameter here and set it explicit. It is only used in one place.

[daschuer]

I am not sure if the ControllersDefaultCueColor node has any value.

The in case of free programmable RGB leds, it is up to the mapping author to pick RGB codes for the whole palette that either look good and match somehow the screen colors.
The used RGB codes will most likely not match the screen RGB codes.

So if one than in turn uses the ControllersDefaultCueColor directly it may look foreign, is too dark or dazzling.

So we may consider to remove this to not encourage a mapping author using it.

[ferranlala]

Currently skins can set an arbitrary color to be used for cue points with NOCOLOR assigned (via DefaultMark). The situation we want to avoid is a cue point to be displayed with one color on screen and a different color on the controller. To avoid this I can think of three solutions:

• This PR, where skins set a default color for the controllers to use. This solution works under two assumptions:
  • Skins set similar colors to ControllersDefaultCueColor and DefaultMark. [1]
  • Controllers read ControllersDefaultCueColor and tweak it so its properly displayed in the controller [2]
• Remove ControllersDefaultCueColor. This means skins can still customize how the cue points with NOCOLOR look through ControllersFallbackCueColor. Of course they have less freedom on what color to choose.
• Don't let skins customize how the cue points with NOCOLOR look at all. Maybe after Auto assign hotcue color #2122, this customization is less needed. [3]

@daschuer @Swiftb0y @ronso0 @Be-ing
What do you think?

[1] Skins could set DefaultMark to red, and ControllersDefaultCueColor to blue and then we would still have the problem. Also, DefaultMark is defined per waveform widget, so skins could use a different color on each deck. Skin designers are responsible to avoid this pitfalls.

[2] This color transformation must be generic because controller scripts do not know which color they will receive. Thus, this transformation won't be perfect, but can be good enough to avoid colors that look weird on the controller.

[3] This means that NOCOLOR will be removed and replaced by a new regular color. Also, we will need to rethink the DefaultMark node.

[ferranlala]

@daschuer The COs in SkinLoader are leaking. I see the skin loader is not destroyed in MixxxMainWindow::finalize() but in the destructor, why is that?

[daschuer]

Here is the commit where I have done the move:
2826de1
Unfortunately I could not remember what was the reason.

Are you sure that SkinLoader is the issue? It should not own any CO.

[ferranlala]

Well it's the issue because I added COs there. What should own skin-related COs that outlive the currently loaded skin?

[daschuer]

I think you can try to delete it earlier to check, what as the issue.
If that does not work, you may add a clean up function that deletes just the COs earlier.

[ferranlala]

I moved the COs out of SkinLoader to a dedicated class. But when the COs are deleted, SkinLoader still has ControlProxies for those COs. Is this a problem? How does the CO system handle this?

[Swiftb0y]

I might have time in the next couple of days to introduce this change into Components JS, but I wanna wait for #2030 to get merged first.
Also ControllerEngineTest.colorProxy fails on my system as well as in AppVeyor when building this PR.

[daschuer]

I moved the COs out of SkinLoader to a dedicated class. But when the COs are deleted, SkinLoader still has ControlProxies for those COs. Is this a problem? How does the CO system handle this?

This is handeled via a shared pointer:

mixxx/src/control/controlproxy.h

Line 188 in 4756186

QSharedPointer<ControlDoublePrivate> m_pControl;

I guess that you still receive the leaking COs warning because of that.

[Be-ing]

Thank you for working on this, but I also doubt this is a good solution. How about we move away from letting hotcues not have colors? #2122 was a step towards this but it is an option that can be turned off. I think a better solution than both this and #2122 would be showing a 4x2 grid in the preferences for users to choose default colors per hotcue with a color selection menu. This could reuse the ColorMenu class I am introducing in #2238.

[ferranlala]

I think you’re totally right. We can just have different strategies to assign colours To the newly created hotcues. We can let the user choose the preference they like the most in our preferences page. So the strategies can be:

• The same different color for all the hotcues
• Specific color per hotcue type
• Sequentially assigng a different color to each hotcue