D11198: [libbreezecommon] Add box shadow helper

rikmills added a comment. In D11198#292783 , @zzag wrote: > @rikmills Should build now. @zzag It does. Thank you REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc:

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. @rikmills Should build now. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: rikmills, ngraham, broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg,

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. Gosh, I forgot that `qreal` can be also a float. On it. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: rikmills, ngraham, broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot,

D11198: [libbreezecommon] Add box shadow helper

rikmills added a comment. Fails to build from source: https://build.neon.kde.org/job/bionic_unstable_kde_breeze_bin_armhf/44/consoleFull 06:47:17 CMakeFiles/Makefile2:118: recipe for target 'libbreezecommon/CMakeFiles/breezecommon4.dir/all' failed 06:47:17

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. If there are serious problems, we still can revert it. Also, there is a chance that we need to notify sysadmins about new dependency. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: ngraham,

D11198: [libbreezecommon] Add box shadow helper

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit R31:bffe8faa8706: [libbreezecommon] Add box shadow helper (authored by zzag). REPOSITORY R31 Breeze CHANGES SINCE LAST

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. Landing it... REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: ngraham, broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

ngraham accepted this revision as: VDG. ngraham added a comment. +1 visually. Sadly @hpereiradacosta stepped down as Breeze maintainer recently, but since he's already given his stamp of approval, I think this can go in. REPOSITORY R31 Breeze REVISION DETAIL

D11198: [libbreezecommon] Add box shadow helper

abetts added a comment. I thought this was accepted and ready to go. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: ngraham, broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed,

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. #Plasma #VDG folks, what's the status of the shadow patches? REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc:

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. In D11198#277351 , @hpereiradacosta wrote: > To be honest, I can't really see strong differences between the two. But then I have no objection either against keep the dependence on fftw. OK, then I would like to

D11198: [libbreezecommon] Add box shadow helper

hpereiradacosta added a comment. In D11198#277321 , @zzag wrote: > In D11198#238841 , @zzag wrote: > > > F5783057: box-vs-fft.png > > //on the left hand

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. In D11198#238841 , @zzag wrote: > F5783057: box-vs-fft.png > //on the left hand side: box blur, on the right hand size: fft blur// Also, if you think box blur looks okay, I

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. Ping. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: ngraham, broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

ngraham added a comment. Oh OK never mind then, ignore me... REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: ngraham, broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg,

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. In D11198#272069 , @ngraham wrote: > Instead of using pure black, how about instead using a slightly lighter dark gray color from the standard breeze color scheme, like Shade Black (35,38,39)? > >

D11198: [libbreezecommon] Add box shadow helper

ngraham added a comment. Instead of using pure black, how about instead using a slightly lighter dark gray color from the standard breeze color scheme, like Shade Black (35,38,39)? https://community.kde.org/KDE_Visual_Design_Group/HIG/Color I think that might alleviate some of my

D11198: [libbreezecommon] Add box shadow helper

zzag added a comment. Ping. REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

zzag updated this revision to Diff 34593. zzag added a comment. Rebase REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11198?vs=32789=34593 REVISION DETAIL https://phabricator.kde.org/D11198 AFFECTED FILES CMakeLists.txt cmake/Modules/FindFFTW.cmake

D11198: [libbreezecommon] add box shadow helper

zzag updated this revision to Diff 32789. zzag added a comment. Fix invalid read of size 1 Valgrind output: ==8054== Invalid read of size 1 ==8054==at 0x1D8818C6: Breeze::BoxShadowHelper::blurAlphaNaivePass(QImage const&, QImage&, QVector const&) (in

D11198: [libbreezecommon] add box shadow helper

zzag updated this revision to Diff 31846. zzag added a comment. Because this patch hasn't been landed yet, I would like to post here my recent work on optimizing the box shadow helper... Summary of changes: - re-write naive blur helper - modify only alpha channel in blur helpers

D11198: [libbreezecommon] add box shadow helper

zzag updated this revision to Diff 31808. zzag added a comment. Rebase. REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11198?vs=29989=31808 BRANCH refine-shadows-libbreezecommon REVISION DETAIL https://phabricator.kde.org/D11198 AFFECTED FILES

D11198: [libbreezecommon] add box shadow helper

zzag added a comment. If someone(in the future, I guess) will decide to optimize the box shadow helper or get rid of fftw, here's a patch to use box blur : https://phabricator.kde.org/P187 Even though box blur may be faster(I haven't benchmarked it), I still would prefer the "true"

D11198: [libbreezecommon] add box shadow helper

zzag added a comment. In D11198#229897 , @hpereiradacosta wrote: > I deliberately accepted the revision, despite having still some comments about what should be implemented, because I trusted you that you would implement this and only this.

D11198: [libbreezecommon] add box shadow helper

hpereiradacosta added a comment. > I have updated the diff so the "Accepted" should have gone away because I could introduce a bug, etc. Most likely it's a bug, I don't know ... Or Phabricator has AI so it could recognize what changes you wanted and what changes the new diff

D11198: [libbreezecommon] add box shadow helper

zzag added a comment. In D11198#229871 , @hpereiradacosta wrote: > In D11198#229870 , @zzag wrote: > > > @hpereiradacosta could you please accept this diff again? (I updated the diff) > > >

D11198: [libbreezecommon] add box shadow helper

hpereiradacosta added a comment. In D11198#229870 , @zzag wrote: > @hpereiradacosta could you please accept this diff again? (I updated the diff) Must I ? Here the revision is marked accepted and ready to be shipped. Or is it a problem

D11198: [libbreezecommon] add box shadow helper

zzag added a comment. @hpereiradacosta could you please accept this diff again? (I updated the diff) REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed,

D11198: [libbreezecommon] add box shadow helper

zzag updated this revision to Diff 29989. zzag added a comment. prepend `std::` to `erf` REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11198?vs=29858=29989 REVISION DETAIL https://phabricator.kde.org/D11198 AFFECTED FILES CMakeLists.txt

D11198: [libbreezecommon] add box shadow helper

hpereiradacosta accepted this revision. hpereiradacosta added a comment. This revision is now accepted and ready to land. Fix it (std::erf), then ship it ! I have had no time to work on an alternative QGradient blur, and wont in the near future. In the meanwhile lets use this code (that

D11198: [libbreezecommon] add box shadow helper

zzag updated this revision to Diff 29858. zzag added a comment. don't forward declare Qt stuff REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11198?vs=29843=29858 REVISION DETAIL https://phabricator.kde.org/D11198 AFFECTED FILES CMakeLists.txt

D11198: [libbreezecommon] add box shadow helper

hpereiradacosta added inline comments. INLINE COMMENTS > breezeboxshadowhelper.h:30 > +class QRect; > + > + I think the "rule" is to not use forward declarations for classes external to once project. Reason is that the upstream library might decide to turn a class into a struct, or an alias

D11198: [libbreezecommon] add box shadow helper

zzag updated this revision to Diff 29843. zzag edited the summary of this revision. zzag added a comment. - add comments REPOSITORY R31 Breeze CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11198?vs=29535=29843 REVISION DETAIL https://phabricator.kde.org/D11198 AFFECTED FILES

D11198: [libbreezecommon] add box shadow helper

zzag added inline comments. INLINE COMMENTS > broulik wrote in config-breezecommon.h.cmake:1 > This file is generated and should not be checked in What do you mean? We need to pass an input file to the `configure_file`. REPOSITORY R31 Breeze REVISION DETAIL

D11198: [libbreezecommon] add box shadow helper

broulik added inline comments. INLINE COMMENTS > config-breezecommon.h.cmake:1 > +/* config-breezecommon.h. Generated by cmake from > config-breezecommon.h.cmake */ > + This file is generated and should not be checked in REPOSITORY R31 Breeze REVISION DETAIL

D11198: [libbreezecommon] add box shadow helper

zzag added a comment. Hugo, could you please review this patch? REPOSITORY R31 Breeze REVISION DETAIL https://phabricator.kde.org/D11198 To: zzag, #breeze, #vdg, hpereiradacosta Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart