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

Non-floating stem controls for LateNight #13537

Merged
merged 11 commits into from
Sep 23, 2024
Merged

Conversation

JoergAtGithub
Copy link
Member

@JoergAtGithub JoergAtGithub commented Aug 4, 2024

This is an alternative to the UI PR #13515 by @acolombier . It use classic widget groups instead of the floating widget.
grafik

grafik

The functionality is the same, but the widget positioning differs. Both PRs are in prove of concept state and not visually fine tuned.

The benefit of the floating widgets is that the waveform is only hidden by the stem widgets for the tracks that are stem files, while here, the stem widgets are either shown for all tracks or for none.

Please test both PRs #13515 and this one. (Build with FFMPEG and STEM enabled in CMake) and load a .stem file. And review the code of the second commit here. Which approach should we follow?

If you need STEM files for test, you can download the reference files here: https://www.native-instruments.com/fileadmin/ni_media/downloads/stems/free_stems_for_traktor_pro_2.zip

@acolombier
Copy link
Member

That's looking very promising! 🚀

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

had a quick look. in pretty good shape. I just wish the *Manager classes wouldn't get bloated so much, but there is not much we can do about that because Manager classes are fundamentally flawed...

src/effects/effectsmanager.cpp Outdated Show resolved Hide resolved
src/effects/effectsmanager.cpp Outdated Show resolved Hide resolved
src/engine/channels/enginedeck.cpp Outdated Show resolved Hide resolved
src/mixer/playermanager.cpp Outdated Show resolved Hide resolved
src/qml/qmleffectsmanagerproxy.h Outdated Show resolved Hide resolved
src/skin/legacy/legacyskinparser.cpp Outdated Show resolved Hide resolved
src/skin/legacy/legacyskinparser.cpp Outdated Show resolved Hide resolved
@acolombier
Copy link
Member

Thanks for the review @Swiftb0y !

Looks like most of those comments are meant for #13123, on which this PR is based. I will address them, but feel free to continue the review there, in case you have more feedback!

@Eve00000
Copy link
Contributor

Eve00000 commented Aug 8, 2024

I really appreciate the work/time your investing in this matter, so I did a thorough test and I wanted to write a profound report.
due to some errors on my behalve I had a rather strange effect that made it on the other side very clear to compair (see image).

I want to start with a +/- list
subject / design 1 / design 2
** indicating clearly a stem is loaded
-> design 1 only shows stem controls when a stem is loaded, design 2 shows them stem controls fora all decks even when there is no (stem) file loaded. the floating widget seems to be more scalable than the fixed. if you don't need the guicontrols to manipulate the stems they take a lot of space in design 2. Having 4 decks + samplers (even mini view) leaves no space for library.
-> this can / is confusing. A possible solution can be to have little rectangles (same design as Daniel uses in the Keys) in the stemcontrols that show the trackcolor as wel. (maybe the stars can be that color to)

** clarity of the stemcontrols
->the coloring of the tracks in design 1 was/is a little too 'invasive', the colors blinded the readability of the information (loaded effect, volume/fx potmeter level) especially when stems are loaded in all decks. The opacity of the controls when a stem-track is muted is clear. design 2 has the mute buttons as well.
-> design 2 is more 'relaxing" for the eyes, but a stem-track-color would clearify the view from a distance.

** floating or fixed
-> in the floating design the controls cover all views that are not showing the complete waveform (big library, 4-> 2 decks with stems loaded in 3/4) can be confusing. In the fixed design the controls are always there (unless hidden with the button) witch has an influence on the waverforms window as well. (4 decks = huge)
-> I find the fixed design more relaxing

Resumé
Both designs have + and - , so the optimal design must be somewhere in the middle (the truth is out there),

  • stem-track-colored rectangles indicating a stem is loaded, can the opacity be combined?
  • can the effectname be smaller?
  • can there be a different design for "only info" / "info + manipulate", the controls in the first could be smaller
  • can there be an indication in the deck that a stem is loaded?
  • floating design is scalable and leaves place for library , samplers and fx, the fixed design doesn't.

So there should be thought about diffeent user types:

  • using the gui as info ->smaller (hidable) stemcontrols
  • using the gui to control Mixxx -> bigger clear controls

PS: The track in the image remembered me of the early 90's, for a couple of weeks I was overwelmed with "Bells of NY', I got a white label and played (parts of) it maybe 5 times a night. Thanks for remembering me.

afbeelding

@daschuer
Copy link
Member

Here my 2 ct:

  • The floating version was too colorfull.
  • The uncovered area in the floating type was useless
  • The changing size was more anoying than helpfull.
  • Requrement: Alignment of deck waveforms
  • The colored texts may not work well / are readable with all Skin Color schemes and stem colors -> extra area or border
  • The combined waveforms sometimes hide details in single stems. Idea: Side by side version.
  • Nice: Show/Hide controls per deck. Waform chopped on the left, not scaled to always match non stem decks.
  • Force a minimm wavform size that all controls have the normal size
  • I have no demand to show and hide the waveforms, but a demand to switch back to stereo mode and let the track flow without a high CPU load. Maybe an extra small widget that is shown in stereo mode and allows to switch back to stem mode showing the full controlls.

@JoergAtGithub JoergAtGithub changed the title RFC: Alternative non-floating stem controls for LateNight Non-floating stem controls for LateNight Aug 29, 2024
@JoergAtGithub JoergAtGithub marked this pull request as ready for review August 29, 2024 19:27
@ronso0
Copy link
Member

ronso0 commented Sep 8, 2024

Just a quick feedback / idea, let me know what you think:
the play/mute controls could be buttons with a label (not labels + small buttons)
Ie. I'd prefer larger controls as GUI equivalent for pads on (stem-ready) controllers.

I'll provide a commit fixing the indentation soonish, then take a look at the UX and the c++ part.

Copy link
Member

@acolombier acolombier left a comment

Choose a reason for hiding this comment

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

LGTM!

@ronso0's suggestion sounds great, but also conscious than working on the legacy UI is not very easy/rewarding so happy with the current look!

@JoergAtGithub
Copy link
Member Author

Yes, a WStemButton would also make sense, an saves some space. I derived WStemLabel from WLabel, but I guess it should be easy to do the same with WPushButton instead.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

can't comment on the qss and xml changes (@ronso0 could you have a look?) Cpp changes LGTM.

@ronso0
Copy link
Member

ronso0 commented Sep 9, 2024

Yes, working on it right now.

@ronso0
Copy link
Member

ronso0 commented Sep 9, 2024

Fixup is here 3975edd
I fixed the knobs, adjusted the layout a bit, etc.

FWIW the action buttons in LateNight indicate what happens when you push them (not the current state, eg. | | icon when a track is playing), so I changed the stem mute icons, too.
image

@ronso0
Copy link
Member

ronso0 commented Sep 9, 2024

Fixup is here 3975edd

whoops, this had some residues in the SVG.
here's the clean one 709da44, please pick that instead.

@JoergAtGithub
Copy link
Member Author

@ronso0 Is it intended, that the Effect-Name is no longer left alligned?
grafik

@ronso0
Copy link
Member

ronso0 commented Sep 9, 2024

@ronso0 Is it intended, that the Effect-Name is no longer left alligned?

Whoops, only tested with long names. Will fix it.

I didn't change the Classic icon. Will take a look.

@ronso0
Copy link
Member

ronso0 commented Sep 10, 2024

Done. Polished commit is b08fe0e

@JoergAtGithub
Copy link
Member Author

Done. Polished commit is b08fe0e

This looks good! Thank you!

@JoergAtGithub
Copy link
Member Author

I noticed two visual issues, which are independent of the last commit:
grafik

1.) With very long stem labels, only the channel section with this label becomes wider. This is an issue because the waveform shrinks and is no longer aligned to the other waveforms. I think we will need an additional invisible container around all channels
2.) The default quick effect can be "---" or ""

Than we have also a build issue. We have the CMake setting STEM to enable the feature, but this does not affect the skin, only the C++ code. AFAIK an #ifdef like command in the skins is not available?

@ronso0
Copy link
Member

ronso0 commented Sep 10, 2024

1.) With very long stem labels, only the channel section with this label becomes wider. This is an issue because the waveform shrinks and is no longer aligned to the other waveforms.

  • We could set a fixed width and elide too long texts. The ... ellipsis would indicate there's more and a tooltip would provide the entire label.
  • Sure a parent container would work, but IMO that should have a max width.
  • QLabel::setWordWrap(true) could also work to get multi-line text (not tested).

@ronso0
Copy link
Member

ronso0 commented Sep 10, 2024

We have the CMake setting STEM to enable the feature, but this does not affect the skin, only the C++ code. AFAIK an #ifdef like command in the skins is not available?

Some ideas:

  1. abuse new ControlObjects to hide the widgets
    = cumbersome
  2. add a wrapper class for WidgetGroup, eg. StemWidgetGroup, that is only parsed if STEM is enabled
    (+) safe IMO
    (-) we'd need to implement this for every widget class used for stems that is placed in regular widget groups, eg. some stem control in the mixer where wrapping in an extra StemWidgetGroup would be overkill
  3. add a <RequiresStem> node to WidgetGroups in question, and only parse the WidgetGroup if STEM is enabled
    (+) should be easy to implement in LegacySkinParser::parseWidgetGroup
    (+) could also be a helper function called by any widget parser

3 is my favourite.

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

LGTM, except we need to add assertions when accessing the steminfo list.

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

ronso0 commented Sep 18, 2024

@JoergAtGithub I'm currently travelling and i'll review as soon as I'm back home.

@Eve00000
Copy link
Contributor

@ronso0 : enjoy your holiday, have a good time

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

ronso0 commented Sep 19, 2024

ronso0@b9da04d fixes the tabs in waveform_container.xml

@ronso0
Copy link
Member

ronso0 commented Sep 20, 2024

Than we have also a build issue. We have the CMake setting STEM to enable the feature, but this does not affect the skin, only the C++ code. AFAIK an #ifdef like command in the skins is not available?

What about not ifdef'ing the stem label parser itself (which gives us a skin warning "Invalid node name in skin: .."), but use the ifdef in parseStemLabelWidget to log a note saying that WStemLabel requires building with STEM enabled?

Add bounds check for m_stemNo in WStemLabel::slotTrackLoaded
@ronso0
Copy link
Member

ronso0 commented Sep 21, 2024

LGTM, but the indentations of the xml files are screwed up again. I think you need to adjust your editor to use spaces instead of tabs like in c++ code. Fixup or append a commit, as you like.

@JoergAtGithub
Copy link
Member Author

Done!

Copy link
Member

@ronso0 ronso0 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 very much!

@ronso0 ronso0 merged commit 64a4248 into mixxxdj:main Sep 23, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants