Make X11 drop shadow size configurable (was: Revert huge shadow change)
ClosedPublic

Authored by rjvbb on May 18 2017, 11:34 AM.

Details

Summary

ab1fa49680d6a089af6bc6b4fa7849a75548a4db changed the shadow size from 6px to 30px, which is somewhat excessive.

I know this is just changing one magic value back to another, and it's subjective, but this makes it consistent with what other QtCurve themes were expecting; and it's something we've seen some reports complaining [1][2]

Adding a config option is a bit tricky as it's in the shared code between qt4 and qt5 and gtk2.

[1]https://www.reddit.com/r/kde/comments/6at2c1/what_kde_appsprojects_needs_the_most_lovesupport/dhhi261/
[2]https://github.com/QtCurve/qtcurve/issues/45#issuecomment-250686160

Test Plan

Shadows are as they used to be in the last release.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
davidedmundson edited the summary of this revision. (Show Details)May 18 2017, 11:54 AM
davidedmundson edited the test plan for this revision. (Show Details)
davidedmundson added reviewers: yuyichao, rjvbb.
rjvbb edited edge metadata.May 18 2017, 12:55 PM

From what I understand these shadows concern among others those of pop-up and drop-down menus? I don't know if I'd appreciate those becoming 5x smaller in my own theme setting. I'll try on my end but could you post screenshots before and after too, please?

Making this an option will not be that hard, it will just require copying the value from the regular opts struct to a variable in the shared library. The simplest way would be to have a global variable in x11helpers.cpp, with a setter function that gets called with the value read from the config file or the config dialog.

It may actually be more complicated to change the value at runtime, the code doesn't seem to written with that possibility in mind.

Another thing: Yichao, do you know it this shadow size value has an equivalent for non-X11 implementations?

yuyichao requested changes to this revision.May 18 2017, 2:34 PM

changed the shadow size from 6px to 30px

No it doesn't (it does in image size, but not shadow size). It also changed the formula so the change in (HWHM) size is only about 2x and it also makes it lighter. The actual change is.

So I'm certainly fine with an option (passed in as a parameter to X11Init functions) but we shouldn't change the shadow to 5x smaller (which would basically be turning it off)

This revision now requires changes to proceed.May 18 2017, 2:34 PM
rjvbb added a comment.May 18 2017, 2:41 PM

So I'm certainly fine with an option (passed in as a parameter to X11Init functions)

A parameter to X11Init means the effect of changes cannot be evaluated without restarting the application. Also, those functions are called before the configuration is read from file.

I'll have a look.

rjvbb added a comment.May 18 2017, 5:25 PM

Here's a draft implementation to put this property under user control. I've put it in the Advanced section, but wasn't sure exactly how to call the control if the parameter doesn't represent the exact size (= I've also left out the unit).

Done only for Qt5, Qt4 and GTk will follow when/if this is acceptable.

Nice fix. Thanks

I think the name is fine. It's still the "size" . It's a gradient that fades away with the end being at this many pixels away.

rjvbb added a comment.May 18 2017, 6:29 PM

You're welcome. Maybe I'll "commandeer" this revision from you then, unless you feel like doing the copy/pasting to Qt4 and GTk yourself? ;)

rjvbb commandeered this revision.May 21 2017, 9:40 PM
rjvbb edited reviewers, added: davidedmundson; removed: rjvbb.

Taking over from David.

rjvbb updated this revision to Diff 14739.May 21 2017, 9:47 PM
rjvbb edited edge metadata.
rjvbb edited the test plan for this revision. (Show Details)

this is a full patch enabling control over the X11 shadow size with spinboxes in the Advanced panel of the Qt4 and Qt5 KCMs.

The non-X11 implementation has drop shadows too; it'd be nice to couple their size to the new control too, if possible.

rjvbb retitled this revision from Revert huge shadow change to Make X11 drop shadow size configurable (was: Revert huge shadow change).May 21 2017, 9:48 PM
rjvbb set the repository for this revision to R626 QtCurve.
davidedmundson added inline comments.May 24 2017, 9:23 PM
lib/utils/x11helpers.cpp
336

why? 0 is a valid input

rjvbb marked an inline comment as done.May 24 2017, 11:15 PM
rjvbb added inline comments.
lib/utils/x11helpers.cpp
336

Indeed, I guess I missed a key there :)

rjvbb updated this revision to Diff 14826.May 24 2017, 11:20 PM
rjvbb marked an inline comment as done.

accept 0-size drop shadows

rjvbb set the repository for this revision to R626 QtCurve.May 24 2017, 11:21 PM
yuyichao added inline comments.May 25 2017, 1:51 AM
lib/utils/x11helpers.cpp
339

Have you tested the behavior when a shadow is registered and then destroyed?

rjvbb added inline comments.May 25 2017, 7:47 AM
lib/utils/x11helpers.cpp
339

If you mean did I try changing the shadow size at runtime via the spinbox in the configuration dialog, then yes, I did. I didn't notice any problems, but only checked the shadow with drop-down menus.

I think it'd be a good idea if you tested the patch too. As the author of the underlying code you might (should) have more intimate insight of what might go wrong.

looks good to me

yuyichao accepted this revision.Jun 1 2017, 1:07 PM
This revision is now accepted and ready to land.Jun 1 2017, 1:07 PM