[Differential] [Commented On] D3480: [effects] Add a colorpicker effect

[davidedmundson (David Edmundson)]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161124/ec8be73a/attachment-0001.html>