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
