Details
- Reviewers
hpereiradacosta - Group Reviewers
Breeze VDG - Commits
- R31:5a15e8ed40e1: [kstyle] Refine shadows
Diff Detail
- Repository
- R31 Breeze
- Branch
- shadows
- Lint
No Linters Available - Unit
No Unit Test Coverage
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.
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
kstyle/breezeshadowhelper.cpp | ||
---|---|---|
144 | ok. I know this whole chunc of code is to make it look like css, but this is ugly sorry. |
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 | ||
---|---|---|
144 | 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. |
kstyle/breezeshadowhelper.cpp | ||
---|---|---|
144 | 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. Please change. |
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
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..
- don't use aggregate initialization(available only in >= C++20)
- bump shadow strength to 100%(to match decoration shadow strength)
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 :)
Shadows in Qt Designer
It's hard to see them because background is gray... I couldn't find another app with MDI, sorry. :(
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. :)
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)
I already rebased onto master(HEAD: 7f18aa39ed261828c774094c404ee7a3ed3b060e). Or I missed something?