Differential D23480

[kwin/effects/presentwindows] Remove window dimming and move title bars to bottom
Needs RevisionPublic
Actions

Authored by cameronrodgers on Aug 26 2019, 11:44 PM.

Details

Reviewers
ngraham
Group Reviewers
KWin
VDG
Plasma
Summary

This patch removes the dimming on windows in Present Windows, but keeps the dimming on the background. It also moves the title bars to the bottom of the windows to further increase readability and ease of visual recognition. In order to prevent title bars overlapping windows, the spacing between windows in flexible grid is increased a bit. Here is a screenshot:


IMO it is clearly much better looking and more useful this way.

There is another revision being discussed that only changes window dimming, and includes a setting for it in the configs:
D8388
I think people in that discussion may be overthinking it. Shouldn't we make the simplest change first (before adding outline effects, etc)? I haven't seen anyone indicate that they actually like the darkening effect. It's visually confusing. Why not just get rid of it outright?

Please feel free to merge this thread/mark duplicate if you think it's appropriate (though this isn't an exact duplicate, and changes the title bars), I am new and don't know the methodology here. I just hope that these two issues are fixed soon, since they are simple fixes.

Test Plan

Apply patch to kwin, compile, and run to see the change.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
Restricted Application added a subscriber: kwin. · View Herald TranscriptAug 26 2019, 11:44 PM
cameronrodgers requested review of this revision.Aug 26 2019, 11:44 PM
romangg added a subscriber: romangg.Aug 26 2019, 11:46 PM

In your summery upload screenshots directly here instead of using third party services. Link to other revisions by mentioning DXXXX instead of full http links.

cameronrodgers edited the summary of this revision. (Show Details)Aug 27 2019, 12:16 AM
In D23480#519885, @romangg wrote:

In your summery upload screenshots directly here instead of using third party services. Link to other revisions by mentioning DXXXX instead of full http links.

Done.

zzag added a subscriber: zzag.Aug 27 2019, 3:47 AM

I think people in that discussion may be overthinking it

No, they are not. There's has to be some way to show what currently highlighted client is. Apparently the most straightforward thing to do besides dimming clients is to draw outline. However, it's worth to point out that present windows effect or rather C++ version of it is feature frozen.

cameronrodgers added a comment.Aug 27 2019, 4:12 AM
In D23480#519908, @zzag wrote:

I think people in that discussion may be overthinking it

There's has to be some way to show what currently highlighted client is.

The windows still get bigger when selected/moused over, and if the close button is enabled, that also indicates it.

However, it's worth to point out that present windows effect or rather C++ version of it is feature frozen.

I can't claim to know the process behind that, but it seems like a lot of people want it to be changed (and have for quite a while). Is the QML rewrite being developed or planned somewhere? The original wishlist bug report for this issue was posted two years ago... a small patch would have been a good solution to tide people over and improve the UX in the meantime.

ngraham accepted this revision.Aug 27 2019, 1:23 PM
ngraham added a subscriber: ngraham.

I think the UI changes in here are good. An outline would be good too, but the hovered-over window does grow a bit bigger, which is fine.

"X is feature-frozen" is the most frustrating reason to reject a patch and I don't think it's valid. If something is feature-frozen pending a rewrite, the maintainer needs to step up and do that work. If the maintainer isn't going to do that, then the code can't be feature-frozen, because that amounts to just killing development entirely, which is inappropriate for a piece of open-source software developed in public.

This revision is now accepted and ready to land.Aug 27 2019, 1:23 PM
ngraham requested changes to this revision.Aug 28 2019, 5:31 PM

Actually I found a UI regression that happens with this patch. There's now a white/gray line underneath the title:

This revision now requires changes to proceed.Aug 28 2019, 5:31 PM
davidedmundson added a subscriber: davidedmundson.Aug 28 2019, 5:51 PM

If we do remove the highlight (and I'm willing to say, that's purely aesthetics and therefore not my realm) we should remove all the code that does the highlight.

This still animates, and still updates every frame during the animation only to show the same value.

cameronrodgers added a comment.EditedAug 28 2019, 9:29 PM
In D23480#521324, @ngraham wrote:

Actually I found a UI regression that happens with this patch. There's now a white/gray line underneath the title:

Good catch. This seems to be related to the Blur effect, as it doesn't occur if Blur is turned off in Desktop Effects. I agree it's a regression for the title bars, but this problem is actually more widespread. The current code (without this patch applied) already has the white/gray/transparent line around icons, and around (some) title bars. Here is a screenshot of the current Present Windows effect:


Do we have any clues on how to prevent this? I'll be looking into it later.

In D23480#521330, @davidedmundson wrote:

If we do remove the highlight (and I'm willing to say, that's purely aesthetics and therefore not my realm) we should remove all the code that does the highlight.

This still animates, and still updates every frame during the animation only to show the same value.

Unfortunately, it appears that the highlight variable is also used in window scaling calculations:
https://github.com/KDE/kwin/blob/master/effects/presentwindows/presentwindows.cpp#L369
As a result, I think most or all of the code calculating the "highlight" still plays some role. Since "highlight" would no longer actually have anything to do with window brightness, would it be appropriate to rename the variables and functions to reflect the change in functionality?

davidedmundson added a comment.Sep 26 2019, 12:20 PM

FWIW, I intend to look into effect frames which will hopefully address this.

I don't think the "feature freeze" applies as this isn't introducing a new feature.

I think @zzag has a point that it's important to show which window has focus - but maybe we can just put a highlight on the EffectFrame.

ngraham added a comment.Oct 10 2019, 3:48 PM

@cameronrodgers I think renaming the variable is OK. Please feel free to continue the work on this.

cameronrodgers added a comment.Oct 11 2019, 5:58 AM
In D23480#544775, @ngraham wrote:

@cameronrodgers I think renaming the variable is OK. Please feel free to continue the work on this.

I've been busy with schoolwork, but I did a bit more testing and I think it's pretty clear that blur is interacting with the title bars in a bad way.


As you can see in the pic, the color of the messed up title bar edges (and icons) reflects the color of the background image. Some are greenish, some white, blue, etc.
As well, the visual regression isn't a result of changing any of the highlight code (reverting the highlight stuff doesn't fix it in my patch). It's from moving the title bars over the edges of the windows, so that the blur effect on the edges takes in some of the wallpaper color. The problem is actually there in the code without this patch, it's just not very visible (if at all) because the title bars are centered on the windows.

In D23480#538095, @davidedmundson wrote:

FWIW, I intend to look into effect frames which will hopefully address this.

EffectFrame is probably a good place to look for the source of the problem. I want to take a look at it, but it may be a bit beyond me.

jgahde added a subscriber: jgahde.Jan 15 2021, 12:01 PM

Revision Contents
Changeset List

Diff 64710

View Options

effects/presentwindows/presentwindows.cpp

Loading...