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 :: adjust styles of skin context menus, tooltip… 2nd attempt #2363

Merged
merged 47 commits into from
Feb 14, 2020

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 16, 2019

Synchronise styles of skin right-click menus, tooltip, ...
[2nd attempt after #2337 was merged too early. See conversation there...]
[hopefully all changes are included here. ]

This should definitely be included in the 2.3 release because it makes the entire regular GUI interaction a consistent experience. No more OS themes crushing in, except popup windows like file dialogs and warnings.

It affects

  • Tracks table menu
  • hotcue menu
  • cover art menu
  • tooltips
  • effect selector drop-down
  • table header menu
  • editable fields (Search box, beat spinboxes, library items)
  • skin settings menu (Tango, LateNight, maybe Deere)
  • sidebar menu

This is how the popups etc. are supposed to look like:
menus-popups-etc_deere
menus-popups-etc_latenight
menus-popups-etc_shade-dark
menus-popups-etc_tango

ToDo

  • use relative font size where applicable? Nope, maybe later on..
  • style indeterminate (partially checked) checkboxes (tracks' Crates menu)
  • style editable fields
  • Shade: fix library selection colors in all color schemes
  • include AutoDJ trasition mode drop-down from make better use of intro/outro markers with AutoDJ #2103
  • remove WEffectSelector style hack from MacOS stylesheets
  • restrict QScrollBar, QTreeView, QTableView and QTextBrowser styles to Mixxx widgets like the library, WEffectSelector and #fadeModeComboBox (don't style QFileDialogs etc.)
  • restrict QHeaderView styles to Mixxx widgets (#Library)
  • restrict QMenu styles to Mixxx widgets (#Library, WOverView, WSEarchLineEdit, QSpinBox)
  • include WCueMenuPopup src/widget: Replace CueMenu implementation with a custom popup #2362

Please test with various themes, also on Windows and MacOS, also without desktop styles (window manager olnly, like i3wm)

@ronso0 ronso0 changed the title Revert "Revert "Skins :: adjust styles of skin context menus, tooltip… Skins :: adjust styles of skin context menus, tooltip… 2nd attempt Nov 16, 2019
@ronso0
Copy link
Member Author

ronso0 commented Nov 16, 2019

It would be nice if someone with MacOS 10.11 could test if the effect selector drop-down looks like this, i.e. checkmarks drawn properly, no offset/cropped list items:
image

@ronso0 ronso0 marked this pull request as ready for review November 18, 2019 09:26
@ronso0
Copy link
Member Author

ronso0 commented Nov 18, 2019

@esbrandt Could you please test Deere again? The closed effect selector should now have a dark bg color.

@@ -7,7 +7,7 @@
}


/* hack to hide the checkmark, otherwise text gets cut off
/* Previously necessayr hack to hide the checkmark, otherwise text gets cut off
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just delete the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

For documentation. It took a while to notice and figure out the fix, so its valuble info IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather have the code deleted if it is no longer necessary. It is still in the Git history if that info is needed later.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 20, 2019

IMO the contrast for highlighted menu items in Deere should still be improved.

@ronso0
Copy link
Member Author

ronso0 commented Nov 20, 2019

I'll take a look.

@Holzhaus
Copy link
Member

Holzhaus commented Nov 21, 2019

Thanks @ronso0, nice work!

I noticed a few more issues while testing:

The context menus look bit too large on my 4K screen:

2019-11-21-142430_609x451_scrot

2019-11-21-142039_1439x1821_scrot

Same for tooltips:
2019-11-21-142631_3840x2160_scrot

The file picker (for selecting a new cover) looks broken:
2019-11-21-142105_1518x1390_scrot

@ronso0
Copy link
Member Author

ronso0 commented Nov 21, 2019

Thanks for testing!

Idk how the file picker is affected by the changes.
On which OS are you?
I'll try to limit the QScrollBar style to WLibrary QScrollBar, WLibrarySidebar QScrollBar, and find a similiar selector for the dialog bg.

About the menu/tooltip font size:
we have several font issues in master, and it would be nice to solve those for 2.3:

  • search box / table header don't scale with the Library font size set in Preferences
  • library icons don't scale consistently with the Library font size ...
    (BPM lock, 'played' checkbox, Preview button)

For now I'll remove the fixed fon sizes from the menu, tooltip and other elements, hoping they use the OS default size.

@daschuer
Copy link
Member

Out of curious, what are the native dimensions of the cover?
What are your display scaling settings?

@Holzhaus
Copy link
Member

Out of curious, what are the native dimensions of the cover?

Either 500x500px (embedded in the FLAC tag) or 550x550px (cover.jpg in the same folder). I don't know which one Mixxx uses.

What are your display scaling settings?

None AFAIC. I don't have a DE installed (using i3wm via startx) and didn't set any scaling settings. Qt seems to determine the scale factor automatically from the screen size. I can disable it via QT_AUTO_SCREEN_SCALE_FACTOR=0, but that's about it.

@ronso0
Copy link
Member Author

ronso0 commented Nov 21, 2019

I'll try to limit the QScrollBar style to WLibrary QScrollBar, WLibrarySidebar QScrollBar

...and do the same for QHeaderView. Uff

@ywwg
Copy link
Member

ywwg commented Nov 21, 2019

Does this PR also cover style of dialog boxes spawned by right click menus? Right now they are hard to read on LateNight:

image

@Holzhaus
Copy link
Member

@ronso0: Idk how the file picker is affected by the changes.
On which OS are you?

I use Arch btw.

@ronso0
Copy link
Member Author

ronso0 commented Nov 25, 2019

Does this PR also cover style of dialog boxes spawned by right click menus? Right now they are hard to read on LateNight:

image

I remember experiments with overview labels with those exact colors (css green & blue) in #2342 but in this branch I don't use those styles.

@ronso0
Copy link
Member Author

ronso0 commented Nov 25, 2019

@Holzhaus Do you see the skinned file selector scrollbars also in Deere?

I'll try to limit the QScrollBar style to WLibrary QScrollBar, WLibrarySidebar QScrollBar

...and do the same for QHeaderView. Uff

Scrollbars may also pop up in the effect selector if someone has a lot of external effects enabled.
Double Ufff..

to avoid styling any QFileDialog that's parented to #Mixxx main widget.
@ronso0
Copy link
Member Author

ronso0 commented Nov 25, 2019

@Holzhaus Could you please pull and test the cover dialog again with LateNight?
With the previous commit all scrollbar and background styles should be appplied to Mixxx widgets only.

@ronso0 ronso0 mentioned this pull request Nov 25, 2019
2 tasks
@Holzhaus
Copy link
Member

@ronso0 Voilà:
2019-11-25-202420_3840x2160_scrot

@ronso0
Copy link
Member Author

ronso0 commented Nov 26, 2019

alright, thx!
with i3wm only, previously the Mixxx styles must have popped up all over the place. so your screens are good for working on keepin all Mixx styles, well within Mixxx..

@ronso0
Copy link
Member Author

ronso0 commented Jan 21, 2020

Thanks for testing this again & again!
Ok, so in your case CueNumberLabel & CuePositionLabel don't inherit styles from WCueMenuPopup but have to be addressed explicitely. Will fix.

I chose a rather large spacing for menu items for better readability, font size is still set by OS, which is probably also the reason for large appearance in your screenshot with 100% scaling on 4k UHD.
I'm curious how it looks like when you run it with scale_factor=2.0

@ronso0
Copy link
Member Author

ronso0 commented Jan 21, 2020

If the menu item spacing is considered too wide with 100% scaling by other users, as well, I'd decrease that, of course.

@ronso0
Copy link
Member Author

ronso0 commented Jan 22, 2020

remaining ToDos:

  • in-depth review on macOS
  • opinions on the menu items spacing

Apart from that this is ready to merge IMO so it receives broader testing from those who rely on pre-built binaries. I bet we'll discover minor issues anyway, but those can be fixed later on.

@Holzhaus
Copy link
Member

When using Tango, the content of the label field is not visible unless selected:

2020-01-23-132700_619x358_scrot

opinions on the menu items spacing

I think the spacing is ok. For some reason, I feel like all custom UI elements (i.e. everything execept library and menu bar) are too small on my 4K screen (or library and menu bar are huge when using QT_SCALE_FACTOR=2), but that issue is unrealated to this PR.

@ronso0
Copy link
Member Author

ronso0 commented Jan 23, 2020

When using Tango, the content of the label field is not visible unless selected:

Fixed. I just double-checked the other QLineEdit styles and I think this was the last missing piece in this regard.

library and menu bar are huge when using QT_SCALE_FACTOR=2

you refer to the table header?

@Holzhaus
Copy link
Member

Nope, the top menu bar. The table header is actually fine, it's just that the entries are large in comparison:
2020-01-23-153221_3840x2160_scrot

2020-01-23-153024_3840x2160_scrot

@Holzhaus
Copy link
Member

By the way, in Tango there are some icons visible below the "No Headphones configured". I think this is a bug, but this also happens in master:
2020-01-23-153749_158x47_scrot

@ronso0
Copy link
Member Author

ronso0 commented Jan 23, 2020

By the way, in Tango there are some icons visible below the "No Headphones configured".

It's opaque now.
The PreviewDeck cover I'd leave translucent, though, so there's some feedback for when loading / previewing a track from the library. It'll be hard to make the (UX) situations with unconfigured devices consistent for mic, vinyl and headphone (how control canges are processed / allowed)...

@Holzhaus
Copy link
Member

Holzhaus commented Feb 6, 2020

This is ready to merge, but needs some MacOS testing, right?

@Holzhaus
Copy link
Member

Holzhaus commented Feb 7, 2020

I just checked again. In direct comparison, the spacing between the items could be a bit smaller. But I guess that's just personal preference.

2020-02-07-133748_602x461_scrot
2020-02-07-134128_466x422_scrot

@ronso0
Copy link
Member Author

ronso0 commented Feb 7, 2020

This is ready to merge, but needs some MacOS testing, right?

Yep, final feedback from MacOS is missing.
Besides the merge conflicts, this is left to do:

  • somehow remove middle elide of effect names in LateNight, this is impractical n small screens (can't tell what's causing this)
  • reduce menu item spacing

@ronso0
Copy link
Member Author

ronso0 commented Feb 7, 2020

somehow remove middle elide of effect names in LateNight

for some reason the label of the (closed) effect selector is shown elided sometimes, indeveloper mode though. strange..

@ronso0
Copy link
Member Author

ronso0 commented Feb 7, 2020

@Holzhaus the qss check has proven to be very helpful once more. Thx!
especially now that I can't test locally..

@Holzhaus
Copy link
Member

Is this ready to review and merge?

@ronso0
Copy link
Member Author

ronso0 commented Feb 14, 2020

this is ready for some time already.
still needs testing/confirmation on MacOS & WIN.
can't tell who to ping for that right now.

I still think we can merge and fix minor issues afterwards when nightly builds are avaliable dor everyone

@Holzhaus
Copy link
Member

We could merge this now and get feedback during the 2.3 beta phase.

@ronso0
Copy link
Member Author

ronso0 commented Feb 14, 2020

Yup! +1

@Holzhaus Holzhaus merged commit 4ef6ab9 into mixxxdj:master Feb 14, 2020
@Holzhaus
Copy link
Member

Done. Thank you for your work!

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.

6 participants