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

(usability) Waveform prefs: group options, adjust tabstops, reorder ui file #13615

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Aug 30, 2024

This improves usability in the waveform preferences by grouping the options:
Waveforms / Overview / Caching /OpenGl info

waveform-pref-groups_s

The checkbox
[ ] Enabled
looks a bit weird in the 2nd column. Could as well be
Enabled [ ] (in 1st column)

Note:
Unfortunately there is an issue with QGroupBox: it seems to have some minimal height which is not reached with 3 items, which in turn unnecessarily expands the group box. A spacer at the bottom of the page fixes this, downside is the extra space at the bottom (didn't manage to minimize that..)

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 really comment much on the ui file changes but ur in the expert in that anyways so there is not much to argue here

src/preferences/dialog/dlgprefwaveform.cpp Outdated Show resolved Hide resolved
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.

LGTM. thank you. Does anyone want to look at the ui file?

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.

Thank you. This is already an improvement.

Here some comments:

  • The Group box lable "Wavform" can become "Scrolling Wavewform" or such, to better distinguished it from the "Waveform Overview".
  • The checkboxes around the Waveform comobox are looking spreaded. Can we put the m all in one line?
  • The title column in both group boxes are not aligned.
  • If you disable the waveforms, it feels random what is grayed out and what not. In addition the hint about missing accelerations appears. Do we need to gray out anything at all? In case of a crashing config, the user may want to keep the checkbox disabled, adjust the settings an than enable.

If you prefer to merge it as it is, just give a hint.

@ronso0
Copy link
Member Author

ronso0 commented Sep 1, 2024

The title column Type labels in both group boxes are not aligned.

Fixed.

@ronso0
Copy link
Member Author

ronso0 commented Sep 1, 2024

If you disable the waveforms, it feels random what is grayed out and what not. In addition the hint about missing accelerations appears.

I torn about this.
I think it unused / inapplicable options shoud indeed be grayed out. About the 'Requires acceleration' hint: I implemented that approach for the skin settings in LateNight, but these hints are overlays there and don't change the layout.

Therefore, IMO all (scrolling) waveform options should be grayed out.
Except... the Visual Gain settings, since they're also applied to the overviews if normalization is disabled.
(which I noticed just now, after using Mixxx for almost ten years 😆 )
I think we should add a hint in the overview group.

@ronso0
Copy link
Member Author

ronso0 commented Sep 1, 2024

The Group box lable "Wavform" can become "Scrolling Wavewform" or such, to better distinguished it from the "Waveform Overview".

Yep, fixed.

@ronso0
Copy link
Member Author

ronso0 commented Sep 1, 2024

The checkboxes around the Waveform comobox are looking spreaded. Can we put the m all in one line?

Fixed.

@ronso0 ronso0 added the waveform label Sep 6, 2024
@ronso0 ronso0 added this to the 2.6-beta milestone Sep 6, 2024
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.

3 participants