v4 soloframe monitoring button fix

[kpr0th]

This fixes an issue where a button in a soloframe in "monitoring" mode can't be de-activated. It can be activated, but then it gets stuck in the on state until after the parent function stops.

The docs say it should be able to be stopped: https://docs.qlcplus.org/v4/virtual-console/button (at the bottom of the page). And v5 (qmlui) allows it to be stopped.

[kpr0th]

The attached .qxw file can be used for testing. Steps:

• start the cuelist (or press the "New Scene 1" button in the regular frame)
• observe that the "New Scene 1" button border in the soloframe turns orange (monitoring mode)
• then click the "New Scene 1" button in the soloframe, and observe that it turns green (active mode)
• then try clicking that button one more time

The docs say it should turn off. But without this fix, it stays active.

And note that v5 already works the way the docs describe. Thus this was only a v4 bug so far as I can tell.

c-frame-button-test.zip

[coveralls]

coverage: 32.079% (-0.01%) from 32.092% when pulling 42e526b47bbf4c0465f387b548010010c412cd0c on kpr0th:v4-soloframe-button-fix into 3d891a8f3c7c8d2bf6b8627f2fac77a166e51288 on mcallegari:master.

[mcallegari]

The attached .qxw file can be used for testing. Steps:

• start the cuelist (or press the "New Scene 1" button in the regular frame)
• observe that the "New Scene 1" button border in the soloframe turns orange (monitoring mode)
• then click the "New Scene 1" button in the soloframe, and observe that it turns green (active mode)
• then try clicking that button one more time

The docs say it should turn off. But without this fix, it stays active.

And note that v5 already works the way the docs describe. Thus this was only a v4 bug so far as I can tell.

c-frame-button-test.zip

Hi @kpr0th. I tried the sequence above and it works with the current sources and without your patch. Am I missing something?

[kpr0th]

Hmm... I see that too. I'm assuming another check-in along the way changed this behavior for the better. However, I can still kind of re-create the original problem. Step 1: start the cuelist; note that both of the "New Scene 1" buttons turn orange. Step 2: click the "New Scene 1" button in the REGULAR frame (not the soloframe); note that its border turns green but the soloframe button is still orange. Step 3: click the "New Scene 1" button in the SOLOframe; note that its border turns green. Step 4: try to click the "New Scene 1" button in the soloframe to turn it off -- it stays on.

Admittedly this is a strange corner case. It was more impactful in its earlier form, where just a single activation of the scene from outside the soloframe would cause this behavior. Now that it seems to take 2 different activations from 2 different vcwidgets, it's not something likely to be encountered. But I still think this code change would be helpful. And I haven't seen any downsides to it yet, for whatever that's worth.

[kpr0th]

p.s. - i wonder if this commit was what partially fixed this? https://github.com/mcallegari/qlcplus/commit/e0fe51b5003168939fd531c458b6c4d3cdb5674e#diff-86b7f621a9528a26ef619acf7fa276240f6770326bdb8aed08bdaa67c6e6a6f1 With the addition of emitDisableStateChanged(). Just a guess; not certain it is related but I couldn't find anything else.

[mcallegari]

I found the commit https://github.com/mcallegari/qlcplus/commit/2340ed656468ea05844cc7ad3ab775544d45ebdb that added this extra check https://github.com/mcallegari/qlcplus/blob/master/ui/src/virtualconsole/vcbutton.cpp#L722

7 years have passed and I honestly have no idea why I added that but it seems too specific to me to be removed. Perhaps it has to do with override attributes so maybe the use case is verified with complicated nested functions. So for now I decided not to include this change for 2 reasons: 1- the original issue seems to be resolved with other changes 2- I'm scared of regressions! :worried:

If any other case related to this comes up, I'll keep in mind this change and bring it back. In any case, thank you for your contribution!

[kpr0th]

Thanks for looking this over.

I would normally agree with your fear of regression. But with v5 in the works, and this being v4-only, does that make it any less scary??? ;-)

I think that if statement was so specific because you hadn't built the tri-state button states yet. All it does is handle the special case of clicking a button in a solo-frame that was indirectly active -- now shown as an orange-outlined "monitoring mode" button. It needs to be able to go "fully" active when you click it the first time, instead of turning off. I.e. this was just a special case where "Active" didn't really mean fully-active. Then, it still needs to turn off when you click it a second time. But because you can have state()==Monitoring now that represents this exact case, I don't think you need that extra check any more. In fact, this is literally the only place where either of those helper functions (isChildOfSoloFrame or f->startedAsChild) are called -- so if you accept this change you can remove both of those function definitions too.

The problem is, that logic is still preventing from turning that button back off in some remaining corner cases. As I type this, I just realized that corner case has to be when f->startedAsChild is still true. Maybe I'll try to chase down why that's still showing true in my corner case, and submit a new change to fix that issue instead.