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

Skin updates #3285

Merged
merged 5 commits into from
Feb 18, 2021
Merged

Skin updates #3285

merged 5 commits into from
Feb 18, 2021

Conversation

poelzi
Copy link
Contributor

@poelzi poelzi commented Nov 10, 2020

I was putting some love into the deer skin and was able to skin the menubar after some small changes. Since we are skinning, having a default menubar feels like a foreign object, so I consider this 2.3 worthy.

  • Deere: option for a larger UV master.
  • Indicate green/yellow area in the UV meters
  • Allow skinning of menubar Skins: allow menubar to be styled #3372
  • Skin Deer Menubar future PR by @ronso0
  • Small adjustments of the alpha value for track colors for better visibility (all skins)

Screenshot_20201111_002045

@daschuer
Copy link
Member

I cannot see a difference in case of Gnome or Unity.
Can you add also a "Before" Screenshot?

@daschuer
Copy link
Member

I am in doubt that the large VU Meter should be a preference option. The designer should decide.

@daschuer
Copy link
Member

OK there is a difference with Deere:
grafik

@poelzi
Copy link
Contributor Author

poelzi commented Nov 11, 2020

I am in doubt that the large VU Meter should be a preference option. The designer should decide.

I differ. Some people might like to have same sized UV meters and this is a simple option that does not much harm.

@poelzi
Copy link
Contributor Author

poelzi commented Nov 11, 2020

I cannot see a difference in case of Gnome or Unity.
Can you add also a "Before" Screenshot?

It depends on your system colors. If you run a dark skin it might look ok, on a light skin, at least on KDE, it looks ugly.

@Holzhaus
Copy link
Member

I cannot see a difference in case of Gnome or Unity.
Can you add also a "Before" Screenshot?

It depends on your system colors. If you run a dark skin it might look ok, on a light skin, at least on KDE, it looks ugly.

https://mixxx.org/screenshots/

res/skins/Deere/style.qss Outdated Show resolved Hide resolved
res/skins/Deere/style.qss Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented Nov 11, 2020

OK there is a difference with Deere:
grafik

One reason I didn't start working on styles for DlgtrackInfo is that once you start overwriting native styles you need to take care of all other styles, too, and make sure they look good on all platforms. Thus my library menu style PR has grown enormously and has taken ages to finish.

@ronso0
Copy link
Member

ronso0 commented Nov 11, 2020

Please rebase on 2.3 so crash fixes from #3144 are included and this PR is testable.

@ronso0
Copy link
Member

ronso0 commented Nov 11, 2020

I am in doubt that the large VU Meter should be a preference option. The designer should decide.

I differ. Some people might like to have same sized UV meters and this is a simple option that does not much harm.

@Be-ing ??

@poelzi
Copy link
Contributor Author

poelzi commented Nov 11, 2020

@ronso0 I felt your pain yesterday. So, maybe as a quick workaround we could just set the colors of the menus and a border.
With changing check symbols the pain started which might have to do with using SVG as icons, or at least, does not make it easier.

@poelzi
Copy link
Contributor Author

poelzi commented Nov 11, 2020

This is how 2.3 Deer looks with KDE on a light skin.
Screenshot_20201111_143147

@ronso0
Copy link
Member

ronso0 commented Nov 11, 2020

@ronso0 I felt your pain yesterday. So, maybe as a quick workaround we could just set the colors of the menus and a border.
With changing check symbols the pain started which might have to do with using SVG as icons, or at least, does not make it easier.

maybe take a look at a system theme and check how menus and checkboxes are styled there.
I doubt it fails because of SVGs, those work in the track menu. You may need to set the indicator size explicitely in css.

@poelzi
Copy link
Contributor Author

poelzi commented Nov 12, 2020

@ronso0 do you have some time helping on this one ? I have so many open points that this styling thing drives me nuts. You wrote that you did some styling of the menus already ?

@ronso0
Copy link
Member

ronso0 commented Nov 12, 2020

I think it'll take an entire evening to look into this and supply comprehensive styles, let alone applying this to all skins.
Unfortunately I don't have time for this until sunday.

@poelzi
Copy link
Contributor Author

poelzi commented Nov 12, 2020

@ronso0 even if it takes you 2 weeks, I would be super happy getting your skilled styling on board here. I tried different things this evening but could not get something look nice everywhere :(

@ronso0
Copy link
Member

ronso0 commented Nov 12, 2020

Are you asking just about the styling or about purging quirks like the one @daschuer experienced?

@ronso0 ronso0 added this to the 2.3.0 milestone Nov 16, 2020
@ronso0
Copy link
Member

ronso0 commented Nov 16, 2020

@poelzi
I suggest you split this up into smaller chunks to get the finished parts merged

  • Deere design
  • track color opacity
  • menubar styling, c++ requirements (no skin changes): we need to check what happens on macOS, Windows and various linux DEs, and since we're short on active Win/macOS testers it would be better to have style-capable builds available and users can test stylesheets without having to build Mixxx on their own
  • WIP menubar stylesheets

@ronso0 ronso0 marked this pull request as draft November 16, 2020 15:46
@Be-ing
Copy link
Contributor

Be-ing commented Nov 17, 2020

I agree with @daschuer and @Holzhaus. I am not a fan of the big meter option in Deere. It looks out of place to me and I don't think the option is really helpful. Having more options is not inherently good.

@ronso0
Copy link
Member

ronso0 commented Nov 17, 2020

I agree with @daschuer and @Holzhaus. I am not a fan of the big meter option in Deere. It looks out of place to me and I don't think the option is really helpful.

I think it could be a useful addition and we should make it the default.
But it's to big IMO compared to the channel meters. If we limit it to half the height of the mixer it looks good.
Also let's include the colored meter backpaths.

@ronso0
Copy link
Member

ronso0 commented Nov 17, 2020

  • menubar styling, c++ requirements (no skin changes): we need to check what happens on macOS, Windows and various linux DEs, and since we're short on active Win/macOS testers it would be better to have style-capable builds available and users can test stylesheets without having to build Mixxx on their own

IMO we should definitively pick that for 2.3 and subsequently style the menubars in point releases.
This feature has been requested for ages.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 17, 2020

IMO we should definitively pick that for 2.3 and subsequently style the menubars in point releases.

I think that effort would be better spent getting rid of the menu bar.

@ronso0
Copy link
Member

ronso0 commented Nov 17, 2020

IMO we should definitively pick that for 2.3 and subsequently style the menubars in point releases.

I think that effort would be better spent getting rid of the menu bar.

There's already the next step in progress #3189 getting rid of the menu bar. After that we'll have to rework The Menu (however that will look like) and make it accessible for screen readers.
That'll take ages, and 2.3 will be the stable version for ~2 years :: looking at our release tempo o_o`
That said, it doesn't make much sense to me accepting the current situation while the styling groundwork is already done here.

@poelzi I you don't have time to split off the menubar theming prerequisities I'll open a separate PR with that commit and finish it in the next weeks.

@ronso0
Copy link
Member

ronso0 commented Nov 26, 2020

I picked up 851adbe for #3372

@ronso0
Copy link
Member

ronso0 commented Nov 30, 2020

Alright, I managed to adjust sizes and margins of the menu item, checkboxes & icons by using em
image
PR coming soon...

At first I was puzzled why the font-sizes wouldn't react to OS font size changes, then I realized it would only obey font settings made in qtconfig / Qt5 Settings (thus the italic font above).
I think we should not mess with the menu font settings since those are important accessibility parameters users need to be able to adjust by themselves (and we don't have to worry about the layout as we have to in the Mixxx skins), so either omitting menu font sizes in the stylesheets or setting them to 1em ensures readable menus and should not create accessibility regressions.

By setting some aburd font I also noticed that Mixxx font settings are not complete: Qt config could screw up GUI buttons, labels, table headers etc. I think there are only a few adjustments missing, so I guess I can fix all of them for 2.3, too.

@poelzi poelzi force-pushed the skin-updates-menustyling branch from 87175a2 to b24df79 Compare December 2, 2020 17:22
@poelzi
Copy link
Contributor Author

poelzi commented Dec 2, 2020

@ronso0 I removed the skin styling commits here and only left the large VU meter option in deer, the dimmed background image in VU and the increase in background color in the libraries. I adjusted them individually depending on how clear the background is visible.

@poelzi poelzi marked this pull request as ready for review December 4, 2020 02:38
@poelzi poelzi requested a review from ronso0 December 6, 2020 16:18
@ronso0
Copy link
Member

ronso0 commented Dec 6, 2020

re large VU meter in Deere:
some tweaking is necessary to align it with some channel controls. ronso0@c9dc500 shoud do the trick.

re track colors:
LGTM, could you also increase it in Shade?

@ronso0
Copy link
Member

ronso0 commented Dec 6, 2020

btw I made some good progress with the menubar styling (Deere only as of now).
I hope to finish that and apply the styles to the other skins this week.

@ronso0
Copy link
Member

ronso0 commented Dec 7, 2020

re large VU meter in Deere:
some tweaking is necessary to align it with some channel controls. ronso0@c9dc500 shoud do the trick.

re track colors:
LGTM, could you also increase it in Shade?

btw to keep the commit history clean please make use of git --fixup ... and git rebase -i --autosquash for this PR because a) it is rather small and b) more about the effect than about reviewable code. https://fle.github.io/git-tip-keep-your-branch-clean-with-fixup-and-autosquash.html
thx!

@ronso0 ronso0 removed the ui label Dec 10, 2020
If the option is enabled, the master UV meter will takeup
as much space as possible. Especially on the 4 deck settings
with all controls enabled, the UV meter is very small.
Show in the unlight UV meter which parts are green and yellow
through a darker version of the normal meter.
Slightly increase the alpha value so the track color is better visible.
@poelzi poelzi force-pushed the skin-updates-menustyling branch from b24df79 to f11330f Compare December 13, 2020 20:13
@poelzi
Copy link
Contributor Author

poelzi commented Dec 13, 2020

@ronso0 I tried your patch but the spacing on top just did not make sense for me. The purpose of the large VU option is, to use as much space as possible for the main VU meter. Only the spacer on the bottom for crossfader assignment was required otherwise it looked strange.

@ronso0 ronso0 changed the title Skin updates & menustyling Skin updates Dec 18, 2020
@ronso0
Copy link
Member

ronso0 commented Dec 18, 2020

@poelzi I'll take another look at this during the weekend.

@ronso0
Copy link
Member

ronso0 commented Jan 4, 2021

@poelzi I know the Deere mixer can be a pita to work with but in this branch the alignment of the large Main VU is horrible in some configurations. We should not sacrifice asthetics just for having a large VU meter.
Please try to fix that somehow with conditional spacers.
image

Edit whoopsy, the VU alignment is broken in 2.3 already: with 2 decks and crossfader hidden the EQs and VUs look for example.

@poelzi
Copy link
Contributor Author

poelzi commented Feb 6, 2021

I was not able to reproduce this on my branch, it always looks ok

@ronso0
Copy link
Member

ronso0 commented Feb 14, 2021

this is the config I refer to: 4 decks, separate waveforms
image
how does that look like for you?

please just add some spacers for this config and we can merge.

@ronso0
Copy link
Member

ronso0 commented Feb 14, 2021

with ronso0@95db5d7 it looks like this, there's an offset only when there's no quick effect configured
image

pick this, double-check that it looks okay to you and then we can merge.

<SingletonContainer>
<ObjectName>StereoVUMeterMaster</ObjectName>
</SingletonContainer>
</Children>
Copy link
Member

Choose a reason for hiding this comment

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

@poelzi
Copy link
Contributor Author

poelzi commented Feb 18, 2021

@ronso0 done, looks good here

@ronso0
Copy link
Member

ronso0 commented Feb 18, 2021

Okido, the rest LGTM
Thanks!

@ronso0 ronso0 merged commit f31c2e3 into mixxxdj:2.3 Feb 18, 2021
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.

5 participants