foreign-toplevel-management: Report parent toplevel

[dkondor]

This is an implementation for swaywm/wlr-protocols#52

Works together with: https://github.com/dkondor/wayfire/tree/foreign-toplevel-report-parent

Example client: https://github.com/dkondor/cairo-dock-core/tree/wayland_egl

Tested: not displaying child views in the taskbar

Untested: actually grouping together views with the same parent

I'm happy to work on additional test cases if needed.

Potential issues:

• I've added a field storing the parent to the wlr_foreign_toplevel_handle_v1 struct. This is necessary so that foreign_toplevel_manager_bind() can send information about parents to the new client. This breaks ABI though.
• In wlr_foreign_toplevel_handle_v1_destroy(), I've added code to check if the destroyed handle is not stored in the parent field for any other toplevel. I don't really like this solution, since this makes destroying a toplevel handle scale with the number of toplevels managed here (and destroying N toplevels will have a complexity of N^2). Since the number of toplevels is expected to be low, I don't think it is a big problem in practice though. Potential alternatives:
  -- Store the children of a toplevel in a list. I don't like this, since it essentially duplicates functionality that is likely already present in the compositor elsewhere.
  -- Do not do check this and require the caller to ensure that either (1) a parent handle is only destroyed after all child handles; (2) wlr_foreign_toplevel_handle_v1_set_parent() is called with NULL or another parent before destroying the parent handle
• Strangely, I could not find any documentation for wl_resource_find_for_client(), so I used it based on the assumption that it does what I think. I'd be grateful if someone more familiar with these interfaces could confirm that I'm using it correctly.

[dkondor]

Related PR for wayfire: WayfireWM/wayfire#769

[ammen99]

See comments.

[ammen99]

LGTM once the style issues are fixed.

[emersion]

Hmm. This won't work because the parent == toplevel->parent check in wlr_foreign_toplevel_handle_v1_set_parent will bail out early.

I think it'd make sense to just send the event here.

[dkondor]

I was thinking the caller would always set the handle for the child before destroying the parent, but actually there is no guarantee for this, so it is better to send it here. I've added comments to make it clear that this function will send parent event for any child toplevels.

[emersion]

The order of these arguments is a little bit misleading, I'd prefer having the wl_resource first and then the parent. And maybe we can replace the wl_resource argument with a proper wlr_foreign_toplevel_handle_v1, to avoid mixing resource types up.

[dkondor]

Agreed, I've changed the order. I prefer using the resource, since this is a static function that is only called from two places in this file and the resource is already available, especially here:
https://github.com/dkondor/wlroots/blob/cf88b7f0bf73602eeff3a9161b1ea9a919ec4128/types/wlr_foreign_toplevel_management_v1.c#L589
it seems simpler to just use it.

[emersion]

Apart from this, looks fine to me.

[emersion]

If possible, please update our test client in examples/.

[dkondor]

HI,
thanks, I've addressed the review comment and also updated the example client. Let me know if there's something more (and also, if I should squash everything into one commit).

[emersion]

LGTM, thanks!