Review Request 120471: Add Registry::sync() signal

[Martin Gräßlin]

> On Oct. 6, 2014, 12:25 p.m., Martin Gräßlin wrote:
> > src/client/registry.h, line 407
> > <https://git.reviewboard.kde.org/r/120471/diff/1/?file=316079#file316079line407>
> >
> >     I would recommend to move it to ConnectionThread as it's more connection related then registry related.
> >     
> >     Also please add a test case for it (both threaded and unthreaded) - should be fairly simple in fact.
> 
> Sebastian Kügler wrote:
>     I've experimented with that, and ran into a few issues.
>     
>     We need to call
>     
>     wl_display_get_registry(display);
>     wl_display_sync(display);
>     
>     in this order, that's what the Wayland API suggests.
>     
>     I've tried moving the whole callback mechanism into ConnectionThread, but ConnectionThread doesn't know enough about the Registry to issue the wl_display_sync right after wl_display_get_registry. Essentially, sync really is a global sync, and comes from the registry, rather than the ConnectionThread.
>     
>     Semantically, it does make more sense in ConnectionThread, along with connected() and failed() etc.. So I've tried keeping the mechanism in Registry, but emitting ConnectionThread's signal, if a connectionthread has been set on the registry. This also doesn't work very well, as in the tests (as an example), one doesn't get a sync signal unless one sets up the Registry. ConnectionThread isn't what triggers the wl_display_sync callback, it's set up in the registry.
>     
>     So what the wl_display_sync callback really does, is "I'm done announcing interfaces", it is in fact more related to Registry, and not to ConnectionThread. In that regard, the current patch may be the best option, perhaps giving sync() a clearer name, and reflecting it better in the API docs? Other solutions altogether?

Thanks for looking into it. So yeah let's rename it to make more clear what the signal is about, in my book it can be very verbose kind of "interfacesAnnounced"


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120471/#review67980
-----------------------------------------------------------


On Oct. 7, 2014, 3:56 a.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120471/
> -----------------------------------------------------------
> 
> (Updated Oct. 7, 2014, 3:56 a.m.)
> 
> 
> Review request for kwin, Plasma and Martin Gräßlin.
> 
> 
> Repository: kwayland
> 
> 
> Description
> -------
> 
> Add Registry::sync() signal
> 
> Emitted when the Wayland display is done flushing the initial interface
> callbacks, announcing wl_display properties. This can be used to compress
> events. Note that this signal is emitted only after announcing interfaces,
> such as outputs, but not after receiving callbacks of interface properties,
> such as the output's geometry, modes, etc..
> This signal is emitted from the wl_display_sync callback.
> 
> For this, we add a wl_callback_listener to the registry's Private,
> enqueue its events properly, if necessary, and trigger the signal
> through a callback mechanism similar to the wl_registry callbacks.
> 
> This signal allows users of the API to find out when the signal
> emissions, such as outputAnnounced, etc. for all currently existing
> interfaces is complete.
> 
> 
> Diffs
> -----
> 
>   autotests/client/test_wayland_registry.cpp 571be0f 
>   src/client/registry.h 9e63a2b 
>   src/client/registry.cpp 22f9484 
> 
> Diff: https://git.reviewboard.kde.org/r/120471/diff/
> 
> 
> Testing
> -------
> 
> tests in libkscreen exercise this feature, it works as expected, meaning I can notify when all initial synchronization is done.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20141007/9e895319/attachment-0001.html>