Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skins :: separate play button / play_indicator + related improvements #2619

Merged
merged 8 commits into from
May 3, 2020

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 1, 2020

This aims to prevent update issues with play / play_indicator buttons when using blinking Cue modes (see #2342 (comment) and below): the Play buttons would sometimes not update correctly because they had 3 connections attached (play, play_indicator, cue_xx)
Also they blinked like pressed / unpressed in LateNight which was confusing, or at least inconsistent.

Furthermore this unifies the Sampler play buttons.

  • Decks' Play button connections are now separated:
  • top Layer is the actual button with play(left click) and cue_set (right click) connection
  • bottom layer has the play_indicator connection, used to signal Cue mode blinking and previewing from Cue/hotcues
  • remove the play_indicator connection from samplers' Play button:
    IMO it's distracting to have loaded samplers blink all the time with blinking Cue modes
  • set Play right-click in mini decks to cue_default in all skins to have a way to return to the Cue point. Overview hotcue icons, play and cue_default suffice to prepare a deck; for more detailed preparation we have the regular decks & waveforms
  • sampler Play buttons should all use a Pause icon when playing
  • add play_cue_default tooltip, use it for minimal decks' Play button

Idea:

  • in Shade the sampler buttons in the deck row could use an indictator to signal the sampler is loaded (in other skins the sample title is an sufficient indicator)

ronso0 added 6 commits April 1, 2020 23:40
This aims to prevent play/play_indicator update issues with various cue modes.
This aims to prevent incorrect play/play_indicator updates with
various blinking Cue modes.
To be used in minimal decks where there's only a Play button with
cue_default right-click connection.
* use Pause icon when playing
* use group,play as indicator instead of play_indicator
@ronso0 ronso0 changed the title [WIP] Skins :: separate play button / play_indicator + related improvements Skins :: separate play button / play_indicator + related improvements Apr 1, 2020
@ronso0 ronso0 added the skins label Apr 1, 2020
@ronso0
Copy link
Member Author

ronso0 commented Apr 3, 2020

@Be-ing Could you please try to reproduce the indicator update hickups with this branch?

@ronso0 ronso0 requested a review from Be-ing April 4, 2020 18:50
<SetVariable name="display_connection_control"><Variable name="group"/>,play_indicator</SetVariable>
</Template>
<WidgetGroup><!-- Play button + play_indicator -->
<Layout>stacked</Layout>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is an interesting hack but I suspect there is a bug in WPushButton or in the engine's handling of play or play_handling at the root of this...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but maybe it's simply indicator confusion for buttons with two connections affecting the displayValue. now the Play button has only one connnection.

@Holzhaus
Copy link
Member

This now conflicts with master.

@ronso0
Copy link
Member Author

ronso0 commented Apr 15, 2020

Conflicts resolved.

@ronso0 ronso0 added this to the 2.3.0 milestone Apr 16, 2020
@Holzhaus
Copy link
Member

@Be-ing Does this fix the issue for you? Shall we merge?

@daschuer
Copy link
Member

daschuer commented May 2, 2020

It looks like this PR works around a widget bug. I am struggling to follow the discussion and understand what is the main issue fixed by the stacked layout.

I like to dig for the root cause.

Can you add a step by step guide, what to do to reproduce the issue, what is shown,
what should be shown instead.

@ronso0
Copy link
Member Author

ronso0 commented May 2, 2020

I can't reproduce this anymore, just experinced this once.
@Be-ing noticed this with Numark mode IIRC: deck paused, Cue & Play blink, press Play #2342 (comment)

However, the changes proposed here solve some styling issues:
The blinking Play button (deck paused) would show the Pause icon in Deere & LateNight which is wrong and confusing. Also in LateNight the Play button would appear pressed when blinking, which is also wrong.

So I recommend to merge this regardless of the CO issue.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, issue in Deere with toggling play and pause icon is fixed.

@daschuer daschuer merged commit ddcde77 into mixxxdj:master May 3, 2020
@ronso0 ronso0 deleted the play-indicators branch May 3, 2020 16:10
@daschuer
Copy link
Member

daschuer commented May 9, 2020

@Be-ing Does this fix https://bugs.launchpad.net/mixxx/+bug/1856937 for you?

@daschuer
Copy link
Member

daschuer commented May 9, 2020

Does this also fix https://bugs.launchpad.net/mixxx/+bug/1855620

@ronso0 ronso0 mentioned this pull request Jun 2, 2020
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants