romangg added inline comments.
Restricted Application edited projects, added KWin; removed Plasma.

INLINE COMMENTS

> blur.cpp:35
>  
> +#define BORDER_SIZE 5
> +

`static const` and in the KWin namespace please. Also camel case then.

> blur.cpp:139
> +     *
> +     * The texture blur amount is depends on the downsampling iterations and 
> the offset value.
> +     * By changing the offset we can alter the blur amount without relying 
> on further downsampling.

The texture blur amount depends

> blur.cpp:152
> +     * have good minimum and maximum blur amount, but there would be around 
> only 5 blur strength
> +     * steps.
> +     * This means we can't have constant offset value.

I think 5 blur strength steps to select from is fine, if that means we can get 
rid off the magic numbers. Other opinions?

> blur.cpp:180
> +    m_blurConfigData.append({4, 7, 170});
> +    m_blurConfigData.append({4, 8, 200});
> +}

All this data is constant, so it doesn't make sense to reinitialize it on every 
Blur instantiation and it is only a helper for reconfigure. Maybe instead as 
static const in the file?

See here how to do it for a QVector: 
https://forum.qt.io/topic/32353/solved-creating-a-const-qvector-with-values

> blur.cpp:212
> +
> +    int blurStrength = BlurConfig::blurStrength();
> +    m_downSampleIterations = 
> m_blurConfigData[blurStrength].downSampleIterations;

You need to guard against out of bounds values.

REPOSITORY
  R108 KWin

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

To: anemeth, #plasma, #kwin
Cc: romangg, zzag, anthonyfieroni, mart, davidedmundson, fredrik, ngraham, 
plasma-devel, kwin, #kwin, iodelay, bwowk, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol

Reply via email to