colorchooser: clean up xdpw request

[t-8ch]

The xdpw request creates an object on the bus. Not removing it means that the next call to method_pick_color() will fail as the old object is still around.

Also make sure to clean up in error cases.

[t-8ch]

Note: The calls to sd_bus_ do not set errno, but instead return negative error values. But xdg-desktop-portal-wlr uses errno for error messages, so the messages are wrong.

[emersion]

This looks good to me, although I wonder if we may hit a race condition here? What if the D-Bus client on the other side tries to reference the object after we've destroyed it, but before it receives PORTAL_RESPONSE_SUCCESS? Disclaimer, I am not familiar with the D-Bus object lifetime rules.

[emersion]

Note: The calls to sdbus do not set errno, but instead return negative error values. But xdg-desktop-portal-wlr uses errno for error messages, so the messages are wrong.

Ah, sounds like yet another thing worth fixing!

[t-8ch]

Looking at #283 The request may not even be needed. And indeed, it works without. It seems unclear if it is necessary, but I'm not familiar with the portal details.

But being able to only pick one color every seems wrong.

What if the D-Bus client on the other side tries to reference the object after we've destroyed it, but before it receives PORTAL_RESPONSE_SUCCESS? Disclaimer, I am not familiar with the D-Bus object lifetime rules.

No idea, I guess I'll read the specs some time.

[emersion]

Well, the problem is that the D-Bus client on the other side may call some methods on the request object, e.g. to cancel it. We're just lucky that most of the time it doesn't happen.

[t-8ch]

Why is xdg-desktop-portal-wlr even creating its own requests?

Aren't these managed by the core xdg-desktop-portal itself? At least our dbus requests already contains the object path to an existing request. We can subscribe to that requests Close().

[t-8ch]

(Calling a method on a local proxy of deleted bus object should just be a normal error case, nothing more)

[emersion]

See https://github.com/flatpak/xdg-desktop-portal/blob/main/data/org.freedesktop.impl.portal.Request.xml

Note the "impl" in the name: request objects that we create are used by xdg-desktop-portal.

[emersion]

(Also ref the GTK impl for instance: https://github.com/flatpak/xdg-desktop-portal-gtk/blob/6ca76f3a1bb9376dc0cc8b8ed3735ecc4c6a6743/src/request.c#L94)

[t-8ch]

Note the "impl" in the name

Thanks, I missed that.

[t-8ch]

Using gcolor3, multiple requests actually work. The handles in the calls are different:

/org/freedesktop/portal/desktop/request/1_193/portal815735286
/org/freedesktop/portal/desktop/request/1_193/portal1708057073

However when using kcolorchooser (which is a very thin wrapper around QColorDialog):

/org/freedesktop/portal/desktop/request/1_189/t
/org/freedesktop/portal/desktop/request/1_189/t

Which prevents multiple invocations. Also .Close() is not invoked by either one.

[t-8ch]

If I read the KDE implementation correctly, they also remove the dbus object when the dialog is closed. So similar to my cleanup here. (Which also needs to be extended for all other request usages)