| davidedmundson | |
| broulik |
| Plasma on Wayland | |
| KWin |
The effect exports itself to DBus as object "/ColorPicker" and provides
an own interface "org.kde.kwin.ColorPicker".
It has one exported method to DBus "pick" which returns a QColor. When
invoked an interactive position picking selection is started. If it ends
the effect reads the color value at the picked position from the OpenGL
color buffer.
This implements T4568.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| effects/colorpicker/colorpicker.cpp | ||
|---|---|---|
| 52 | So this won't work in case of QPainter? Maybe the color picker should communicate that to the user when it's not possible to pick a color? | |
| effects/colorpicker/colorpicker.h | ||
| 38 | David complained about using different interfaces in another review (I'm fine with that though) | |
| effects/colorpicker/colorpicker.cpp | ||
|---|---|---|
| 52 | The supported means KWin won't load the effect at all. How to communicate that to the user is then up to the UI which tries to invoke the Colorpicker. You will get a DBus error. Also in future I don't see a reason to not also support QPainter | |
| effects/colorpicker/colorpicker.h | ||
| 38 | No, he complained about different services. Different interface was fine. | |
looks good to me too.
| effects/colorpicker/colorpicker.cpp | ||
|---|---|---|
| 57 | KWin has a lovely class ClearablePoint for doing exactly this | |
| 65 | FWIW, this line isn't needed | |
| 85 | FWIW (though not worth changing it) if you're going to have n=1 you can use glReadPixels | |
| 102 | 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) | |
| 111 | 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. | |
| effects/colorpicker/colorpicker.h | ||
| 38 | 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. | |
| effects/colorpicker/colorpicker.cpp | ||
|---|---|---|
| 57 | only in utils.h, we cannot use that in the effects :-( | |
| 85 | 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>" | |
| 102 | good point. I copied that from the documentation. | |
| 111 | thanks! I didn't know that and thought one needs to use the QDBusError enum. | |