libportal icon indicating copy to clipboard operation
libportal copied to clipboard

Add support for the InputCapture portal

Open whot opened this issue 3 years ago • 2 comments

For the corresponding Portal PR, please see https://github.com/flatpak/xdg-desktop-portal/pull/714

This is a draft PR to add support for the input capture portal. Most of it is straightforward portal implementation but this portal does have a larger surface and more state than most, which raises a few questions:

  • [ ] ~~XdpSession is re-used for input capture portals now too. This makes namespacing (or the lack thereof) slightly awkward, e.g. the activated signal on the session is instead inputcapture-activated to avoid future conflicts, which then requires the other signals to be namespaced too.~~
    • ~~ideally we'd have a XdpInputCaptureSession as subclass of XdpSession but the latter is declared final right now and I'm not 100% if that's an ABI break then.~~ edit: this was nacked in #102
  • [ ] XdpInputCaptureSession now contains an XdpSession and the API is namespaced properly.
  • [x] The enable/disable/release methods ~~are paired with their respective _finish, but tbh this is of questionable value. Even if the portal keeps these as requests, I'd argue hiding this away here would make for better API - we can use the session signals to notify the caller out-of-band.~~ Update June 06: these are method calls now
  • [ ] the pointer barriers and capture zones have their own GObjects with properties and little other functionality. This makes for slightly more sensible API but it also conflicts a bit with e.g. XdpSession which doesn't have properties but setters/getters only.
  • [x] the Disabled signal is still missing here I just noticed
  • [x] there's a dbusmock/pytest based test suite included. ~~that won't run without patched dbus-python and dbusmock~~ but it makes testing so much easier. specifically, it allows to test libportal without an existing implementation of the actual portal. Merged to main in #99, this is just the inputcapture tests now

The above are the larger topics, I'd appreciate a high-level review of the API and suggestions for where to go with this.

cc @jadahl

whot avatar Jun 02 '22 07:06 whot

if anyone is following along: the most recent changes from today change the implementation so that we have an API that creates and takes XdpInputCaptureSession objects. Inside that object is an XdpSession object that represents the session, it's exposed to the user on request via xdp_input_capture_session_get_session() so the user can e.g. subscribe to closed.

This is basically a working implementation of #112 (while not touching the RD/SC API) and should eventually allow us to create an InputCapture session from an existing XdpSession object, see https://github.com/flatpak/xdg-desktop-portal/pull/864.

whot avatar Aug 30 '22 05:08 whot

Just FTR, this now sits on top of #114 because I need both to work at the same time and juggling branches is annoying.

whot avatar Sep 07 '22 01:09 whot

marking as ready, I stared at this for too long, I can no longer see any bugs...

whot avatar Aug 07 '23 18:08 whot

What's keeping this from landing at this point? It seems to have been sitting in a ready state for a month now...

Conan-Kudo avatar Sep 14 '23 16:09 Conan-Kudo

I had this on my list to look at today. Thanks for beating me to it, @ebassi

matthiasclasen avatar Sep 19 '23 15:09 matthiasclasen

Many thanks for the review!

I have applied the changes and forced push to @whot's branch now.

Please also note that I had to drop a few suggested changes at it was breaking the build on Ubuntu 20.04 for the CI (I suspect the version of Glib is older on that target).

ofourdan avatar Oct 04 '23 08:10 ofourdan

ftr, I fixed a buggy test case, but this branch should still be ready to go.

whot avatar Oct 09 '23 22:10 whot

gentle ping for merging :)

whot avatar Oct 17 '23 08:10 whot