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

LateNight: fix & rearrange Sync/Leader buttons, side by side #4119

Merged
merged 1 commit into from
Jul 24, 2021

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jul 18, 2021

Edit: final icons
image
image

image

Please test if this icon works.
If so I'll use it for the other skins, too.

Btw it's not clear to me what the dim state does -- the tooltip doesn't clarify that either.
@ywwg Can you explain / improve that?

@github-actions github-actions bot added the skins label Jul 18, 2021
@ronso0 ronso0 added this to the 2.4.0 milestone Jul 18, 2021
@Holzhaus
Copy link
Member

Btw it's not clear to me what the dim state does -- the tooltip doesn't clarify that either.

I don't know if I understand your question correctly, but there are 3 sync leader states:

  • Follower: this deck follows the clock of another deck (the sync leader)
  • Leader: this deck is the sync leader. All other decks will follow this deck's clock. The leader can be:
    • Implicit leader: the leader was picked automatically by Mixxx
    • Explicit leader: the leader was picked explicitly by the user

All of this only makes a difference when at least one of the sync decks has a non-const beatmap. The leader deck will play unchanged, the other decks (followers) will be timestretched so that the beats align.

These playback speed changes are much more noticable on some tracks than on others.

Setting the leader explicitly is useful when one track is just a drum loop and the other one has melody. In such a case I'd set the melody track as leader to prevent an "old tape effect" (wow an flutter), because small tempo changes will be much less noticable on the drum track.

@coveralls
Copy link

coveralls commented Jul 18, 2021

Pull Request Test Coverage Report for Build 1060676345

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 28.633%

Files with Coverage Reduction New Missed Lines %
src/engine/enginevumeter.cpp 1 90.24%
Totals Coverage Status
Change from base Build 1060492554: 0.02%
Covered Lines: 20102
Relevant Lines: 70206

💛 - Coveralls

@ronso0
Copy link
Member Author

ronso0 commented Jul 19, 2021

Great, thanks for the summary @Holzhaus

Leader is implicit if Sync is locked on 2+ decks.
Leader is explicit if the user wants that.
From explicit I can switch back to implicit. Then Mixxx can make any other deck the implicit leader if some conditions are met?

@ronso0
Copy link
Member Author

ronso0 commented Jul 19, 2021

Files with Coverage Reduction New Missed Lines %
src/engine/cachingreader/cachingreaderworker.cpp 2 63.48%

???

@daschuer
Copy link
Member

I have played a bit with this and unfortunately the follower state is not clear for me. The clock is too dominant in that case.
grafik
How about this, dimm the inactive clock and remove it in the follower case.
In this case the orange clock moves around which looks nice IMHO.

@ywwg
Copy link
Member

ywwg commented Jul 19, 2021

@ywwg Can you explain / improve that?

dim state is "soft leader", which is when Mixxx picks which deck is the leader. Bright state is when the user picks an explicit leader. Users should normally be ok with whatever leader Mixxx picks, and on rare occasion they may want to pick something explicit.

Note that if none of the decks are soft leader, the Internal Clock is actually being selected. We will eventually need a user interface for the Internal Clock (which is just a bpm selection and sync button -- Traktor has the exact same thing)

@ywwg
Copy link
Member

ywwg commented Jul 19, 2021

I like the idea of adding / removing the clock

@ywwg
Copy link
Member

ywwg commented Jul 19, 2021

lol I didn't scroll up -- thanks @Holzhaus for the great explanation!

@ywwg
Copy link
Member

ywwg commented Jul 19, 2021

From explicit I can switch back to implicit. Then Mixxx can make any other deck the implicit leader if some conditions are met?

Yes, any deck with sync activated, that is playing, has a bpm, and is audible, is a candidate to be implicit leader. If any of these conditions changes the sync code will magically find the next best deck :)

@ronso0
Copy link
Member Author

ronso0 commented Jul 19, 2021

Passing the clock around sounds reasonable, but there shouldn't be a blank button, = the button icon shouldn't change either IMO.
If Sync lock is enabled on 2+ decks one deck (implicit leader) would have a lit, blank button next to Sync and you'd have to look at other decks to know what that icon would be.
That is confusing.

@daschuer
Copy link
Member

The wall clock icon is confusing alone ;-)

@daschuer
Copy link
Member

But yes, that is true. Can we visualize the token passing differently?

@ronso0 ronso0 force-pushed the sync-leader-gui branch 2 times, most recently from 812244e to d5f35d4 Compare July 20, 2021 01:08
@ronso0
Copy link
Member Author

ronso0 commented Jul 20, 2021

I added the crown icon for testing, select "PaleMoon -crown" theme.

@ronso0
Copy link
Member Author

ronso0 commented Jul 20, 2021

But yes, that is true. Can we visualize the token passing differently?

I have no idea, yet

@Holzhaus
Copy link
Member

But yes, that is true. Can we visualize the token passing differently?

I have no idea, yet

My suggestion would be to make the icon black on dark gray for followers or sync disabled (white icon is too prominent), black on yellow (or a less red orange tone) for implicit leader, black on red for explicit leader.

@ronso0
Copy link
Member Author

ronso0 commented Jul 20, 2021

outline for implicit leader:
image

My suggestion would be to make the icon black on dark gray for followers or sync disabled (white icon is too prominent), black on yellow (or a less red orange tone) for implicit leader, black on red for explicit leader.

okay, so I'll make a less prominent icon for the follower/disabled state.

@ronso0 ronso0 force-pushed the sync-leader-gui branch from d5f35d4 to 2034842 Compare July 21, 2021 13:52
@ywwg
Copy link
Member

ywwg commented Jul 21, 2021

I think we've done sufficient bikeshedding for this first implementation -- let's try living with it in HEAD for a while and see how we like it

@ronso0 ronso0 marked this pull request as draft July 21, 2021 22:13
@ronso0
Copy link
Member Author

ronso0 commented Jul 22, 2021

alrighty, so I'll compress this and let you know when it's ready

@ronso0 ronso0 force-pushed the sync-leader-gui branch 2 times, most recently from a921a05 to 8d2661d Compare July 23, 2021 18:48
@ronso0 ronso0 force-pushed the sync-leader-gui branch from 8d2661d to 72595f8 Compare July 23, 2021 18:50
@ronso0 ronso0 marked this pull request as ready for review July 23, 2021 18:50
@ronso0
Copy link
Member Author

ronso0 commented Jul 23, 2021

done. please test once more.
images in description are updated.

@Holzhaus
Copy link
Member

Thanks, LGTM. We can still change stuff later on if we deem it necessary.

Some warnings, but probably unrelated to this PR:

warning [Main] /home/jan/Projects/mixxx/src/skin/legacy/legacyskinparser.cpp:2038 SKIN ERROR at skin:/decks/rate_controls_compact.xml:98 <PushButton>: Invalid <TooltipId> in skin.xml: "sync_master"
 qt.svg:warning [Main] Cannot open file 'skin:/buttons_/btn__square_set.svg', because: No such file or directory
 qt.svg:warning [Main] Cannot open file 'skin:/buttons_/btn__square_active.svg', because: No such file or directory
warning [Main] /home/jan/Projects/mixxx/src/skin/legacy/legacyskinparser.cpp:2038 SKIN ERROR at skin:/decks/rate_controls.xml:92 <PushButton>: Invalid <TooltipId> in skin.xml: "sync_master"
warning [Main] /home/jan/Projects/mixxx/src/skin/legacy/legacyskinparser.cpp:2038 SKIN ERROR at skin:/decks/rate_controls_compact.xml:98 <PushButton>: Invalid <TooltipId> in skin.xml: "sync_master"
warning [Main] /home/jan/Projects/mixxx/src/skin/legacy/legacyskinparser.cpp:2038 SKIN ERROR at skin:/decks/rate_controls.xml:92 <PushButton>: Invalid <TooltipId> in skin.xml: "sync_master"
warning [Main] /home/jan/Projects/mixxx/src/skin/legacy/legacyskinparser.cpp:2038 SKIN ERROR at skin:/decks/rate_controls_compact.xml:98 <PushButton>: Invalid <TooltipId> in skin.xml: "sync_master"
warning [Main] /home/jan/Projects/mixxx/src/skin/legacy/legacyskinparser.cpp:2038 SKIN ERROR at skin:/decks/rate_controls.xml:92 <PushButton>: Invalid <TooltipId> in skin.xml: "sync_master"
warning [Main] /home/jan/Projects/mixxx/src/skin/legacy/legacyskinparser.cpp:2038 SKIN ERROR at skin:/decks/rate_controls_compact.xml:98 <PushButton>: Invalid <TooltipId> in skin.xml: "sync_master"
warning [Main] /home/jan/Projects/mixxx/src/skin/legacy/legacyskinparser.cpp:2038 SKIN ERROR at skin:/decks/rate_controls.xml:92 <PushButton>: Invalid <TooltipId> in skin.xml: "sync_master"
 qt.svg:warning [Main] Cannot open file 'skin:/palemoon/buttons/btn_embedded_sync.svg', because: No such file or directory
 qt.svg:warning [Main] Cannot open file 'skin:/palemoon/buttons/btn_embedded_sync_active.svg', because: No such file or directory

@Holzhaus Holzhaus merged commit e73ada6 into mixxxdj:main Jul 24, 2021
@ronso0 ronso0 deleted the sync-leader-gui branch July 24, 2021 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants