Do not hardcode unhovered windows brightness on present windows effect
Needs ReviewPublic

Authored by salvacorts on Oct 20 2017, 9:56 PM.

Details

Reviewers
None
Group Reviewers
KWin
VDG
Plasma
Summary

BUG: 385522

I've added the functionality to let users choose the brightness of unhovered windows on present windows effect.

I dont know if that would work properly. Theoretically, now brightness can be choosed with a slider on present windows configuration menu.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
salvacorts created this revision.Oct 20 2017, 9:56 PM
Restricted Application added a project: KWin. · View Herald TranscriptOct 20 2017, 9:56 PM
Restricted Application added subscribers: KWin, kwin. · View Herald Transcript
ngraham added a reviewer: KWin.EditedOct 20 2017, 10:00 PM
ngraham added a subscriber: ngraham.

Can you add "BUG: 385522" to the Summary on its own line? That special keyword will cause the bug to get closed if this is accepted and merged.

Also, can you change the title to something short and descriptive? The title becomes the commit message.

salvacorts retitled this revision from Bug fix for https://bugs.kde.org/show_bug.cgi?id=385522 to Do not hardcode unhovered windows brightness on present windows effect.Oct 20 2017, 10:19 PM
salvacorts edited the summary of this revision. (Show Details)

Can you add "BUG: 385522" to the Summary on its own line? That special keyword will cause the bug to get closed if this is accepted and merged.

Also, can you change the title to something short and descriptive? The title becomes the commit message.

Sure, done.

I want input from the VDG whether they consider this a required feature.

Especially things to consider here:

  • The behavior we have has been around since 2008, why change after 9 years?
  • The config would be extremely hidden, nobody would know about it
  • The reasoning "but Windows does it", is not valid to me, we are not copy cats

From KWin side: PresentWindows is actually in change freeze since a long time. The code is fragile and I don't like to see it being changed again. The code is straight forward and I don't think it would cause issue, but I'm looking at the bigger picture here with having blocked changes for years because the code is fragile.

Hi!

Regarding to your considerations, that's why I think you might consider these changes:

  • The behavior we have has been around since 2008, why change after 9 years?

Because user tastes and desktops changes in 9 years. I don't think that's a good approach to not to accept a "feature request"/"Bug Fix"/"Improvement".

  • The config would be extremely hidden, nobody would know about it

No, its not. You can easily find it on "Desktop Behavior" menu. Someone who would be interested on tweaking that effect will find that easily. You can search it on the search field from system config indeed.

  • The reasoning "but Windows does it", is not valid to me, we are not copy cats

That was an example, also mac is one. I am not saying you to copy the effect from windows as "a cat". (If I want to see KDE look as Windows: I would be using windows). It was an example to show what I meant with unhovered windows brightness.

As you said. these changes would not unstabilize that effect; so I don't see why to not to take into account these changes.

Hi!

Regarding to your considerations, that's why I think you might consider these changes:

  • The behavior we have has been around since 2008, why change after 9 years?
Because user tastes and desktops changes in 9 years. I don't think that's a good approach to not to accept a "feature request"/"Bug Fix"/"Improvement".
  • The config would be extremely hidden, nobody would know about it

No, its not. You can easily find it on "Desktop Behavior" menu. Someone who would be interested on tweaking that effect will find that easily. You can search it on the search field from system config indeed.

I'm now hacking almost 10 years on KWin, and have been the maintainer for six, seven years. Trust me: if I say it's hidden, it is hidden. The number of users who will change this can be counted on one hand and that's in absolute numbers. It's a general problem with all the config options hidden in the desktop effect specific settings. It's only something for someone who actively looks through the config options. If you don't know that one can change the setting one will not find it.

I know that these "obscure" and hidden options are not configured, because every bug report in KWin asks for the support information. And the support information includes the options of all effects. We see that users hardly change the default effects, we see that they hardly change the configs. We even see that options are broken for years without anyone noticing.

That's why I am not so fond about adding further options. It's the solution for one person's workflow. That's fine it's open source, anyone is allowed to do it. But it comes with a cost: it means yet another config option. And if there is one thing KDE is negatively famous about is that we have space shuttle control modules. Due to that we need to be careful when adding new options.

We need to evaluate this carefully. Is the benefit of this option really worse it? Or if the change what this option is showing something we should just make the default?

FWIW, my preference is to change the default rather than adding another option, because as folks have pointed out, darkening non-selected windows in Present Windows mode can make it hard to distinguish the contents.

Even just reducing the amount of darkening would improve this IMHO, without the need for a new option.

zzag added a subscriber: zzag.Sep 7 2018, 1:32 PM

From KWin side: PresentWindows is actually in change freeze since a long time. The code is fragile and I don't like to see it being changed again. The code is straight forward and I don't think it would cause issue, but I'm looking at the bigger picture here with having blocked changes for years because the code is fragile.

That's pretty good argument!

Frankly, dimming windows is not really good idea, imho. Adding outline around windows would be much better, imho. On the other hand, as Martin said, the code is fragile. E.g., in some cases, calculateWindowTransformations can bring KWin to knees.

Maybe we could come up with something better when we port the Present Windows and the Desktop Grid effect to QML?

Zren added a subscriber: Zren.Nov 28 2018, 4:43 AM
Zren added inline comments.
effects/presentwindows/presentwindows.cpp
317

There's code in PresentWindowsEffect::postPaintScreen() that fires if it's using the magic number 0.3.

if (i.value().highlight != 0.3)

winData->highlight is used to tell us how far into a "scale + brightness" tween we're in.

data.multiplyBrightness(interpolate(0.4, 1.0, winData->highlight));
const qreal scale = interpolate(1.0, tScale, winData->highlight);
tx = qRound((tx-rect.x())*winData->highlight);
ty = qRound((ty-rect.y())*winData->highlight);
rect.setWidth(rect.width()*scale);
rect.setHeight(rect.height()*scale);

So it controls more than just the brightness.

We could change the code to:

data.multiplyBrightness(interpolate(m_unhoverBright, 1.0, winData->highlight));

Which I know works: https://www.youtube.com/watch?v=QEHC43zMIMc#t=6m00

IMHO, there should be no dimming at all; the whole point of the present windows effect is to use visual identification instead of having to read the title of each window. Visual id is faster, I think.

Removing the dimming effect altogether should be less code, which hopefully makes is less fragile.

ndavis added a subscriber: ndavis.EditedMar 26 2019, 8:57 PM

What if:

  • Windows are at full brightness when no window is hovered over
  • When the user hovers over a window, all other windows are dimmed

Pros:

  • Better readability when no window is hovered over, making it easier to scan all windows
  • It is still easy to tell what window you're about to select

Cons:

  • It may be difficult or annoying to not hover over a window by accident in some cases (depends on the pointing device, the number of windows and the aspect ratios of the windows).
  • There might be a performance impact from lowering the brightness of all except one window instead of only changing one window to full brightness
Zren added a comment.May 28 2019, 11:21 PM

I just realized how difficult "drawing a rectangle" outline is in C++ KWin Effects. We're required to have codepaths for OpenGL, XRender, and QPainter... uhg. No clue how to even test QPainter.

I was able to use the showpaint + snaphelper effects as examples for getting this. I currently draw a rectangle using GL_LINES around any window that has winData->highlight > 0. I think I'll try only outlining a single highlighted window and only if it's winData->highlight == 1 after the "zoom in" animation finishes. Now that I'm only outlining 1 window maybe. To animate the outline opacity, I'd need to draw multiple rectangles which is tricky.

jgahde added a subscriber: jgahde.Jan 15 2021, 12:02 PM
farion added a subscriber: farion.Sep 29 2021, 9:45 AM

And if there is one thing KDE is negatively famous about is that we have space shuttle control modules. Due to that we need to be careful when adding new options.

I'm not very deep in KDE hacking yet, but I'm using KDE since KDE 3. And for me "space shuttle control" is one of the USPs of KDE and usually my main argument in DE discussions. At some point a user will come to a situation where it is not satisfied with something. And than the user will search for the option in the config. Sometimes this search is hard, but if you find the option your problem is solved. If you don't find the option - like here - you'll be frustrated. That's why (usually silent) people like me are using KDE. If you don't want to maintain an option in the UI - cause the amount of people using it - why not just add it to the config file. I would say, that the default does not really matter. Both solutions are working somehow, and most people won't notice the difference.

meven added a subscriber: meven.Sep 29 2021, 1:07 PM

If you don't want to maintain an option in the UI - cause the amount of people using it - why not just add it to the config file. I would say, that the default does not really matter. Both solutions are working somehow, and most people won't notice the difference.

In D8388#321816, @zzag wrote:

From KWin side: PresentWindows is actually in change freeze since a long time. The code is fragile and I don't like to see it being changed again. The code is straight forward and I don't think it would cause issue, but I'm looking at the bigger picture here with having blocked changes for years because the code is fragile.

That's pretty good argument!

Frankly, dimming windows is not really good idea, imho. Adding outline around windows would be much better, imho. On the other hand, as Martin said, the code is fragile. E.g., in some cases, calculateWindowTransformations can bring KWin to knees.

Maybe we could come up with something better when we port the Present Windows and the Desktop Grid effect to QML?

The UI part was never the troubling part here.

The parent bug of 385522 https://bugs.kde.org/show_bug.cgi?id=303438 was closed due to the fact the presentwindow effect is to be deprecated in favor of the new overview effect that does not dim other windows in the first place.

I think we can close this differential, the overview effect show replace the presentwindow effect for Plasma 5.24 I believe.