bilelmoussaoui / ashpd

A Rust wrapper around XDG portals DBus interfaces
https://bilelmoussaoui.github.io/ashpd/ashpd/
MIT License
234 stars 45 forks source link

fix barrier_id=0 in input-capture example #215

Open feschber opened 3 months ago

feschber commented 3 months ago

According to the documentation, barrier-ids must be non-zero.

(https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.InputCapture.html#org-freedesktop-portal-inputcapture-setpointerbarriers)

feschber commented 3 months ago

Apparently, I missed this when writing the example.

feschber commented 3 months ago

Apparently, I missed this when writing the example.

And GNOME does not seem to verify this, but the plasma portal does. So that is probably, why I missed it.

bilelmoussaoui commented 3 months ago

Can we make the BarriedId type an alias of NonZeroU32 as well to avoid such issues ?

feschber commented 3 months ago

There is this:

barrier_id (u)

The barrier id of the barrier that triggered. If the value is nonzero, it matches the barrier id as specified in [org.freedesktop.portal.InputCapture.SetPointerBarriers](https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.InputCapture.html#org-freedesktop-portal-inputcapture-setpointerbarriers).

If the id is zero, the pointer barrier could not be determined. If the id is missing, the input capture was not triggered by a pointer barrier.

Where more than one pointer barrier are triggered by the same movement it is up to the compositor to choose one barrier (or use a barrier id of zero).

in the activated signal. Idk if it would be possible to convert this to an Option? But just using NonZeroU32 would break here.

bilelmoussaoui commented 3 months ago

There is this:

barrier_id (u)

The barrier id of the barrier that triggered. If the value is nonzero, it matches the barrier id as specified in [org.freedesktop.portal.InputCapture.SetPointerBarriers](https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.InputCapture.html#org-freedesktop-portal-inputcapture-setpointerbarriers).

If the id is zero, the pointer barrier could not be determined. If the id is missing, the input capture was not triggered by a pointer barrier.

Where more than one pointer barrier are triggered by the same movement it is up to the compositor to choose one barrier (or use a barrier id of zero).

in the activated signal. Idk if it would be possible to convert this to an Option? But just using NonZeroU32 would break here.

I see, I think Option<BarriedId> would make more sense in this case :)

feschber commented 3 months ago

I'm also just realizing, that this is not 100% correct in it's current state. It should really be an Option<Option<BarrierId>>

According to the documentation, zero means "the barrier could not be determined" and for the future, when eventually a capture can be started manually without barriers, there is no barrier id at all.

Currently, by the looks of things we always assume that a barrier id is passed. So really this should be Option<Option<NonZeroBarrierId>>, eventhough I'm not quite sure what "barrier could not be determined" is supposed to mean...

bilelmoussaoui commented 3 months ago

cc @whot, would you mind clarifying this bit

If the id is zero, the pointer barrier could not be determined

whot commented 3 months ago

I'm not quite sure what "barrier could not be determined" is supposed to mean...

it's basically a cop-out behavior, sorry :) But it basically means "yes, you have pointer barriers configured somewhere, yes, having those caused the input capture, no, it's not clear which barrier triggered it". It distinguishes between InputCapture being activated through other means (which admittedly don't yet exist).

Possible use-case may be a partial barrier on a screen edge where the compositor decides to activate IC on the whole screen edge anyway. But yeah, it's mostly cop-out and tbh I forgot this was in there :( So unfortunately Option<Option<NonZeroBarrierId>> it is.

feschber commented 3 months ago

I'm not quite sure what "barrier could not be determined" is supposed to mean...

it's basically a cop-out behavior, sorry :) But it basically means "yes, you have pointer barriers configured somewhere, yes, having those caused the input capture, no, it's not clear which barrier triggered it". It distinguishes between InputCapture being activated through other means (which admittedly don't yet exist).

Possible use-case may be a partial barrier on a screen edge where the compositor decides to activate IC on the whole screen edge anyway. But yeah, it's mostly cop-out and tbh I forgot this was in there :( So unfortunately Option<Option<NonZeroBarrierId>> it is.

I still find it a bit weird from a barrier usecase perspective. What client would be associated with this activation token if it comes from "no barrier"? The only reasonable thing to do here would be to just immediately release the capture again. Is there any usecase that would make this "useful" to have?

But this is more of a discussion for xdg-desktop-portal in general I guess.

whot commented 3 months ago

the portal interaction is always with one client anyway, so "what client" is not really a question, or am I misunderstanding something here?

A slightly contrived use-case: client has an input capture, gets disabled (e.g. a local password prompt). Once that's done and the client calls Enable it immediately gets the capture again despite this not being triggered by a barrier, merely because the client has barriers and we want to provide this continuity.

feschber commented 3 months ago

I am referring to barrier-clients in a barrier use case. You would need some sort of Map<BarrierId, Client> that maps the barrier that was activated to the client that is associated with that barrier. So if no barrier is reported by the activation token, you have to arbitrarily assign a client to the activation...

feschber commented 3 months ago

The case you describe makes sense but it would also be possible to just report the previous barrier_id imo.

whot commented 2 months ago

I feel like I'm being dense: the barrier id is just a number, there's no guarantee they're unique (except within the client). So you already cannot rely on the barrier id identifying a client anyway. Signals are (supposed to be) only sent to the session so the session handle is the thing that identifies the client. What am I getting wrong here?

feschber commented 2 months ago

I feel like I'm being dense: the barrier id is just a number, there's no guarantee they're unique (except within the client). So you already cannot rely on the barrier id identifying a client anyway. Signals are (supposed to be) only sent to the session so the session handle is the thing that identifies the client. What am I getting wrong here?

If you configure two barriers left and right, then you can give barrier-id 1 to left and 2 to right in SetPointerBarriers. So in order to find out which barrier triggered the capture, you can check the barrier_id from the Activated signal.

"barrier_id The non-zero id of this barrier. This id is used in the org.freedesktop.portal.InputCapture::Activated signal to inform which barrier triggered input capture."

So if there is no barrier_id its impossible to tell if left or right was triggered. That's all I'm trying to say here.

whot commented 2 months ago

yes, but that's the "feature" of the whole missing/zero/number: in the future we will have other triggers than pointer barriers (e.g. keyboard shortcuts) and that's where the barrier_id will be missing altogether so that had to be worked into version 1 of the portal.

zero is for the case of activated because you have barriers, not because of any specific barrier. Which would imply things like pointer motion is not necessary (depends on the use-case) and it may serve as distinction for a future client that has barriers and keyboard shortcuts. We don't have a use-case for it and I'd probably not add it if I were to re-implement this today but the interface is what it is now, removing that could be considered an API break.

bilelmoussaoui commented 2 months ago

so what is the tldr here?

feschber commented 2 months ago

so what is the tldr here?

Sorry for the late reply

TLDR is: We want Option<Option<NonZeroU32>> to comply with the spec, as long as zbus can correctly deserialize that from an Option<u32>

I can check later whether or not this is possible.

A6GibKm commented 2 months ago

I would suggest to do a proper enum and implement serialize/deserialize, from a user perspective Option<Option<T>> does not give any info and most people will just flat it into a Option<T>.

feschber commented 2 months ago

I would suggest to do a proper enum and implement serialize/deserialize, from a user perspective Option<Option<T>> does not give any info and most people will just flat it into a Option<T>.

yeah that probably makes most sense.