Add validation to control objects

[ywwg]

No description provided.

[rryan]

This has a race condition -- m_pValidator could become NULL between the m_pValidator check and m_pValidator->validateChange call. You should copy it to a local first.

[rryan]

Also not a giant fan of the one line if-statement/body ;).

[ywwg]

that's approved google style!

[daschuer]

+1 for always use braces
if ( ... ) {
return;
}

Good ..

• for recognising control flow.
• clean diffs if we insert a statement before return later

[daschuer]

set() should become bool set() to inform the caller if the set was successful

[rryan]

Yea :) it's a personal preference and not in the Google C++ style guide. My eyes can easily scan over a 1-liner with return dangling at the end while a block is harder to miss.

[daschuer]

I wander how much flexibility of the control object we really need (vs. simplicity and performance).
This affects the behavior and the validator. I think in general if it is good to have them exactly there but with this NULL pointer approach we have to deal with updates for the objects when they are not fully setup.

I am not sure if this is possible, but I would really like to have an owner/user concept for the control concept.
The owner is responsible to setup the control object and once it is finished, it can be released for usage.

What is the difference between validator and behaviour? Is it possible to join them?

[rryan]

I think validator and behavior are useful to keep separate. I think of validators as a user-defined validation function while the behavior is more intrinsic to the type of the control.

[ywwg]

Updated with some notes addressed. Still no enable/disable, but with better pointer ownership and cleaner implementation.

[rryan]

Since you are providing an inline definition of a method that uses the class you can't forward declare it. The only reason this works is this file includes control.h which includes the controlvalidator.h.

[rryan]

Nice! But this still has the lifecycle and threading issues I pointed out in the last round of comments. The validator outlives the EngineBuffer so any sets to the control after the EngineBuffer is deleted will segfault. Also, no thread should touch EngineBuffer except the callback thread and letting the valdiator have a pointer to EngineBuffer means any thread that changes the play control can call code in EngineBuffer.

I think the right solution here is either an enabled/disabled property for controls or an EnabledValidator / DisabledValidator that you set on the play control when the state changes within EngineBuffer.

[ywwg]

OK, I thought adding the check in the validator for if(m_pEngine ==NULL) was enough to solve the first problem.

but you're right, a more generic enabled/disabled is needed.

What about controls that require more validation than yes, no, however, like the seek validation?

[ywwg]

that's weird because when I didn't have the constructor/destructor the compiler complained about possible undefined behavior without a virtual destructor

[rryan]

Yea you're right, it should have a virtual destructor. I've done that elsewhere in Mixxx and I should fix that too.

[daschuer]

Hi Owen,

I had time to think a little bit about your validation concept ... and I think it will not work in all cases.
Sorry about this late concerns.

We have always the tiny moment between validation and the actual set of the new value. The high priority audio thread can always interrupt the GUI thread in particular this moment. So all validation might be invalid after that.

The problem in the current trunk solution is that parts of mixxx rely on the play control as indicator for play. e.g. The he feedback LED might be on without a playing track.

One solution might be to handle controls like play as the user command. (You can't stop the user to press play) In addition we need a "playing" or "play_fb"control, which has always the value for the play button feedback LED.

Since Mixxx can only react on play commands each time the audio callback is executed. It is fine to evaluate the "play" control there and set the feedback "playing" and the "play" control back to the real "Play value from the engine buffer.

Kind regards,

Daniel

[rryan]

The fact that validation can be interrupted after it runs is fine I think. If the validator validates a value, it has validated that value regardless of whether the control has been updated in the meantime. Take for example playposition. The user wants to seek to 0 and the current value is 0.5. Let's say there's a validator that makes sure that you set it to a positive number (contrived I know since we support the lead-in) but it validates 0 then is interrupted. The engine runs and updates playposition to 0.6. The GUI thread resumes, and the set to 0 is completed. It is still valid to seek to 0 because that's what the user requested.

[rryan]

Making a play indicator control would work but there is already tons of existing state (skins, controller scripts) using play as a toggle button and indicator combined.

[daschuer]

Hi RJ,

in your case everything is fine, because we have several seek controls for the user command and the play position control for the "truth" feedback. If a user seeks a track to 0.5 which is currently unloaded nothing bad will happen.

Owens Validator concept is fine if the validate rules are not changed during run time.

Introducing a play indicator will not effect the current solutions in skins if they are not changed. The still have the "problem" that the playbutton is lit when no track is loaded for a short moment, but so what. All the critical things like AutoDJ and also new skins could work with the new "play indicator"

[ywwg]

OK I've added enabled/disabled. I'm worried that we might want to call set() and overwrite the value even if the control is disabled. See how I had to be careful where to set enabled/disabled on the play button.

[daschuer]

As I pointed out, there might be a problem with interrupting after changing m_Enabled.
If we offer a general solution for this, it opens the door for race conditions later on other controls as well.

What are the controls which have a benefit of this? Where it is really needed?

We may offer a thread save solution for this: We may put checking m_Enabled and writing in a critical section.
But I like a clean "play indicator" solution more.

[daschuer]

can we close this one?

[ywwg]

oh yeah