Delegated dispatchers

[PolyMeilex]

This is an attempt to remove all delegate macros from Smithay, as well as remove obnoxious type constraints on D.

Here is an example diff of ZxdgOutputManagerV1 implementation in Smithay with new delegated dispatches.

impl<D> GlobalDispatch<ZxdgOutputManagerV1, (), D> for OutputManagerState
where
-    D: GlobalDispatch<ZxdgOutputManagerV1, ()>,
-    D: Dispatch<ZxdgOutputManagerV1, ()>,
-    D: Dispatch<ZxdgOutputV1, XdgOutputUserData>,
-    D: 'static,
{
}

All of the constraints on D can be removed, as OutputManagerState is the one implementing them.

This also completely removes the need to write and invoke delegate_*! macros.

I managed to implement delegated GlobalData without any breaking changes just by adding new default generic, but for ResourceData it's more cumbersome, as I had to add new DelegatedResourceData user data type. While this does not introduce any braking changes, the split is weird. As we now have .data::<U>() and .delegated_data::<U, DelegatedTo>() variants. Ideas on how to merge those types together without polluting the whole API are welcome.

(Smithay PR that demonstrates this in action Smithay/smithay#1327)

[elinorbgr]

This is a nice take on this problem! Thanks a lot, the general principle looks good to me 👍

Given this alters the generated code in a non-backward-compatible way, this will be a breaking change of wayland-scanner, and I think we should probably treat it as a breaking change of wayland-server as well as remove the cumbersome macros in the process.

Apart from waiting on the resolution of #698 for our CI to be functional again, I think before this is in a mergeable state it needs:

• Proper documentation of the newly introduced API, and update of the relevant module-level docs
• Equivalent changes for wayland-client
• Proper changelog entries

[ids1024]

For an implementation in wayland-client, will this run into issues with requests that take a new_id?

https://docs.rs/wayland-client/latest/wayland_client/protocol/wl_seat/struct.WlSeat.html#method.get_keyboard for instance is defined as:

pub fn get_keyboard<U: Send + Sync + 'static, D: Dispatch<WlKeyboard, U> + 'static>(
    &self,
    qh: &QueueHandle<D>,
    udata: U
) -> WlKeyboard

Without the delegated implementation the macro generated, Dispatch<WlKeyboard, U> wouldn't be implemented.

Events with a new_id meanwhile are rare, and it seems wayland-server handles these differently (Smithay has to use backend.create_object directly.) It seems a bit odd that wayland-server and wayland-client do this differently.

[codecov]

Codecov Report

Attention: Patch coverage is 54.50000% with 91 lines in your changes are missing coverage. Please review.

Project coverage is 74.11%. Comparing base (eb7a57b) to head (9c1e369).

Files	Patch %	Lines
wayland-server/src/dispatch.rs	59.39%	54 Missing ⚠️
wayland-server/src/client.rs	0.00%	19 Missing ⚠️
wayland-server/src/display.rs	14.28%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
- Coverage   75.34%   74.11%   -1.23%     
==========================================
  Files          48       45       -3     
  Lines        8257     7969     -288     
==========================================
- Hits         6221     5906     -315     
- Misses       2036     2063      +27     

Flag	Coverage Δ
main	58.35% <4.02%> (-0.02%)	⬇️
test--	77.90% <54.50%> (-3.18%)	⬇️
test--server_system	61.72% <54.50%> (-2.78%)	⬇️
test-client_system-	69.17% <54.50%> (-2.98%)	⬇️
test-client_system-server_system	52.12% <54.50%> (-2.70%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[PolyMeilex]

Ok, client side is done.
I went ahead and broke the API of QueueHandle::make_data<_,_, DelegateTo>, by adding the DelegateTo, as I suppose it's rarely used anyway, and this allows us to avoid delegated/non-delegated split of the API.

Code generated by the scanner will just output:

fn get_registry(qh) -> WlRegistry {
    let data = qh.make_data::<_, _, D>();
    ...
}

So nothing will change for the general case of global D type.

We as SCTK on the other hand can call the constructor manually with our DelegateTo type instead:

let data = qh.make_data::<WlRegistry, _, DelegateTo>(());
let wl_registry = display.send_constructor(wl_display::Request::GetRegistry {}, data).unwrap();

I belive that the code needed to call constructors manually is simple enough to no warrant an additional get_registry_delegated::<DelegateTo>() generation, or forcing the DelegateTo generic on regular get_registry.

---

Also it turns out that thanks to ObjectData::data_as_any on client side the generic on Proxy::data getter is totally not needed, I will experiment with similar approach on server side as well.

[ids1024]

I belive that the code needed to call constructors manually is simple enough to no warrant an additional get_registry_delegated::() generation, or forcing the DelegateTo generic on regular get_registry.

Yeah, we wouldn't really want to end up with a duplicated _delegated method for every method that creates an object. Not to require awkward explicit type generics. (And we can't have a generic with a default on methods: rust-lang/rust#36887).

The only other possibility I can think of is if Dispatch was implemented on UserData, not State. Then the dispatcher would be implied by the type of the udata, at the cost of not being able to use standard types like () as a udata. This would also be a bigger breaking change.

[PolyMeilex]

Also it turns out that thanks to ObjectData::data_as_any on client side the generic on Proxy::data getter is totally not needed, I will experiment with similar approach on server side as well.

I gave up, the D generic on server side ObjectData<D> trait makes it harder to do, especially withtrait_upcasting being nightly only, so I'll leave it as is.

Btw, Docs should be up-to-date now as well. So I guess this could be considered ready.

[PolyMeilex]

Gentle ping @elinorbgr, no rush tho.

[elinorbgr]

LGTM overall, a few documentation nitpicks and two last points:

• Please add changelog entries for these changes to the relevant crates (wayland-client, wayland-server and wayland-scanner)
• The two new examples could really use a few comments that explain what they do, what the flow of messages will be, etc...

[elinorbgr]

Aren't we also removing the delegate_dispatch macros in the process? Or do you think there is a good reason to keep them alongside this new API?

[elinorbgr]

I don't think this is a valid rust syntax

[PolyMeilex]

Right, fixed

[PolyMeilex]

Or do you think there is a good reason to keep them alongside this new API?

Nope, removed.
Those are easy to copy-paste if someone really needs them for some reason.

[ids1024]

In wayland-client, Globals::bind still has a bound on State: Dispatch<I, U> + 'static. Presumably it needs to be changed/duplicated to support delegation?

[ids1024]

Seems to work with something like:

    pub fn bind_delegated<I, State, U, DelegateTo>(
        &self,
        qh: &QueueHandle<State>,
        version: RangeInclusive<u32>,
        udata: U,
    ) -> Result<I, BindError>
    where
        I: Proxy + 'static,
        State: 'static,
        U: Send + Sync + 'static,
        DelegateTo: Dispatch<I, U, State> + 'static,
    {
        ...

        Ok(self
            .registry
            .send_constructor(
                wl_registry::Request::Bind { name, id: (I::interface(), version) },
                qh.make_data::<I, U, DelegateTo>(udata),
            )
            .unwrap())

Then bind can be defined as:

    pub fn bind<I, State, U>(
        &self,
        qh: &QueueHandle<State>,
        version: RangeInclusive<u32>,
        udata: U,
    ) -> Result<I, BindError>
    where
        I: Proxy + 'static,
        State: Dispatch<I, U> + 'static,
        U: Send + Sync + 'static,
    {
        self.bind_delegated::<I, State, U, State>(qh, version, udata)
    }

[ids1024]

registry_queue_init likewise requires State: Dispatch<wl_registry::WlRegistry, GlobalListContents>. So without sctk's relegate_registry, this now requires the application using sctk to explicitly implement Dispatch<wl_registry::WlRegistry, GlobalListContents>.