D11198: [libbreezecommon] Add box shadow helper

[Rik Mills]

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: rikmills, ngraham, broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

[Vlad Zagorodniy]

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, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

[Vlad Zagorodniy]

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, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

[Rik Mills]

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 /workspace/build/libbreezecommon/breezeboxshadowhelper.cpp: In 
function ‘void Breeze::BoxShadowHelper::blurAlphaNaive(QImage&, int)’:
  06:47:17 /workspace/build/libbreezecommon/breezeboxshadowhelper.cpp:138:56: 
error: conversion from ‘QVector’ to non-scalar type ‘const 
QVector’ requested
  06:47:17  const QVector kernel = computeGaussianKernel(radius);
  06:47:17~^~~~
  06:47:17 In file included from /usr/include/qt4/QtGui/qpolygon.h:45:0,
  06:47:17  from /usr/include/qt4/QtGui/qmatrix.h:45,
  06:47:17  from /usr/include/qt4/QtGui/qtransform.h:44,
  06:47:17  from /usr/include/qt4/QtGui/qimage.h:45,
  06:47:17  from /usr/include/qt4/QtGui/qpixmap.h:50,
  06:47:17  from /usr/include/qt4/QtGui/qpainter.h:49,
  06:47:17  from /usr/include/qt4/QtGui/QPainter:1,
  06:47:17  from 
/workspace/build/libbreezecommon/breezeboxshadowhelper.h:27,
  06:47:17  from 
/workspace/build/libbreezecommon/breezeboxshadowhelper.cpp:21:
  06:47:17 /usr/include/qt4/QtCore/qvector.h: In instantiation of ‘int 
QVector::sizeOfTypedData() [with T = double]’:
  06:47:17 /usr/include/qt4/QtCore/qvector.h:503:49:   required from ‘void 
QVector::realloc(int, int) [with T = double]’
  06:47:17 /usr/include/qt4/QtCore/qvector.h:340:32:   required from ‘void 
QVector::reserve(int) [with T = double]’
  06:47:17 /workspace/build/libbreezecommon/breezeboxshadowhelper.cpp:62:30:   
required from here
  06:47:17 /usr/include/qt4/QtCore/qvector.h:323:49: warning: cast from 
‘QVector*’ to ‘const Data* {aka const QVectorTypedData*}’ 
increases required alignment of target type [-Wcast-align]
  06:47:17  return reinterpret_cast(&(reinterpret_cast(this))->array[1]) - reinterpret_cast(this);
  06:47:17 
~^
  06:47:17 make[5]: 
[libbreezecommon/CMakeFiles/breezecommon4.dir/breezeboxshadowhelper.cpp.o] 
Error 1
  06:47:17 make[4]:  [libbreezecommon/CMakeFiles/breezecommon4.dir/all] Error 2

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, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

[Vlad Zagorodniy]

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, broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

[Vlad Zagorodniy]

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 UPDATE
  https://phabricator.kde.org/D11198?vs=34593&id=37266

REVISION DETAIL
  https://phabricator.kde.org/D11198

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindFFTW.cmake
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt
  libbreezecommon/breezeboxshadowhelper.cpp
  libbreezecommon/breezeboxshadowhelper.h
  libbreezecommon/config-breezecommon.h.cmake

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

[Vlad Zagorodniy]

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

[Nathaniel Graham]

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
  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

[Andres Betts]

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, jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

[Vlad Zagorodniy]

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: ngraham, broulik, abetts, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

[Vlad Zagorodniy]

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 leave it as is.

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

[Hugo Pereira Da Costa]

hpereiradacosta added a comment.


  In D11198#277321 , @zzag wrote:
  
  > 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 will switch box shadow helper to 
box blur. (that way, we don't need fftw)
  
  
  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.

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

[Vlad Zagorodniy]

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 will switch box shadow helper to 
box blur. (that way, we don't need fftw)

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

[Vlad Zagorodniy]

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

[Nathaniel Graham]

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, sebas, apol, mart

D11198: [libbreezecommon] Add box shadow helper

[Vlad Zagorodniy]

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)?
  >
  > https://community.kde.org/KDE_Visual_Design_Group/HIG/Color
  >
  > I think that might alleviate some of my concern regarding the harsh 
blackness at the bottom of the windows with this patch chain. Either that, or 
maybe just increase the spread closer to the center of the box? Or both?
  
  
  I didn't change shadow color. It's still rgb(35, 38, 39), IIRC. :)

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

[Nathaniel Graham]

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 concern regarding the harsh blackness 
at the bottom of the windows with this patch chain. Either that, or maybe just 
increase the spread closer to the center of the box? Or both?

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

[Vlad Zagorodniy]

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

[Vlad Zagorodniy]

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&id=34593

REVISION DETAIL
  https://phabricator.kde.org/D11198

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindFFTW.cmake
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt
  libbreezecommon/breezeboxshadowhelper.cpp
  libbreezecommon/breezeboxshadowhelper.h
  libbreezecommon/config-breezecommon.h.cmake

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

[Vlad Zagorodniy]

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 /home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==by 0x1D8819F3: Breeze::BoxShadowHelper::blurAlphaNaive(QImage&, 
int) (in /home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==by 0x1D8822DA: Breeze::BoxShadowHelper::boxShadow(QPainter*, 
QRect const&, QPoint const&, int, QColor const&) (in 
/home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==by 0x1D60C8CC: Breeze::ShadowHelper::shadowTiles() (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x1D60C341: Breeze::ShadowHelper::loadConfig() (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x1D618B51: Breeze::Style::loadConfiguration() (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x1D613D3E: Breeze::Style::Style() (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x1D63C52F: Breeze::StylePlugin::create(QString const&) (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x7B18B62: QStyleFactory::create(QString const&) (in 
/usr/lib/libQt5Widgets.so.5.10.1)
==8054==by 0x7AABA9B: QApplication::style() (in 
/usr/lib/libQt5Widgets.so.5.10.1)
==8054==by 0x7AABDF5: QApplicationPrivate::initialize() (in 
/usr/lib/libQt5Widgets.so.5.10.1)
==8054==by 0x7AABE5A: QApplicationPrivate::init() (in 
/usr/lib/libQt5Widgets.so.5.10.1)
==8054==  Address 0x16b332f7 is 3 bytes after a block of size 19,044 alloc'd
==8054==at 0x4C2CEDF: malloc (vg_replace_malloc.c:299)
==8054==by 0x82FDEDA: QImageData::create(QSize const&, QImage::Format) 
(in /usr/lib/libQt5Gui.so.5.10.1)
==8054==by 0x82FE06C: QImage::QImage(QSize const&, QImage::Format) (in 
/usr/lib/libQt5Gui.so.5.10.1)
==8054==by 0x82FE0A5: QImage::QImage(int, int, QImage::Format) (in 
/usr/lib/libQt5Gui.so.5.10.1)
==8054==by 0x1D8819C5: Breeze::BoxShadowHelper::blurAlphaNaive(QImage&, 
int) (in /home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==by 0x1D8822DA: Breeze::BoxShadowHelper::boxShadow(QPainter*, 
QRect const&, QPoint const&, int, QColor const&) (in 
/home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==by 0x1D60C8CC: Breeze::ShadowHelper::shadowTiles() (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x1D60C341: Breeze::ShadowHelper::loadConfig() (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x1D618B51: Breeze::Style::loadConfiguration() (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x1D613D3E: Breeze::Style::Style() (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x1D63C52F: Breeze::StylePlugin::create(QString const&) (in 
/home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==by 0x7B18B62: QStyleFactory::create(QString const&) (in 
/usr/lib/libQt5Widgets.so.5.10.1)
  
  The reason: I forgot that the kernel is of size `2 * radius + 1` so when 
blurAlphaNaivePass 
  convolving near ends it doesn't take 1 into account. Overall, fix looks like 
this
  
diff --git a/libbreezecommon/breezeboxshadowhelper.cpp 
b/libbreezecommon/breezeboxshadowhelper.cpp
index 625cb26a..17d18ecd 100644
--- a/libbreezecommon/breezeboxshadowhelper.cpp
+++ b/libbreezecommon/breezeboxshadowhelper.cpp
@@ -118,7 +118,7 @@ void blurAlphaNaivePass(const QImage &src, QImage &dst, 
const QVector &ke
 }

 for (int x = src.width() - radius; x < src.width(); x++) {
-const uchar *window = in + (x - radius) * alphaStride;
+const uchar *window = in + (x - radius - 1) * alphaStride;
 qreal alpha = 0;
 const int outside = x + radius - src.width();
 for (int k = 0; k < kernel.size() - outside; k++) {

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11198?vs=31846&id=32789

BRANCH
  arcpatch-D11198

REVISION DETAIL
  https://phabricator.kde.org/D11198

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindFFTW.cmake
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt
  libbreezecommon/breezeboxshadowhelper.cpp
  libbreezecommon/breezeboxshadowhelper.h
  libbreezecommon/config-breezecommon.h.cmake

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

[Vlad Zagorodniy]

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
  - reserve memory for kernel in computeGaussianKernel
  
  See, benchmark results 
https://docs.google.com/spreadsheets/d/1ykAUyspF6BgCFb_U7-muLDEYzrOTNiT5CvFgqCU2xTs/edit?usp=sharing

REPOSITORY
  R31 Breeze

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11198?vs=31808&id=31846

BRANCH
  refine-shadows-libbreezecommon

REVISION DETAIL
  https://phabricator.kde.org/D11198

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindFFTW.cmake
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt
  libbreezecommon/breezeboxshadowhelper.cpp
  libbreezecommon/breezeboxshadowhelper.h
  libbreezecommon/config-breezecommon.h.cmake

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

[Vlad Zagorodniy]

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&id=31808

BRANCH
  refine-shadows-libbreezecommon

REVISION DETAIL
  https://phabricator.kde.org/D11198

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindFFTW.cmake
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt
  libbreezecommon/breezeboxshadowhelper.cpp
  libbreezecommon/breezeboxshadowhelper.h
  libbreezecommon/config-breezecommon.h.cmake

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

[Vlad Zagorodniy]

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" Gaussian because it looks better to me.
  
  F5783057: box-vs-fft.png 
  
  //on the left hand side: box blur, on the right hand size: fft blur//

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

[Vlad Zagorodniy]

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. 
  >  Basically when a revision is accepted, disregarding how many new diffs are 
uploaded to it, untill someone changes the status back to "request changes".
  
  
  Thanks for explanation. The more I live, the more I learn. :)

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D11198

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Hugo Pereira Da Costa]

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 introduced.
  
  No no that's not how it works. 
  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. 
  Basically when a revision is accepted, disregarding how many new diffs are 
uploaded to it, untill someone changes the status back to "request changes". 
  So ... Fill free to push

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D11198

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Vlad Zagorodniy]

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)
  >
  >
  > Must I ? Here the revision is marked accepted and ready to be shipped. Or 
is it a problem with ARC ?
  
  
  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 introduced.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D11198

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Hugo Pereira Da Costa]

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 with ARC ?

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D11198

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Vlad Zagorodniy]

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, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Vlad Zagorodniy]

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&id=29989

REVISION DETAIL
  https://phabricator.kde.org/D11198

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindFFTW.cmake
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt
  libbreezecommon/breezeboxshadowhelper.cpp
  libbreezecommon/breezeboxshadowhelper.h
  libbreezecommon/config-breezecommon.h.cmake

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Hugo Pereira Da Costa]

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 you have spent quite 
some time on already). 
  There is still time to revisit it in the future. 
  Thanks !

INLINE COMMENTS

> breezeboxshadowhelper.cpp:41
> +double kernelNorm = 0.0;
> +double lastInt = 0.5 * erf((-radius - 0.5) / den);
> +

erf -> std::erf (since c++11). 
same elsewhere. I think it is clearer to add the namespace explicitly, now that 
it is properly defined.

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D11198

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Vlad Zagorodniy]

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&id=29858

REVISION DETAIL
  https://phabricator.kde.org/D11198

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindFFTW.cmake
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt
  libbreezecommon/breezeboxshadowhelper.cpp
  libbreezecommon/breezeboxshadowhelper.h
  libbreezecommon/config-breezecommon.h.cmake

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Hugo Pereira Da Costa]

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 into future changes, which will then break your code.
Please include the headers here directly, and remove them from the cpp

REPOSITORY
  R31 Breeze

REVISION DETAIL
  https://phabricator.kde.org/D11198

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Vlad Zagorodniy]

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&id=29843

REVISION DETAIL
  https://phabricator.kde.org/D11198

AFFECTED FILES
  CMakeLists.txt
  cmake/Modules/FindFFTW.cmake
  kstyle/CMakeLists.txt
  libbreezecommon/CMakeLists.txt
  libbreezecommon/breezeboxshadowhelper.cpp
  libbreezecommon/breezeboxshadowhelper.h
  libbreezecommon/config-breezecommon.h.cmake

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Vlad Zagorodniy]

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
  https://phabricator.kde.org/D11198

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Kai Uwe Broulik]

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
  https://phabricator.kde.org/D11198

To: zzag, #breeze, #vdg, hpereiradacosta
Cc: broulik, abetts, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, sebas, apol, mart

D11198: [libbreezecommon] add box shadow helper

[Vlad Zagorodniy]

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