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.

Sign up for GitHub

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

Jump to bottom

Passthrough: improve UI / UX #4794

Merged
merged 7 commits into from
Oct 10, 2022
Merged

Passthrough: improve UI / UX #4794

merged 7 commits into from
Oct 10, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jun 11, 2022 •
edited
Loading

  • Waveforms: show 'Passthrough' label
  • Overview: get 'Passthrough' label color from skin node
  • Overview: prohibit mouse interaction
  • prohibit cue changes (CUE, hotcues, intro, outro) while seeking is blocked (queued, waveform doesn't move)
    .
  • revert/adjust 5af73d2 since that's now obsolete

@github-actions github-actions bot added the ui label Jun 11, 2022
@ronso0 ronso0 mentioned this pull request Jun 11, 2022
Merged
@ronso0 ronso0 force-pushed the waveform-show-passthrough-label branch from b9343db to d8dc386 Compare June 11, 2022 17:23
@github-actions github-actions bot added the skins label Jun 11, 2022
@ronso0 ronso0 force-pushed the waveform-show-passthrough-label branch 4 times, most recently from 8bcaa10 to 53eb75d Compare June 14, 2022 08:21
@ronso0 ronso0 marked this pull request as ready for review June 14, 2022 08:23
@ronso0 ronso0 added the engine label Jun 14, 2022
@ronso0 ronso0 added this to the 2.4.0 milestone Jun 14, 2022
daschuer
daschuer requested changes Jun 29, 2022
View reviewed changes
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.

Looks and works good, only one minor finding.

The hotcues are still lit according tho the loaded track. I guess that is OK, or do we want to switch them all off?

src/engine/controls/cuecontrol.cpp Outdated Show resolved Hide resolved
src/engine/controls/cuecontrol.cpp
this,
&CueControl::cueClear,
Qt::DirectConnection);
connect(m_pCueGoto,
Copy link
Member

Choose a reason for hiding this comment

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

Some of these controls are event driven.
I can imagine that the logic is struggling if an event happens during disconnected.

Not sure if this actually is an issue because all buttons are momentary and it unlikely that a user presses one during switching ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Uff, good point. "Unlikely" != impossible : | like release a pressed hotcue when toggling passthrough..
I'll take a look. Hopefully there are only minor UX issues, no crashes..

@ronso0
Copy link
Member Author

ronso0 commented Jun 29, 2022

The hotcues are still lit according tho the loaded track. I guess that is OK, or do we want to switch them all off?

Well, the reason I started workign on this was that the deck state was not consistent:
playback is blocked, as well as seeking, otoh pressing cue / hotcues would appear to have some effect (playpos in the overview jumps) but that doesn't match the waveform's (real) playposition, since seeking is blocked.

So I thought the hotcues and (visual) overview seeking should be 'frozen' as well.

@daschuer
Copy link
Member

So I thought the hotcues and (visual) overview seeking should be 'frozen' as well.

Yes it is frozen with the last state. And when you load another track during pass through the hot cues button and colors changes to the new track. Kind of OK.

@ronso0
Copy link
Member Author

ronso0 commented Jun 29, 2022

Yeah, because IIRC we agreed that at least some degree of deck preparation should be possible, for when passthrough is disabled.

@daschuer
Copy link
Member

That makes sense.

@ronso0 ronso0 force-pushed the waveform-show-passthrough-label branch 2 times, most recently from 40c2e89 to 1e797bb Compare July 2, 2022 03:00
@ronso0 ronso0 marked this pull request as draft July 2, 2022 19:15
@ronso0 ronso0 force-pushed the waveform-show-passthrough-label branch from 1e797bb to fc56c59 Compare July 2, 2022 21:21
daschuer
daschuer reviewed Jul 4, 2022
View reviewed changes
src/engine/controls/cuecontrol.h Outdated
@@ -346,6 +351,8 @@ class CueControl : public EngineControl {
ControlObject* m_pHotcueFocusColorNext;
ControlObject* m_pHotcueFocusColorPrev;

ControlProxy* m_pPassthrough;
Copy link
Member

Choose a reason for hiding this comment

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

Unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, because creating and connecting it obviously got lost when splitting cue control changes into separate commits.
so: No, I'll --fixup

@ronso0
Copy link
Member Author

ronso0 commented Jul 4, 2022

@daschuer
I switched back to Draft earlier because I had to check the event dependencies of some hotcue controls, as well as look for errors when splitting commits. The latter turned out to be true, just you noticed earlier and spent time reviewing while I planned to maybe rebase anyway : |
So I hope the time spent reviewing shortens the final round ; )

There is indeed a small issue with blocking Cue controls it seems:
loading a track with passthrough enabled would not light the Cue point (since Cue flashing is disabled) but it would also not seek to Cue after disabling passthrough. Before, seek request were queued. I'll take a look and I'll probably rebase again.

@ronso0 ronso0 force-pushed the waveform-show-passthrough-label branch from fc56c59 to b7be713 Compare July 4, 2022 19:20
@mixxxbot mixxxbot mentioned this pull request Aug 22, 2022
Closed
@ronso0
Copy link
Member Author

ronso0 commented Aug 31, 2022

There is indeed a small issue with blocking Cue controls it seems:
loading a track with passthrough enabled would not light the Cue point (since Cue flashing is disabled) but it would also not seek to Cue after disabling passthrough. Before, seek request were queued. I'll take a look and I'll probably rebase again.

This is workign as before. Somehow my 'track load point' was changed to 'Intro start'.

Ready for review.

@ronso0 ronso0 marked this pull request as ready for review August 31, 2022 21:38
daschuer
daschuer requested changes Sep 17, 2022
View reviewed changes
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.

This works good, we have only some violated debug assertions.

src/widget/wwaveformviewer.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Sep 18, 2022 •
edited
Loading

Thanks for your review @daschuer

I already fixed what you pointed out, as well as some leaking controls, though I obviously forgot to push.

@ronso0
Copy link
Member Author

ronso0 commented Oct 9, 2022

*ping

daschuer
daschuer approved these changes Oct 10, 2022
View reviewed changes
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, thank you.

@daschuer daschuer merged commit 3d846d3 into mixxxdj:main Oct 10, 2022
@ronso0 ronso0 deleted the waveform-show-passthrough-label branch October 29, 2022 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschuer daschuer daschuer approved these changes

Assignees
No one assigned
Labels
engine skins ui
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ronso0 @daschuer