[kstyle] Refine shadows
ClosedPublic

Authored by zzag on Mar 9 2018, 10:58 AM.

Details

Summary

Refine shadows in order to match decoration shadows. See D11069

The refined KStyle shadows(from Small to Very Large)

Desktop experience with the refined shadows

Depends on D11198

Diff Detail

Repository
R31 Breeze
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zzag created this revision.Mar 9 2018, 10:58 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 9 2018, 10:58 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
zzag requested review of this revision.Mar 9 2018, 10:58 AM
zzag planned changes to this revision.Mar 9 2018, 10:59 AM
zzag edited the summary of this revision. (Show Details)Mar 9 2018, 11:04 AM
ngraham added a subscriber: ngraham.Mar 9 2018, 2:10 PM

What shadow size is being used in the screenshot?

zzag added a comment.Mar 9 2018, 2:44 PM

What shadow size is being used in the screenshot?

Large. Well, currently, shadow params are shared among all sizes.

You can help by providing CSS shadows. :)

IMHO, Large and Very Large shadow sizes should not be too "large" and "very large". In addition to that, maybe, they have to be lighter than decoration shadows.

zzag added a comment.Mar 9 2018, 2:46 PM

Yet, I would like to have single shadow size for kstyle shadows.

I am confused. Where is the shadow rending code ? How is it shared between style and deco ?
In general, the sharing should be a separate patch, that could be applied even with the current shadows, before modifying how shadows are drawn ...
That would make a two steps patch: first share shadow code (with existing shadow) between kstyle and decoration, then modify shadow code.

Some more comments will follow

hpereiradacosta added inline comments.Mar 9 2018, 3:51 PM
kstyle/breezeshadowhelper.cpp
105

ok. I know this whole chunc of code is to make it look like css, but this is ugly sorry.
The fact that you need to repeat all the member names for each new set of parameters is not maintainable in the long run. Please add a proper constructor to the structure (with three params), and use this to initialize the various sets of parameters.
Idem in the kdecoration.

zzag added a comment.EditedMar 9 2018, 6:53 PM

I am confused. Where is the shadow rending code ?

BoxShadowHelper::boxShadow() does pretty much all rendering.

How is it shared between style and deco ?

Only box shadow rendering is shared. It's not obvious to me what I can share beside that. Params? Then shadow paint function should receive qpainter and the params. What about QMargins? They will be calculated twice and it also would require copying logic between kstyle, kdecoration, and libbreezecommon. The same applies to the top-left corner of the shadow inner rect and size of it. Also, kdecoration renders outline while kstyle not.

I believe I could solve this by creating a bunch of classes, but it would complicate things a lot more.

Update
IMHO, DRY obsession is good, but in this case it would make things worse.

In general, the sharing should be a separate patch, that could be applied even with the current shadows, before modifying how shadows are drawn ...
That would make a two steps patch: first share shadow code (with existing shadow) between kstyle and decoration, then modify shadow code.

Some more comments will follow

kstyle/breezeshadowhelper.cpp
105

No, I just prefer this method of initialization of structs. IMHO, it's more readable.

I'll see what I can do.

PS. I'd say it looks like js.

hpereiradacosta added inline comments.Mar 9 2018, 8:58 PM
kstyle/breezeshadowhelper.cpp
105

yeah well, but unless you sign up as the maintainer of this code, what you prefer is not the most relevant, sorry.

What I don't like with this construction is that if I change the name of a member of the class, I also have to change all the initializations.
This is against c++ encapsulation.
Ideally you would even have getters rather than direct access to members.

Please change.

In D11175#222124, @zzag wrote:

I am confused. Where is the shadow rending code ?

BoxShadowHelper::boxShadow() does pretty much all rendering.

How is it shared between style and deco ?

Only box shadow rendering is shared. It's not obvious to me what I can share beside that. Params? Then shadow paint function should receive qpainter and the params. What about QMargins? They will be calculated twice and it also would require copying logic between kstyle, kdecoration, and libbreezecommon. The same applies to the top-left corner of the shadow inner rect and size of it. Also, kdecoration renders outline while kstyle not.

But then, this code is compiled in a separate library ?
How is it linked against the kstyle code ? Doesn't that require changes to CMakeLists ? I'm confused.
(sorry if I missed something: I have not had the time to actually try apply the patches and compile them).

I believe I could solve this by creating a bunch of classes, but it would complicate things a lot more.

Update
IMHO, DRY obsession is good, but in this case it would make things worse.

So that means that the code is not actually shared, but copied ? I am fine with this also, I just don't see how it works here .
Not sure I understand what you mean by DRY obsession ?

In general, the sharing should be a separate patch, that could be applied even with the current shadows, before modifying how shadows are drawn ...
That would make a two steps patch: first share shadow code (with existing shadow) between kstyle and decoration, then modify shadow code.

Some more comments will follow

zzag added a comment.Mar 9 2018, 11:10 PM

But then, this code is compiled in a separate library ?
How is it linked against the kstyle code ? Doesn't that require changes to CMakeLists ? I'm confused.

Yes, it is compiled as a separate shared library. Yes, it requires changes to CMakeLists.txt. (e.g. add include_directories and target_link_libraries)

(sorry if I missed something: I have not had the time to actually try apply the patches and compile them).

That's good because they don't build with KDE4. (I will post updated patches soon)

So that means that the code is not actually shared, but copied ? I am fine with this also, I just don't see how it works here .

For the most part, yes..

It works as follows:

  • create a "render" target (QPixmap or QImage)
  • paint each separate shadow onto that render target with BoxShadowHelper::boxShadow(). Please note, each shadow is centered around some box so all shadows are aligned.
  • clip inner rect
  • pass the result somewhere along

Not sure I understand what you mean by DRY obsession ?

Some people try to get rid of repetitions even when it's not really needed.


Also, could you please point me out how to create shadows on plugin load? Maybe, I could implement it..

zzag updated this revision to Diff 29122.Mar 9 2018, 11:30 PM
  • fix KDE4 build
  • move cmake changes to this patch
zzag planned changes to this revision.Mar 9 2018, 11:32 PM
  • fix params issues
zzag updated this revision to Diff 29128.Mar 10 2018, 12:12 AM
  • move libbreezecommon to another patch
zzag planned changes to this revision.Mar 10 2018, 12:12 AM
zzag edited the summary of this revision. (Show Details)
zzag updated this revision to Diff 29198.Mar 11 2018, 1:33 AM
  • add shadow params
zzag planned changes to this revision.Mar 11 2018, 1:33 AM

change the way default params are represented

zzag updated this revision to Diff 29541.EditedMar 14 2018, 10:23 PM
  • don't use aggregate initialization(available only in >= C++20)
  • bump shadow strength to 100%(to match decoration shadow strength)
zzag retitled this revision from [WIP][kstyle] refine shadows to [kstyle] refine shadows.Mar 14 2018, 10:27 PM
zzag edited the summary of this revision. (Show Details)
zzag edited the summary of this revision. (Show Details)
zzag added a subscriber: abetts.

See current menu shadows in revision's summary.

zzag added a comment.Mar 14 2018, 10:29 PM

Hugo, could you please review it?

zzag added a comment.Mar 14 2018, 10:57 PM
This comment was removed by zzag.
hpereiradacosta accepted this revision.Mar 20 2018, 9:11 AM

ship it.
Thanks !

This revision is now accepted and ready to land.Mar 20 2018, 9:11 AM
zzag planned changes to this revision.Mar 22 2018, 2:38 PM

Fix MDI child window shadows.

In D11175#231610, @zzag wrote:

Fix MDI child window shadows.

I have pushed a fix to the "current" shadows already. so you would need to rebase this patch to master. (I did it locally, this will create some minor conflicts).
I also have a local patch for fixing the MDI shadows on top of this patch, if you are interested.
Note that I also commented on the possible horizontal offset that you left in your implementation, for the kdecoration part. It also applies here. I would drop it :)

zzag updated this revision to Diff 30238.Mar 22 2018, 3:31 PM
  • fix MDI child window shadows
This revision is now accepted and ready to land.Mar 22 2018, 3:31 PM
zzag requested review of this revision.Mar 22 2018, 3:34 PM

Shadows in Qt Designer

It's hard to see them because background is gray... I couldn't find another app with MDI, sorry. :(

hpereiradacosta accepted this revision.Mar 22 2018, 3:36 PM

Ship it, thanks !

This revision is now accepted and ready to land.Mar 22 2018, 3:36 PM
zzag added a comment.Mar 22 2018, 3:42 PM
In D11175#231610, @zzag wrote:

Fix MDI child window shadows.

I have pushed a fix to the "current" shadows already. so you would need to rebase this patch to master. (I did it locally, this will create some minor conflicts).
I also have a local patch for fixing the MDI shadows on top of this patch, if you are interested.

MDI shadows have another problem? Is it because of this patch?

Note that I also commented on the possible horizontal offset that you left in your implementation, for the kdecoration part. It also applies here. I would drop it :)

Yeah, I added them to keep everything consistent(the box shadow helper expects QPoint to be an offset). I'd like to keep it, if you don't mind. :)

zzag added a comment.Mar 22 2018, 3:43 PM

I'd like to land it but KWin patches haven't landed yet.

I have pushed a fix to the "current" shadows already. so you would need to rebase this patch to master. (I did it locally, this will create some minor conflicts).
I also have a local patch for fixing the MDI shadows on top of this patch, if you are interested.

MDI shadows have another problem? Is it because of this patch?

Yes (another problem) and no (because of this patch).

As you quoted on IRC, they used the raw shadowSize enum as an actual value. This was correct for the old-old shadows, but not any more when Nathan and myself switched to using an enum. Thats what required fixing, disregarding whether your patch gets landed or not.

Note that I also commented on the possible horizontal offset that you left in your implementation, for the kdecoration part. It also applies here. I would drop it :)

Yeah, I added them to keep everything consistent(the box shadow helper expects QPoint to be an offset). I'd like to keep it, if you don't mind. :)

Ok. No problem.
Still, you'll probably need to rebase your patch to the current master (because of the fix I introduced above)

zzag added a comment.EditedMar 22 2018, 3:53 PM

Still, you'll probably need to rebase your patch to the current master (because of the fix I introduced above)

I already rebased onto master(HEAD: 7f18aa39ed261828c774094c404ee7a3ed3b060e). Or I missed something?

zzag updated this revision to Diff 31810.Apr 10 2018, 12:55 PM

Rebase.

zzag updated this revision to Diff 34595.May 21 2018, 5:38 PM

Rebase

zzag retitled this revision from [kstyle] refine shadows to [kstyle] Refine shadows.May 21 2018, 5:39 PM
zzag updated this revision to Diff 35491.Jun 3 2018, 9:53 PM

Rebase.

This revision was automatically updated to reflect the committed changes.