Added optional transparency/blur to menu frames
ClosedPublic

Authored by anemeth on Jan 28 2018, 10:34 PM.

Details

Summary

Added option to set transparency and blur behind menu frames such as right click context menu, toolbar menu, etc.
Transparency and blur is disabled by default.

Test Plan

Diff Detail

Repository
R31 Breeze
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
anemeth requested review of this revision.Jan 28 2018, 10:34 PM
anemeth edited the summary of this revision. (Show Details)Jan 28 2018, 10:36 PM
anemeth edited the test plan for this revision. (Show Details)
anemeth added reviewers: hpereiradacosta, Plasma, VDG.
anemeth edited the summary of this revision. (Show Details)
anemeth edited the summary of this revision. (Show Details)Jan 28 2018, 10:40 PM
anemeth retitled this revision from Added transparency/blur to menu frames to Added optional transparency/blur to menu frames.
zzag added a subscriber: zzag.EditedJan 29 2018, 3:45 PM

Looks great!

BUT, it would be nice to have a little bit bigger contrast. Could you please enable background contrast and upload a screenshot what it looks like?

zzag added inline comments.Jan 29 2018, 3:48 PM
kstyle/breeze.kcfg
188

Redundant comment.

zzag added inline comments.Jan 29 2018, 4:11 PM
kstyle/breeze.kcfg
189
<entry name="MenuTransparency" type=""UInt">
  <default>0</default>
  <max>100</max>
</entry>

maybe something like this?

kstyle/breezestyle.cpp
3636

buggy..

background.setAlphaF(StyleConfigData::menuTransparency() / 100.0f);

In D10170#197812, @zzag wrote:

Looks great!

BUT, it would be nice to have a little bit bigger contrast. Could you please enable background contrast and upload a screenshot what it looks like?

I presume you mean the Background contrast effect. It was in the half-enabled state. I checked it to be fully enabled but I did not notice any difference.
However changing the transparency percent improves the contrast, since the default frame background color is (252, 252, 252).
Initially I uploaded 40% transparency to demonstrate the effect, anything lower than that might have been too subtle in a screenshot.
Anyways here are some transparency percent value screenshots with the background contrast effect enabled:

40%:

30%:

20%:

10%:

anemeth added inline comments.Jan 29 2018, 5:07 PM
kstyle/breeze.kcfg
188

What's redundant about it?

zzag added a comment.Jan 29 2018, 5:10 PM

I presume you mean the Background contrast effect. It was in the half-enabled state. I checked it to be fully enabled but I did not notice any difference.

Did you call KWindowEffects::enableBackgroundContrast?

kstyle/breeze.kcfg
188

It's obvious that the following entry is about transparency.

In D10170#197849, @zzag wrote:

I presume you mean the Background contrast effect. It was in the half-enabled state. I checked it to be fully enabled but I did not notice any difference.

Did you call KWindowEffects::enableBackgroundContrast?

Just tried that.
Nothing changed.

anemeth added inline comments.Jan 29 2018, 5:30 PM
kstyle/breezestyle.cpp
3636

The slider has 10 steps and goes from 0 to 10, that's why it's / 10.0
100 steps would be too much in my opinion.

And we need to invert the value of it because the setAlphaF works inverted to our slider and it needs a value between 0.0 and 1.0
That's why we need the 1.0 -
It would be great if we could set the slider to go from 10 to 0 instead, but I don't see how.

zzag added inline comments.Jan 29 2018, 5:48 PM
kstyle/breezestyle.cpp
3636

IMO, transparency should be in range 0-100.

It would be great if we could set the slider to go from 10 to 0 instead, but I don't see how.

Why?

zzag added a comment.Jan 29 2018, 5:58 PM

Just tried that.
Nothing changed.

Try to change contrast, intensity, and saturation args.

Hello,
Hi,
thanks for the patch !

Few comments:
1/ the code does not compile under kde4 (cmake -DUSE_KDE4). You would need to use the proper #ifdef to prevent this

2/ I think the "blur" option should go. Blur should be controlled centrally by the desktop effect. In other words: BlurBehind should always be set to true, and then left to kwin to handle. Having an extra option here seems like micro-management. Why would you need blur behind plasma widgets (as handled by the desktop effect) and not behind menus ? (or vice versa). Also, right now, selecting "blur" in the style settings, but unselecting the desktop effect results in no blur behind menu, and hence inconsistency with what the style option says.

3/ the slider should follow the same convention and design as for the translucency effect: "transparent <----slider---> opaque". 0% should be transparent, 100% should be opaque.
Also, should have the same granularity, so ... percents. (not 10%)

4/ I would really like the VDG opinion on this. IMHO I am not sure if it fits the breeze "guidelines" or design-ideas, in the sense that I do not know why this is needed (what is the use case), aside from "this is pretty".

In other words: why would one need to be able to see what is behind the menu you are currently navigating ?
Navigating a menu is a very focused thing one wants to do: one is looking for a specific action to perform on the current content of the window. To me at least, seeing-through the menu is more a distraction to this task than anything else. (and adding blur to reduce this distraction would in fact go in the direction of not being translucent at all).

If the only reason is "this is pretty", then this has to be balanced with the extra option, code complexity, and configurations to be tested on the maintainer side, that this patch adds.

abetts accepted this revision.Jan 31 2018, 4:02 PM
This revision is now accepted and ready to land.Jan 31 2018, 4:02 PM

Dear Andy,
With all due respect, please stop "accepting" revisions. Revisions should be accepted by the code maintainer, who then endorse the responsibility that the code then remains in a workable state forever after. You are not the breeze maintainer.
In my understanding at least, accepting a revision allows the submitter to commit the code as is (as in "this revision is now accepted and ready to land").
In the current state, this revision is _not_ accepted and ready to land.

You can, of course, vote +1 or -1 to a change, and it is then up to the maintainer to accept/reject/request changes to the revision, taking into account (or not), your +1s or -1s.

Thanks in advance,

Hugo

abetts removed a reviewer: abetts.Jan 31 2018, 4:19 PM
This revision now requires review to proceed.Jan 31 2018, 4:19 PM

Hello,
Hi,
thanks for the patch !

Few comments:
1/ the code does not compile under kde4 (cmake -DUSE_KDE4). You would need to use the proper #ifdef to prevent this

2/ I think the "blur" option should go. Blur should be controlled centrally by the desktop effect. In other words: BlurBehind should always be set to true, and then left to kwin to handle. Having an extra option here seems like micro-management. Why would you need blur behind plasma widgets (as handled by the desktop effect) and not behind menus ? (or vice versa). Also, right now, selecting "blur" in the style settings, but unselecting the desktop effect results in no blur behind menu, and hence inconsistency with what the style option says.

3/ the slider should follow the same convention and design as for the translucency effect: "transparent <----slider---> opaque". 0% should be transparent, 100% should be opaque.
Also, should have the same granularity, so ... percents. (not 10%)

4/ I would really like the VDG opinion on this. IMHO I am not sure if it fits the breeze "guidelines" or design-ideas, in the sense that I do not know why this is needed (what is the use case), aside from "this is pretty".

In other words: why would one need to be able to see what is behind the menu you are currently navigating ?
Navigating a menu is a very focused thing one wants to do: one is looking for a specific action to perform on the current content of the window. To me at least, seeing-through the menu is more a distraction to this task than anything else. (and adding blur to reduce this distraction would in fact go in the direction of not being translucent at all).

If the only reason is "this is pretty", then this has to be balanced with the extra option, code complexity, and configurations to be tested on the maintainer side, that this patch adds.

Maybe I can add some extra reasons. I am not sure if having the extra menu to control Blur is "necessary." However, I see value in having it in our KCMs or as part of an effect. One thing that this patch accomplishes is to have a blurred effect consistent in more areas of the desktop. While now we are able to make windows transparent and blur them, an option to have menus follow that convention is not present. Imagine that maybe a window style or theme allows transparency but that the menus don't look well with the preset and you would like to change it independently, There is no option for that. My thought is that if is about theme/style consistency, this makes sense to have. I feel, however, that it fits better in a KCM or as part of the transparency effect options. Thoughts anyone else in the VDG? @colomar , @ngraham , @andreask

abetts removed a reviewer: VDG.Jan 31 2018, 4:29 PM
zzag added a comment.Jan 31 2018, 5:32 PM

Also, please, take into account contrast issues. That's nice to have blur, but, please, use it wisely.

ngraham added a comment.EditedJan 31 2018, 5:42 PM

FWIW, I don't think I'd ever want this enabled by default, and I'm leery of adding the option to Breeze. I'm not sure how much it really fits the visual style. Only very very subtle blur transparency would ever work here without becoming garish and distracting; the screenshot of the feature looks terrible to me, I'm sorry to say. And using only a very a low level of blur transparency IMHO calls into question what the point even is.

For years, macOS experimented with making things transparent and blurry. People made fun of them. Then Microsoft did it, but even worse. People made fun of them, too. Since then, both of those companies have mostly moved away from blur transparency in their operating systems, or made it optional. I really don't see how letting people move closer towards their historical design mistakes helps us here.

zzag added a comment.Jan 31 2018, 6:07 PM

Since then, both of those companies have mostly moved away from blur transparency in their operating systems, or made it optional.

Did they?

Microsoft is doing some work on Fluent design (https://fluent.microsoft.com/).
macOS is still using blur extensively.

Eh, maybe I'm wrong. Still... [shakes old man stick] I don't like it! These whippersnappers and their transparency! Back in my day things were opaque and we liked it that way!

I won't die on this hill though, since it's being proposed as an optional feature and not a new default.

Eh, maybe I'm wrong. Still... [shakes old man stick] I don't like it! These whippersnappers and their transparency! Back in my day things were opaque and we liked it that way!

I won't die on this hill though, since it's being proposed as an optional feature and not a new default.

I think that's a good way to look at it. I would be OK to make it an option.

zzag added a comment.Jan 31 2018, 6:38 PM

@ngraham: blur is OK but it should be used wisely. Blurring the hell out from UI is not really cool.

IMO, changes like this, require some changes in the "design language".
PS. does Plasma have a design language? (like Fluent or Material design)

In D10170#198655, @zzag wrote:

@ngraham: blur is OK but it should be used wisely. Blurring the hell out from UI is not really cool.

IMO, changes like this, require some changes in the "design language".
PS. does Plasma have a design language? (like Fluent or Material design)

Sort of/mostly: https://community.kde.org/KDE_Visual_Design_Group/HIG

anemeth updated this revision to Diff 26278.EditedJan 31 2018, 6:49 PM

@hpereiradacosta
1/ Done. No more errors when compiling with -DUSE_KDE4. Blur is not enabled with KDE4 breeze.
2/ Done. Blur option gone. Now blurs by default.
3/ Done. Now looks and behaves like the translucency effect.
4/ This is pretty.

@zzag
I got enableBackgroundContrast to work. It is very very poorly documented.
I played around with the values but the endresult does not look better.
In my opinion it looks worse if I change the contrast or intensity.

@ngraham
Windows 10 moves closer and closer to make everything transparent and blurry.
Fluent designed UWP apps make like 30% of the window transparent/blurred.
Titlebars are getting blurred.
macOS blurs pretty much every menu too.
It's not a bad design choice, but maybe a little bit flawed implementation on my part.
The first screenshot I included is too transparent. I only set it so high so it is visible.
Normally you would use around 20% instead of 40%.

zzag added a comment.Jan 31 2018, 7:01 PM

It would nice to have a fully-fledged design language which would specify grid stuff, animations, etc. Something like Fluent design(https://fluent.microsoft.com) or Material design(https://material.io/guidelines/#).

I think this should be a separate topic, so yeah...

zzag added a comment.Jan 31 2018, 7:02 PM

I got enableBackgroundContrast to work. It is very very poorly documented.
I played around with the values but the endresult does not look better.
In my opinion it looks worse if I change the contrast or intensity.

Could you please share screenshots?

Just to show you how it looks compared to something that is generally considered good design: Deepin
I choose the same wallpaper and menu locations.
I tested it with 30% and 40% transparency and 10/15 blur strength (that is likely going to be the new default).

KDE seems a little bit darker. That's because in Deepin the file manager background and the menu background is both white (255, 255, 255), while in KDE they are both gray (252, 252, 252).
I'm not sure a contrast trickery would improve it much.

OK, I have to admit that those examples look really, really good. 30% more than 40%.

januz added a subscriber: januz.Feb 1 2018, 1:02 AM

+1 I wouldn't have this as a default for Breeze, but it looks really cool. I'm sure it'd be a popular option, specially among those trying to mimic macOS.

hpereiradacosta added a subscriber: alake.

Hello,
thanks for taking care of 1, 2 and 3.

for 4 (the use case), I am adding @alake (one of the original designers of the breeze style) and @colomar as reviewer so that they have a chance to give their opinion too.
My personal (not so important) worry, is that we are adding yet another option to the style for something which is only eye candy (in my understanding it brings no improvement to usability, quite the contrary in fact) and the code complexity that goes with it. IMHO this change somewhat muddies Breeze's identity (flat and simple, not bloated) for no real gain. This said, I do appreciate your work.

Opinions welcome

Hugo

IMHO this change somewhat muddies Breeze's identity (flat and simple, not bloated) for no real gain. This said, I do appreciate your work.

The exact opposite.
By default the Breeze style desktop theme is transparent and blurry. Kicker and Kickoff are transparent. The plasmoids on the panel are transparent.
This change can just make it more consistent.

I think the "blur" option should go. Blur should be controlled centrally by the desktop effect. In other words: BlurBehind should always be set to true, and then left to kwin to handle. Having an extra option here seems like micro-management. Why would you need blur behind plasma widgets (as handled by the desktop effect) and not behind menus ? (or vice versa). Also, right now, selecting "blur" in the style settings, but unselecting the desktop effect results in no blur behind menu, and hence inconsistency with what the style option says.

KWin has no way of knowing which pixels should be blurred behind a window. That's the reason the hint exists.
If you set the hint unconditionally, the effect will be applied unconditionally. Even when the menus are fully opaque.

I think the "blur" option should go. Blur should be controlled centrally by the desktop effect. In other words: BlurBehind should always be set to true, and then left to kwin to handle. Having an extra option here seems like micro-management. Why would you need blur behind plasma widgets (as handled by the desktop effect) and not behind menus ? (or vice versa). Also, right now, selecting "blur" in the style settings, but unselecting the desktop effect results in no blur behind menu, and hence inconsistency with what the style option says.

KWin has no way of knowing which pixels should be blurred behind a window. That's the reason the hint exists.
If you set the hint unconditionally, the effect will be applied unconditionally. Even when the menus are fully opaque.

Correct.
So the call to enableBlurBehind should be made only if "asAlphaChannel" _and_ opacity < 1
This still does not need an extra option.
Added comment in the code about this.
Thanks for the clarification.

kstyle/breezestyle.cpp
3636

This should be called only if StyleConfigData::menuOpacity() is < 100
And in fact, I wonder if it is safe to make the call at every painting call.
I guess it depends on how the underlying function is implemented (does it first check for the property ?)

anemeth updated this revision to Diff 26329.Feb 1 2018, 4:09 PM

Now only enabled blur when the frame is transparent.

@hpereiradacosta
drawPanelMenuPrimitive only runs once per panel creation and not 60 times per second, right?
This is what enabled blur behind a window: https://github.com/KDE/kwindowsystem/blob/master/src/platforms/xcb/kwindoweffects.cpp#L245
I don't think it checks if it's already enabled.
But if it's called only once per panel then it's not an issue.

anemeth marked 4 inline comments as done.Feb 1 2018, 4:10 PM
hpereiradacosta added a comment.EditedFeb 1 2018, 4:20 PM

Now only enabled blur when the frame is transparent.

@hpereiradacosta
drawPanelMenuPrimitive only runs once per panel creation and not 60 times per second, right?

Well it is also probably called every time the selected (on mouse-over) item changes. So basically everytime you move the mouse in the list.
Now setting the property only once per menu (or rather, only at creation or when its sizes change), would require quite a bit more machinery. Probably something like a "breezeblurhelper" to which you would need to register the menu at creation (in ::polish) and then deal with resizeevent and configuration changes inside there.
Unless you feel like giving it a shot yourself, I could probably take care of that when this code is committed.
There is some code of this kind (more complex though), in the oxygen repository. It is called oxygenblurhelper.

This is what enabled blur behind a window: https://github.com/KDE/kwindowsystem/blob/master/src/platforms/xcb/kwindoweffects.cpp#L245
I don't think it checks if it's already enabled.
But if it's called only once per panel then it's not an issue.

anemeth updated this revision to Diff 26406.Feb 3 2018, 4:40 AM

Moved blur handling to breezeblurhelper

anemeth updated this revision to Diff 26407.Feb 3 2018, 4:42 AM

Added breezeblurhelper files

Hello,

Thanks for the updated patch and for the work on BlurHelper.
Couple more comments below.

Hugo

kstyle/breezeblurhelper.cpp
145

ok. So this is awkward. The whole code (the whole class in fact), does nothing for KDE4 (because of KWindowEffects not being available).
We should then not have the event filters installed, nor even the blur helper created, etc.
Alternatively, one would need to duplicate the code from KWIndowEffect. as done in the original oxygen code.

For KF5, it is definitly better to use KWindowsEffect. so fine.
For kDE4 applications, not having the blur behind the translucent window is an issue (and there still are some kde4 applications around).
So there, one should either

  • duplicate the code (for this #ifdef branch only
  • disable the blur (as now), and possibly also the transparency (in order not to endup with transparent menus and no blur).
kstyle/breezestyle.cpp
3639

This should go to Breeze::Style::polish.
There is already a "if( qobject_cast<QMenu*>( widget )" there. Just move this to the corresponding code block.

anemeth updated this revision to Diff 26658.Feb 6 2018, 4:21 PM

Implemented the changes suggested by @hpereiradacosta
Only KDE5 Breeze has transparency and blur now

anemeth marked 2 inline comments as done.EditedFeb 6 2018, 4:22 PM

KDE4 Breeze has no trasnparency and blur anymore
@hpereiradacosta can you point me to some KDE4 apps that are still in use? I'm just curious what hasn't been ported yet.

KDE4 Breeze has no trasnparency and blur anymore
@hpereiradacosta can you point me to some KDE4 apps that are still in use? I'm just curious what hasn't been ported yet.

Amarok at least, clementine (?),
vlc (?)
many qt4 based applications not ported to qt5, such as backintime

Anyway, thanks for the updated patch !
Lets wait for a couple more days for someone from VDG to show up (@colomar ? @alake ?), and if none shows up lets land it.

anemeth updated this revision to Diff 26814.Feb 9 2018, 12:13 PM

Fixed a visual glitch when using a scaled display

last round of modifications: simplifications of the blurHelper class.
Also, for the config ui, maybe it makes more sense to call the "Transparency" tab, only "Menus" and change "Menu:" into "Transparency:"
Unless of course one wants to add transparency behind other widgets.

Then I think it is good to go.

kstyle/breezeblurhelper.h
139

Because the use of this class is much simplified wrt the original one, it can be significantly simplified:

_timer (and timerEvent) are unnecessary.
Then update() could be removed, as well as _widgets, as well as the widgetDestroyed callback.
Then update(widget) could be called directly in the event filter.

anemeth updated this revision to Diff 27008.Feb 12 2018, 3:56 PM

Simplified the breezeblurhelper class

anemeth marked an inline comment as done.Feb 12 2018, 3:57 PM

Also, for the config ui, maybe it makes more sense to call the "Transparency" tab, only "Menus" and change "Menu:" into "Transparency:"
Unless of course one wants to add transparency behind other widgets.

Maybe in the future someone wants to add this transparency to (for example) dropdown menu, combobox, or other elements.
I say we should keep the naming vague.

kstyle/breezeblurhelper.cpp
45 ↗(On Diff #27008)

helper argument is not needed

kstyle/breezeblurhelper.h
53 ↗(On Diff #27008)

helper argument is not needed

80 ↗(On Diff #27008)

this can go.
No more slots :)

85 ↗(On Diff #27008)

this can go

Also, for the config ui, maybe it makes more sense to call the "Transparency" tab, only "Menus" and change "Menu:" into "Transparency:"
Unless of course one wants to add transparency behind other widgets.

Maybe in the future someone wants to add this transparency to (for example) dropdown menu, combobox, or other elements.
I say we should keep the naming vague.

Can you post an updated screenshot about the configuration option, and can we get VDG's opinion on it ?

anemeth updated this revision to Diff 27010.Feb 12 2018, 4:11 PM

Removed the _helper variable from the breezeblurhelper class

anemeth marked 4 inline comments as done.Feb 12 2018, 4:12 PM

Can you post an updated screenshot about the configuration option, and can we get VDG's opinion on it ?

This is how it looks now


I made it look like the Translucency effect config

hpereiradacosta accepted this revision.Feb 15 2018, 11:16 AM

ok. No reaction on the UI.
Ship it !
Thanks for the work and the patience.

This revision is now accepted and ready to land.Feb 15 2018, 11:16 AM

@hpereiradacosta I don't have commit access.
Could you please commit it for me?

This revision was automatically updated to reflect the committed changes.
zzag added a comment.Feb 16 2018, 5:01 PM

This breaks build. Missed breezeblurhelper.cpp and breezeblurhelper.h?

zzag reopened this revision.Feb 16 2018, 5:02 PM
This revision is now accepted and ready to land.Feb 16 2018, 5:02 PM
zzag added a comment.Feb 16 2018, 5:15 PM

I've reverted that commit.

This revision was automatically updated to reflect the committed changes.