[Differential] [Updated] D3480: [effects] Add a colorpicker effect

[graesslin (Martin Gräßlin)]

graesslin marked 2 inline comments as done.
graesslin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in colorpicker.cpp:56
> KWin has a lovely class ClearablePoint for doing exactly this

only in utils.h, we cannot use that in the effects :-(

> davidedmundson wrote in colorpicker.cpp:84
> FWIW (though not worth changing it) if you're going to have n=1 you can use glReadPixels

The idea behind glReadnPixels is to have a variant which cannot overflow the passed in buffer. Thus always use glReadnPixels, never use glReadPixels. "ReadnPixelsARB behaves identically to ReadPixels except that it does not write more than <bufSize> bytes into <data>"

> davidedmundson wrote in colorpicker.cpp:101
> there's not a lot of point doing this, you know what connection it is as you only register this on the session bus (line 59)

good point. I copied that from the documentation.

> davidedmundson wrote in colorpicker.cpp:110
> DBus error messages have two parts:
> 
> a computer readable name, and a human readable stirng.
> 
> You want to supply a better name than "org.freedesktop.DBus.Error.Failed" (which is what this expands to) so that spectacle can tell the difference between failed and cancelled without parsing strings meant for humans.

thanks! I didn't know that and thought one needs to use the QDBusError enum.

REPOSITORY
  rKWIN KWin

BRANCH
  color-picker-effect

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #kwin, #plasma_on_wayland, broulik
Cc: davidedmundson, plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161125/d0456347/attachment.html>