davidedmundson added a comment.
looks good to me too.
INLINE COMMENTS
> colorpicker.cpp:56
> + : m_replyConnection(QDBusConnection::sessionBus())
> + , m_scheduledPosition(QPoint(-1, -1))
> +{
KWin has a lovely class ClearablePoint for doing exactly this
> colorpicker.cpp:64
> +{
> +
> QDBusConnection::sessionBus().unregisterObject(QStringLiteral("/ColorPicker"));
> +}
FWIW, this line isn't needed
> colorpicker.cpp:84
> + const QRect geo = GLRenderTarget::virtualScreenGeometry();
> + glReadnPixels(m_scheduledPosition.x() - geo.x(), geo.height() -
> geo.y() - m_scheduledPosition.y(), 1, 1, GL_RGB, GL_UNSIGNED_BYTE, 3, data);
> + m_replyConnection.send(m_replyMessage.createReply(QColor(data[0],
> data[1], data[2])));
FWIW (though not worth changing it) if you're going to have n=1 you can use
glReadPixels
> colorpicker.cpp:101
> + m_picking = true;
> + m_replyConnection = connection();
> + m_replyMessage = message();
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)
> colorpicker.cpp:110
> + // error condition
> +
> m_replyConnection.send(m_replyMessage.createErrorReply(QDBusError::Failed,
> "Color picking got cancelled"));
> + m_picking = false;
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.
> graesslin wrote in colorpicker.h:37
> No, he complained about different services. Different interface was fine.
Just to chime in, mgraesslin is right.
A different interface is very sensible as apparmor and selinux (and in turn
snappy) can filter on interfaces.
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
