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: style main menubar #3788

Merged
merged 7 commits into from May 2, 2021
Merged

Skins: style main menubar #3788

merged 7 commits into from May 2, 2021

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 14, 2021

Deere (I made the toolbar a bit less dark so it blends with the menubar)
image

@ronso0 ronso0 changed the base branch from main to 2.3 April 14, 2021 00:46
@github-actions github-actions bot added the skins label Apr 14, 2021
@ronso0 ronso0 added this to In progress in 2.3 release via automation Apr 14, 2021
@ronso0 ronso0 added this to the 2.3.0 milestone Apr 14, 2021
@ronso0 ronso0 mentioned this pull request Apr 15, 2021
@ronso0 ronso0 force-pushed the mainmenu23 branch 2 times, most recently from 35b9363 to 6328c7d Compare April 19, 2021 16:45
@JoergAtGithub
Copy link
Member

May I recommend to increase the contrast of the check box border. On view angle dependend LCD displays, it can be difficult to see that there is a check box.

@ronso0
Copy link
Member Author

ronso0 commented Apr 19, 2021

@JoergAtGithub
Can you post a draft? Just play with the qss values. in the style sheets the important selectors for checkboxes are
WTrackMenu QMenu::indicator:checked (track menu's Crates submenu)
#MainMenu QMenu::indicator:checked (menubar, checkbox is an SVG image in that case)

Even though I see your demand I think we shouldn't bend over too much to compensate mediocre hardware, because the additional contrast may not be desired when using contemporary IPS panels.

@JoergAtGithub
Copy link
Member

JoergAtGithub commented Apr 19, 2021

This is good to recognize from any angle:
grafik
I changed the SVG to: <rect x="1.0" y="1.0" width="17" height="17" rx="1" ry="1" stroke="#C0C0C0"

For reference the original:
grafik

@Holzhaus
Copy link
Member

Holzhaus commented Apr 19, 2021

To be honest I like more dim (original) version much better.

@ronso0
Copy link
Member Author

ronso0 commented Apr 19, 2021

Me too. I can try to brighten the border a bit, but your proposal is going too far for PaleMoon

@daschuer
Copy link
Member

daschuer commented Apr 20, 2021

I have also stumbled over this thinking the menu bar is grayed out.

Sure am biased as a embedded developer, but I think we are on the wrong track here. Please consider the following:

The overall impression of a skin depends on a the picked output rgb values and the monitor it is displayed on.

If we consider a sRGB Monitor the colors are calibrated to produce the same look at all devices at a 80 cd/m² white luminance at center position of the monitors brightness control.

Only a minimum of users have monitor capable of displaying all sRGB colors AND use it with the sRGB brilliance settings. Some have a weak device where there is no visual diffence between two colours in the RGB neighborhood.

If you like pale skin you have the option to reduce the contrast of the input RGB value or lower the brilliance settings to get the same color in the sRGB color space.
The later method is much more preferred, because it fixes the contrast issue on weak lcd displays and avoids glowing edges due to a full power background lighting behind a black area.

There is also another issue with sourrunding apps if you are not in full screen mode. Using PaleMoon the other apps are kind of dazzling. This is extrem if you open a web page with s white backrounds. After going back to Mixxx PaleMoon you cannot see anything for some seconds. This can already an issue in Mixxx if you display for instance a white cover art.

lt;dr
I recommend to increase the contrast in favour of a lower brilliance setting.

@ronso0
Copy link
Member Author

ronso0 commented Apr 20, 2021

oh, I think we've just discovered the issue that the menubar menus have the wrong font color. That's my mistake that slipped through in the previous PR. (the checkbox request is unrelated)
The main menu should look like this, like the other menus:
image

Do we really need a bright menubar? In that case the screen argumentation would also apply to the toolbar and I'd surprised if that comes up now, months (a year?) after PaleMoon has been merged.
image

@daschuer
Copy link
Member

Do we really need a bright menubar?

No, not at all. The original issue was only the checkbox border in the menu. I can imagine that it is not visible at all on some screens.

All elements of a skin should be consistent for a full screen view.

My long text was more a general advice how to design displays for night use.

For my understanding PaleMoon focuses dark booths, where the display should be eye pleasant, when looking to other objects in the booth and back to the display.

I am afraid it fails for this use case when I see the bright default windows border decoration. It works only in windowed mode if the windows decorations also dark and no bright app is shown.
The background lighting shine through effect cannot be fixed.

So if Pale Moon actually focusses this use case, it should be tested with the display brightness at minimum. And we should verify if the contrast ratio is still good enough to be readable even on poor displays.

I have not find time to check. For verification, we need a photo including some sourrunding objects. A screenshot did not work, because it is displayed with the background light of the viewing device.

@ronso0
Copy link
Member Author

ronso0 commented Apr 20, 2021

I understand, thanks.

I'm using PaleMoon with very low backlight in club environment since I started working on it, and it feels very pleasant for 8h+ with fog and drinks and all the disturbing 'real live effects' ; )
I have a T580 with 15" FullHD IPS display which has rather mediocre/bad specs. ~55% sRGB coverage, ~240cd/m² max, limited viewing angle.

From my experience I'd say my IPS panel is somewhere in the midfield between old, dim, low-contrast panels and contemporary office panels with good or high brightness and contrast and much better sRGB coverage, let alone Macbook panels.
btw: I designed Tango to work well on such older screens. I tested it once with a graphics designers calibrated IPS display and the contrast was stressing. Thus PaleMoon's medium contrast is definitely targeting more capable screens.
Also older screens have mor likely a lower resolution thus the GUI is also more likely to be scaled differently (dpi) and requiring less contrast (see WCAG contrast recommendations).

For verification, we need a photo including some sourrunding objects.

Photos are not suitable to judge this. Digital cameras can hardly reproduce the dynamic range of the human eye, also photo results vary from camera to camera, let alone implicite image post-processing.
If we seriously want to start an evaluation now we'd need to rely on personal feedback of users, maybe comparing PaleMoon with other apps designed for nighttime.

I can test again with my 2012 laptop with an TN panel to adjust label colors in order to find a better compromise. I'll take WCAG 2.0 level AA recommendations as orientation (though we already did that check in the initial PaleMoon PR IIRC).

sidenote: I find the contrast in Deere very poor, even now with IPS, and that was our default skin for some years with receiving complaints.

@daschuer
Copy link
Member

I have just tested PaleMoon in a almost dark room, with dimmed background lighting. The overall contrast is well.
There are only some problematic regions, which can only barley seen, especially when the viewing angle is not optimal. It are these regions:
grafik
grafik
grafik

The library content and the dimmed header play nicely:
grafik

This is interesting, because the color is not much brighter, but the font is bold and the color is warmer. So I guess if we take the header color and add a bit brightness to compensate the narrowness, it will work well (At least on my display which might have its own issues).

Another issue is that the hot-cue buttons are distracting bright, compared to the more important main cue buttons and all other controls:
grafik

Can be fix that by a smaller colored area or do we need to introduce a color filter for the buttons?

Bright covers where not an issue during my test.

@Holzhaus
Copy link
Member

I suggest we continue this discussion on Zulip and restrict this PR to the main menu bar styling.

@daschuer
Copy link
Member

The linked thread issues another topic. For my understanding the menu bar contrast topic is fully explored here. If @ronso0 can confirm my findings, I think we have a plan.

@daschuer
Copy link
Member

I can confirm the menu bar looks good now. I would prefer a bit warmer grey though, but that is a matter of taste.

Did you also consider to fix the MIN/BAL lable? Or is this a topic of another PR.

@ronso0
Copy link
Member Author

ronso0 commented Apr 22, 2021

#888 for the main menu was just a quick shot. I'll adapt that to be warmer.
What keeps me from marking this ready for review are menu issues / inconsistencies on Windows. I plan to fix and maybe find a way to reduce maintenance effort with all the redundant qmenu size/position properties tonight.

There'll be a separate PR fixing the broken SVGs and the visibility issues when I've thested with my old laptop.

@ronso0 ronso0 force-pushed the mainmenu23 branch 2 times, most recently from dc30711 to 83056d9 Compare April 26, 2021 01:40
@ronso0
Copy link
Member Author

ronso0 commented Apr 26, 2021

This is ready for final testing!

  • added indeterminate checkmarks
  • moved redundant main menu styles to common file per platform, next to default.qss

Please check those menus for consistent appearance

  • main menubar
  • all line edit menus: spinboxes, edit track tags in the table, searchbox, edit hotcue label
  • cover art menu
  • table header menu
  • track menu, especially checkboxes in the Crates submenu (locked/inactive, checked, unchecked, indeterminate)
  • library sidebar menu, also checkboxes in the Crates menu

I'll give this a shot on Windows again soonish and force-push if necessary.

@ronso0 ronso0 marked this pull request as ready for review April 26, 2021 02:05
@ronso0
Copy link
Member Author

ronso0 commented Apr 27, 2021

@poelzi Oh saw your comment only now. May your fix went into main?
Edit I don't see a related change in https://github.com/mixxxdj/mixxx/blame/main/src/widget/wcolorpicker.cpp
If you have that fix anywhere available to be split off for 2.3 fine. Otherwise, fine as well : ) this is just a small glitch. and off-topic anyway...

@poelzi
Copy link
Contributor

poelzi commented Apr 27, 2021

commit eb3b2fd was the fix

@ronso0
Copy link
Member Author

ronso0 commented Apr 27, 2021

ah okay. that was for the hotcue menu, here it's about the color picker in the track menu. The issue seems to be that adjustSize() is doing what it should only after the first show event.

@poelzi
Copy link
Contributor

poelzi commented Apr 27, 2021

Ahh, my bad. I would guess same fix. call updateLayout() in the init method so the size in correct when show() is called. I will look into this this week

@ronso0
Copy link
Member Author

ronso0 commented Apr 27, 2021

I tried the fix already, it doesn't work. The issue is really about resizing parent (m_pColorMenu) to content (adjustSize()) is not working for QMenu unitl it's shown the first time. Seems there are possibly ugly workaounds with a fake resizeEvent().
Though this has no priority -IMO- compared to finishing the #3063 #3063 (comment) because the color picker looks perfect from/after the second time it's opened.

@Holzhaus
Copy link
Member

Holzhaus commented Apr 27, 2021

Looks good, but the gap between the checkbox and the label is a bit too large for the library column headers:

Screenshot from 2021-04-27 22-41-14

And the checkboxes themselves look different when comparing the menu bar checkboxes with the context menus (or am I making this up?)

Screenshot from 2021-04-27 22-40-35
Screenshot from 2021-04-27 22-39-58

@ronso0
Copy link
Member Author

ronso0 commented Apr 27, 2021

@Holzhaus

the gap between the checkbox and the label is a bit too large for the library column headers:

How does it look like in 2.3?
How does the sidebar menu look like?
I think need to apply some negative margin,. which has no direct effect here (xfce / Qt 5.21.8) except shrinking the menu in width, but for you (Qt version?) and on Windows that could remove the gap.

And the checkboxes themselves look different when comparing the menu bar checkboxes with the context menus (or am I making this up?)

Yes, the 'internal' menus use px sizes, a css border and SVG checkmarks to style the checkboxes.
For the main menu I use em so all sizes adapt to the system font size. I chose SVG checkboxes to achieve the same outcome for all Qt versions and all Windows and Linux systems we support -- because trying to write css so that the outcome is a properly placed square checkbox for everyone, and also the hovered checkbox + item looks nice....not my cup of tea anymore, very exhausting.._
Blurred box borders with some configurations are the trade-off for reliability : |

@Holzhaus
Copy link
Member

How does it look like in 2.3?

I'll check tomorrow.

The menu bar checkboxes look nice, can we use them in context menus, too? Or is this too much hassle?

@ronso0
Copy link
Member Author

ronso0 commented Apr 28, 2021

hmm.. describe "nice" please.
I think you're seeing a blurry border tbh, so I unintended niceness I'd say ; ]

@Holzhaus
Copy link
Member

Qt version?

$ pacman -Q qt5-base
qt5-base 5.15.2+kde+r183-1

@Holzhaus
Copy link
Member

hmm.. describe "nice" please.
I think you're seeing a blurry border tbh, so I unintended niceness I'd say ; ]

Maybe :D The blur makes the checkbox border appear slighly rounded (like 1px radius or something), maybe that is what I find appealing. Anyway, I think the last remaining major issue is the wide gap between the checkbox and the label in the menu. Can you take care of that? Then we can merge IMHO.

@ronso0
Copy link
Member Author

ronso0 commented Apr 30, 2021

sure, when I'm back home I'll finish this. I guess the required stylesheet differences arw mostly due to different Qt versions, so the distinction Linux / Windows is probably not the ultimate solution, just a workaround that seemed to solve it in the initial wtrackmenu PR (2.1 / 2.2?)

Checkbox tweaking may happen in a follow-up as soon as I established a somewhat ciherent appearance here.

@Holzhaus
Copy link
Member

sure, when I'm back home I'll finish this. I guess the required stylesheet differences arw mostly due to different Qt versions, so the distinction Linux / Windows is probably not the ultimate solution, just a workaround that seemed to solve it in the initial wtrackmenu PR (2.1 / 2.2?)

Checkbox tweaking may happen in a follow-up as soon as I established a somewhat ciherent appearance here.

Maybe we should file a feature request to add something like the CSS media selectors to QSS, like:

@os windows and (qt-min-version: "5.15.2") {
  QLabel {
    background-color: lightgreen;
  }
}

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Great, looks fine. I think the risk that something for other OSes broke is small and it would be a cosmetic issue anyway. I'll merge now to unblock your follow-up PRs. If more patches are needed, these can go into follow-up PRs.

@ronso0
Copy link
Member Author

ronso0 commented Apr 30, 2021

let's merge this tomorrow.
I'll still need to check with Windows. I don't want to touch this again soonish :\

@ronso0 ronso0 marked this pull request as draft April 30, 2021 22:23
@ronso0
Copy link
Member Author

ronso0 commented May 2, 2021

Alright, looks good on Windows and Linux.
Ready for merge.

1322088 is the only c++ change that slipped in -- the Crates submenu has an id now which allows to style it individually.

@ronso0 ronso0 marked this pull request as ready for review May 2, 2021 02:10
Copy link
Member

@Holzhaus Holzhaus 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!

@Holzhaus Holzhaus merged commit a146898 into mixxxdj:2.3 May 2, 2021
2.3 release automation moved this from Needs Review to Done May 2, 2021
@ronso0 ronso0 deleted the mainmenu23 branch May 2, 2021 12:14
@daschuer
Copy link
Member

daschuer commented May 3, 2021

Thank you very much for all the work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.3 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants