Fix play indicator visibility during play state

[daschuer]

According to the CD player templates, the play button should stay dark during cue preview.
When you hit play the play button lit to acknowledge the latched state.

This fixes a regression from #2619

A minor remaining issue is that the play button icon changes to pause during preview.
We can "fix" this only by introducing a state-full new play_indicator cluttering the CO interface.
This is not worth the work IMHO.

[ronso0]

I implemented the 'preview indicators' to indicate that pressing the Play button then would engage regular playing, and in the last years there were no complaints about it, also I don't think we should refer to CD players for our UX. Pressing physical buttons on devices is an entirely different thing that a software with various possibilities to preview (cue/hotcue in GUI and on controllers).

That said, I'd like to the keep the preview indicator, though it needs to be refined.
What about a colored outline for the Play buttons, via stacked layouts as before, without switching icons?

[ronso0]

Btw the preview state is still locked when right-clicking while pressing a hotcue.

[ronso0]

Btw the preview state is still locked when right-clicking while pressing a hotcue.

Ah sorry, thought #3344 was laready merged.

[daschuer]

The preview indicator is still there due to the toogled icon.

In 2.2 the play button is dark during cue preview and it is lit as a feedback when latching the play.
In current 2.3 the play button is lit during preview state, so we have no feedback that the track is not yet playing.
This is the Issue I am trying to solve.

The Cue/Play/Pause button should behave exactly like the connected hardware and the Cue behaviour taken form CD Players selected in the preference.

We have the "play" CO that is is 1 during play AND preview. And we have the "play_indicator" CO that was invented to control the Play button LEDs.

What about a colored outline for the Play buttons, via stacked layouts as before, without switching icons?

We can do it, but I don't like it. But this is a design topic that we can decide per skin. The underlying issue is that we have no clear CO states to easy implement it.

In this PR, "play" controls only the play/pause Icon with a transparent background and the color of the button is controlled by "play_indicator" just like on hardware.

Controlling the whole play button by the display connection looks odd in case of blinking mode and toogling icon.
That was one of the main fix, original done in #2619.

[ronso0]

The preview indicator is still there due to the toogled icon.

That only works in Deere regular decks and Tango (black icon though), LateNight mini decks, LateNight compact decks and in all except Deere it's not very helpful as it uses dark icons which are almost invisble with the defaut background.
Also it seems we don't have a Cue button in Deere mini decks.

We should have a consistent solution in all skins.
I'll open a separate PR with a comprehensive fix.

[daschuer]

Thank you.

[ronso0]

@daschuer
Could you add a preview_indicator connected to m_bPreviewing in cuecontrol.cpp?
With that additional preview_indicator we can use/extend the current Play button stack because in no mode we need blinking play_indicator while previewing. The stack I propose (top > bottom):

play
  - receives clicks
  - transparent background
  - play/pause icons only changes with play state

preview_indicator
  - when idle: transparent 
  - when previewing: blank/grey with a thick colored outline

play_indicator
  - when blinking: colored background

That way we get

• paused (no blink mode): paused icon, regular background
• paused, not at Cue (any blinking mode): current play icon + colored background
• previewing (any mode): pause icon + colored outline

= reliable feedback:

• blinking Play button would still be consistent with blinking controller modes
• the preview outline would be a useful addition for skins.

Is it feasible for for 2.3?

Apart from that, we still have the following issues with this PR / 2.3 (tested with LateNight):

• Denon & Numark mode: Cue not lit when pressed
• no visual difference between previewing and playing (Play button and it's icon)

[daschuer]

I am a bit in doubt if that is worth the work, facing the other issues we have bicking the 2.3 release.

But anyway, let's have a look.

I think the least hacks solution would be a state full play indicator that has different "value" for every on and off state. This way there would be no need to use hard to maintain widget stack hacks for the play button.

[daschuer]

Pioneer mode has a blinking play button during preview.

[daschuer]

Can't we add the preview indicator to the pause icon in the same color of the lit play button?
In play state it will disapear, because it melts into the background.

[daschuer]

Crazy, I have just found this tutorial showing the https://youtu.be/RK2fPaqTNMc
Denon DJ Prime Go with a show inverted blink pattern. They do not show the cue move by play feature of our Denon mode.

[ronso0]

Can't we add the preview indicator to the pause icon in the same color of the lit play button?
In play state it will disapear, because it melts into the background.

I thought that's what was causing the confusion because it looks exactly like when playing?

we have play: lit when previewing
and play_indicator: used for flashing
so full play indicator you refer to will be connected to bool CueControl::isPlayingByPlayButton() and only lit when the Play button is engaged, right?

Yes, that seems more reasonable than connecting to m_bPreviewing and it wouldn't interfere with Pioneer blinking when previewing.
But either way we need a stack, because the reason for #2619 was that multiple connections to a button were not working reliable with blinking modes. That is btw not really hard to maintain once it' works and it can easily be documented in the xml template.

[daschuer]

I thought that's what was causing the confusion because it looks exactly like when playing?

Yes exactly. Currently with this PR we have the preview state already, Play Icon + Pause Background. There is room for improve this.

But either way we need a stack, because the reason for #2619 was that multiple connections to a button were not working reliable with blinking modes.

I am pretty sure that it is working from the c++ side. I can imagine that there is an update issue with the other states like hover in qss. It would be nice If we find out exactly under which condition it can be reproduced.

we have play: lit when previewing
and play_indicator: used for flashing
so full play indicator you refer to will be connected to bool CueControl::isPlayingByPlayButton() and only lit when the Play button > is engaged, right?

The idea was a "play_indicator_ext" or something

0 = pause off 
1 = pause on 
2 = play-ing off 
3 = playing on  
4 = previewing off 
5 = previewing on 

This way we can use a style for every state separate in a single widget.

With the proposed additional preview_indicator

         play        play_indicator     preview_indicator
0 =      0                    0                  0 
1 =      0                    1                  0 
2 =      1                    0                  0
3 =      1                    1                  0
4 =      1                    0                  1 
5 =      1                    1                  1

Here we can use three playbuttons and exchange them according to play and preview_indicator and have also 6 states.
preview_indicator might be useful for other things in mappings.

So I think your Idea is more flexible and is less work, let's go for it.

[Be-ing]

I am not sure adding a new CO for this is a great idea. Will that break old controller mappings which expect play_indicator to signal when to light the LED?

[daschuer]

The play_inducator will remain in any case.
The main purpose is to map the play button LED.

[Be-ing]

Won't controllers' play buttons be out of sync with the skin if there is a new CO introduced?

[daschuer]

This is up to the skin designer.
Controller buttons can't change the labels, skins can.

[Be-ing]

In 2.2 the play button is dark during cue preview and it is lit as a feedback when latching the play.

Why not go back to this?

[daschuer]

In 2.2 the play button is dark during cue preview and it is lit as a feedback when latching the play.

Because the changing play/pause Icon during blinking locks odd on buttons where we don't have both. This PR goes back to the 2.2 where possible and keeps the Icon.

So from this point of view we can merge this here.

@ronso0 likes to have an improved preview feedback, this can be implemented using a new "preview_indicator" CO.

[ronso0]

IMO the real issue here is that play is not only the actual Play button but also light's up when previewing.
That gives a false impression about the actual is_playing status both in skins and controllers.

Even though I agree that a 6-state play_indicator_ext would probably be the best way forward because it allows to have one button with 6 states and we can use the xml <States> and qss to style it, that requires a lot of work in c++ and skins and testing and I doubt we get that ready in time for 2.3.

I added is_playing in #3358

[daschuer]

Superseded by #3358
Thank you @ronso0