Relax strictness of ControllerScriptInterfaceLegacy methods.

[Swiftb0y]

Should address / fix #11473.

The primary issue is fixed in 7ab811c. The problem is that the refactorings in 2.4 caused invalid CO connections to throw an error instead of just warning on the commandline. Especially for user-facing stable APIs we should follow the Robustness principle. This PR makes that possible by introducing the --controller-pedantic flag that mapping developers should use while developing their mappings to make issues like this apparent.
I also took the liberty to apply this conditional throwing code to more methods of the "legacy" API.

[Holzhaus]

Especially for user-facing stable APIs we should follow the Robustness principle.

Postel's Law is controversial and there are legitimate reasons to reject it, but in this case, fault-intolerant behavior in live situation for obscure controller scripts would indeed do more harm than good.

This PR makes that possible by introducing the --controller-pedantic flag that mapping developers should use while developing their mappings to make issues like this apparent.

I think all these flags are too easy to miss. IMHO we should use a single flag for controller development that combines --developer, --controller-debug and --controller-pedantic.
That way we don't need to remind people to use multiple flags. And I cannot think of any legitimate reason to do controller mapping development without the pedantic flag. Maybe just merge this behavior into --developer?

[Swiftb0y]

Thank you for your insight. I agree with the controversial aspect of Postel's law, but in this case I find it appropriate.

The reason I chose to introduce a separate flag was that I was worried that making mappings strict unconditionally was making it hard to debug pre-existing mappings. If a contributor just wants to fix a trivial bug in an old mapping that didn't follow the stricter requirements (eg use connectControl) they'd be distracted/overwhelmed by all the new issues caused by the pedantic flag. A similar argument can be made for skin designers that just want to have access to the --developer features but would be distracted if they suddenly get overwhelmed by debug output and new errors from mappings. Sure, you could disable the controller mapping and change the qt logging categories, but thats not much more use friendly either.

[Swiftb0y]

We could possibly discuss changing the behavior slightly, making --developer strict by default and then making the rest opt-out? Though I don't know whether thats very great either...

[Swiftb0y]

anything for me to do here?

[daschuer]

Compared to -Wpedanric in c++ this flag does a different thing. Here the flag is more like -Werror. It aborts the controllers scripts in case of warnings that we consider here as bugs in mappings that are still recoverable.

So we may want to rename the flag:
--controller-warning-abort
or whatever better comes in our minds.

The other issue is more s philosophy thing.
Here my two cents:
I don't care if a user uses a mapping that has warnings. As long he is satisfied with it fine. We need to make the barrier for this low. I consider the --developer flag as a helper for that.
The other thing is how do we make sure a mapping meets our quality requirements. Ideas: can we count warnings and display a warning ⚠️ in the preferences "has x warnings".
We have also a broken unittest for mappings that needs some love. #11293 Maybe we can revive it and combine it with git, that only touched mappings are checked.

For this PR an optional flag maybe with a better name is good enough.

What do you think?

[JoergAtGithub]

Suggested change

	"Script tried to connect to (%1, %2) using `connectControl` which "
	"is deprecated. Use `makeConnection` instead!")
	"Script tried to connect to (%1, %2) using `connectControl` which "
	"is deprecated. Use `makeConnection` or `makeUnbufferedConnection` instead!")

[JoergAtGithub]

I'm no sure if I missed an aspect, but from my point of view there are only two modes needed:

• If --controller-debug is set, an invalid CO connection warning should show the error dialog
• If --controller-debug is not set, an invalid CO connection warning should be ignored, because it could be a party stopper

[daschuer]

The Missing point is when users want to tweak a legacy mapping with errors. They can't use --controller-debug for looking into their original problem before fixing the pending issues. This is a barrier for user not familiar with coding and Mixxx.

[Swiftb0y]

The other thing is how do we make sure a mapping meets our quality requirements. Ideas: can we count warnings and display a warning warning in the preferences "has x warnings".

I don't think thats very helpful, nor does it really work since warnings accumulate while running the mapping and can't be determined at parse-time.

I'm open to renaming the flag

We have also a broken unittest for mappings that needs some love. #11293 Maybe we can revive it and combine it with git, that only touched mappings are checked.

Some of the ERROR!s are bogus in there but the presets failing due to an uncaught exception are a problem. I'll try to find some time to look into it because those mappings are very likely broken.

[daschuer]

Ok, than do the renaming to whatever fits to you and we can merge this as a good step. Thanks.

[Swiftb0y]

done

[daschuer]

Thank you. LGTM
Can you add this flag also to the manual?