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

client-toolkit/toplevel_info: Ignore done before cosmic events #39

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Oct 21, 2024

The compositor sends a done event for the
ext_foreign_toplevel_handle_v1 before we call get_cosmic_toplevel. Ignore that and wait for the done that occurs after we get cosmic events.

zcosmic_toplevel_handle_v1::state seems to be a mandatory event, so it is valid if awkward to use for the purpose. I don't know about a cleaner solution. We could just ignore the first done, but technically it could race if the compostior sends a state change before it has processed the get_cosmic_toplevel. Maybe a roundtrip/sync could work.

The compositor sends a `done` event for the
`ext_foreign_toplevel_handle_v1` before we call `get_cosmic_toplevel`.
Ignore that and wait for the `done` that occurs after we get cosmic
events.

`zcosmic_toplevel_handle_v1::state` seems to be a mandatory event, so it
is valid if awkward to use for the purpose. I don't know about a cleaner
solution. We could just ignore the first `done`, but technically it
could race if the compostior sends a state change before it has
processed the `get_cosmic_toplevel`. Maybe a roundtrip/sync could work.
@ids1024 ids1024 requested a review from a team October 21, 2024 23:10
Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we somehow gate this on the cosmic global being available in the first place?

@ids1024
Copy link
Member Author

ids1024 commented Oct 22, 2024

Currently there is a hard requirement on the cosmic toplevel info protocol (this is supposed to be generic over cosmic-toplevel v1 and v2).

If we want to produce any useful output when only ext-foreign-toplevel-list-v1, that may need some more testing, and some other changes. I guess TopevelInfo doesn't need to change (fields like state can be an empty set). But currently ToplevelInfoState::toplevels provides an iterator over zcosmic_toplevel_info_v1::ZcosmicToplevelInfoV1 and Option<ToplevelInfo>.

I see #40 adds a constructor that does not panic when cosmic-toplevel isn't present (which is good), but ToplevelInfoState::try_new still returns None without the cosmic protocol. So that won't change this either.

I guess we could merge both PRs, and if we want to change the API and add support for compositors that just have ext-foreign-toplevel-list-v1, that can be done later. I'd like to also abstract wlr-foreign-toplevel-management, though it would be handy if that had an identifier to relate it to ext-foreign-toplevel-list (which we'll need for toplevel capture)...

@Drakulix
Copy link
Member

Currently there is a hard requirement on the cosmic toplevel info protocol (this is supposed to be generic over cosmic-toplevel v1 and v2).

Got it.

I guess we could merge both PRs, and if we want to change the API and add support for compositors that just have ext-foreign-toplevel-list-v1, that can be done later.

Yeah, let's do it later, I thought we already did this, but makes sense.

I'd like to also abstract wlr-foreign-toplevel-management, though it would be handy if that had an identifier to relate it to ext-foreign-toplevel-list (which we'll need for toplevel capture)...

Honestly I think there is a higher chance to finally get ext-foreign-toplevel-info/management going once the experimental protocol stage is available in a couple of weeks.

@Drakulix Drakulix merged commit 0451452 into main Oct 22, 2024
4 checks passed
@Drakulix Drakulix deleted the toplevel-cosmic-done branch October 22, 2024 15:36
@Consolatis
Copy link

Consolatis commented Oct 25, 2024

Doesn't this indicate an issue on the server side of the protocol? I'd expect the base object and all extensions objects to schedule a done event on initialization rather than sending it immediately. That way there would only be one done event rather than a done for each object.

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