Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix barrier_id=0 in input-capture example #215

Merged

Conversation

feschber
Copy link
Contributor

@feschber
Copy link
Contributor Author

Apparently, I missed this when writing the example.

@feschber
Copy link
Contributor Author

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
Copy link
Owner

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

@feschber
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

cc @whot, would you mind clarifying this bit

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

@whot
Copy link
Contributor

whot commented Jun 19, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

whot commented Jun 20, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@whot
Copy link
Contributor

whot commented Jun 21, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

whot commented Jun 24, 2024

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
Copy link
Owner

so what is the tldr here?

@feschber
Copy link
Contributor Author

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
Copy link
Collaborator

A6GibKm commented Jul 15, 2024

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
Copy link
Contributor Author

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.

@bilelmoussaoui
Copy link
Owner

How should we proceed here? given that I am planning an API break release the upcoming weeks, it would be nice to get this on in as well.

@feschber
Copy link
Contributor Author

Sorry for being inactive on this one, I prioritized other things. I will try to get a draft for the Enum serialization suggestion ready this weekend.

@bilelmoussaoui
Copy link
Owner

No worries, take the time you need! thank you :)

@feschber feschber force-pushed the fix-zero-barrier-id-example branch from 9566369 to 244027b Compare October 18, 2024 16:33
@feschber feschber marked this pull request as draft October 20, 2024 10:36
@feschber feschber force-pushed the fix-zero-barrier-id-example branch from ddfd099 to d9692a1 Compare October 20, 2024 12:16
@feschber feschber force-pushed the fix-zero-barrier-id-example branch from 6ccf7bc to c824960 Compare October 20, 2024 12:30
@feschber feschber marked this pull request as ready for review October 20, 2024 19:10
@feschber
Copy link
Contributor Author

This is now a bit of a compromise between

enum ActivatedBarrier {
    Barrier(BarrierID),
    Unkown,
    NoBarrier,
}

and

Option<Option<NonZeroU32>>

in that we have Option<ActivatedBarrier> returned by the barrier_id() function on the Activated signal.
Where ActivatedBarrier is either a BarrierID or Unkown.

I find this pretty intuitive in that None represents an activation of the input-capture through something other than a barrier and Some(ActivationBarrier) represents an activation through a Barrier that may or may not be known.

One thing I am slightly concerned about is the SetPointerBarriersResponse type which now expects a NonZeroU32 and otherwise would fail to deserialize. This should not be a problem for compliant / not buggy portal implementations however.

Let me know what you think about the approach and also the naming, which could probably be improved as well.

@bilelmoussaoui
Copy link
Owner

From a quick look, the approach looks good to me. The only way to know if there are broken portal implementations is by having a library that enforces what is written in the spec.

So I would say, let us merge this and see how things evolve in the future.

@bilelmoussaoui bilelmoussaoui merged commit fc3e5f6 into bilelmoussaoui:master Oct 25, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants