[PATCH wayland] client: add a public function to make a roundtrip on a custom queue

[Pekka Paalanen]

On Wed, 16 Jul 2014 11:13:06 +0300
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

> 2014-07-15 20:39 GMT+03:00 Daniel Stone <daniel at fooishbar.org>:
> > Hi,
> >
> >
> > On Tuesday, July 15, 2014, Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> >>
> >> 2014-07-14 22:31 GMT+03:00 Jason Ekstrand <jason at jlekstrand.net>:
> >> > Guilio,
> >> > Would it be better to name it wl_event_queue_roundtrip and just have it
> >> > take
> >> > the wl_event_queue?  I guess it is sort-of a wl_display operation, but
> >> > you
> >> > could argue it either way.  Thoughts?
> >>
> >> You have a point here, it makes more sense.
> >
> >
> > TBH I'd rather steer clear of that nomenclature, since the 'queue' in an
> > immediate request context implies delayed dispatch, rather than on a queue.
> 
> I didn't realize you could read it as "queue a roundtrip on a
> wl_event". I think that the meaning is quite obvious once you know
> that there is a wl_event_queue type in Wayland.

Hi,

hm, that's a tricky one. Yes, the name wl_event_queue_roundtrip() is
confusing when out of context, but as the argument is a wl_event_queue,
that name is the logical and unfortunate result.

Another problem with wl_display_roundtrip_queue() is that it gets both
wl_display and wl_event_queue as arguments. Therefore the function
would need to check that the wl_event_queue indeed belongs to the given
wl_display. It's just a thing one can get wrong, and it has no benefits
that I see.

wl_display_roundtrip_queue() taking only wl_event_queue as an argument
would be strange, because the name implies a wl_display method.

(Up until this point, I would have agreed with Giulio and Jason...)

But, now I see, that we already have:

int wl_display_dispatch_queue(struct wl_display *display,
                              struct wl_event_queue *queue);
int wl_display_dispatch_queue_pending(struct wl_display *display,
                                      struct wl_event_queue *queue);
int wl_display_prepare_read_queue(struct wl_display *display,
                                  struct wl_event_queue *queue);

So there is precendent, that redundant arguments are ok, and
wl_display_roundtrip_queue() would indeed follow the existing scheme.

Btw. I do not see anywhere, where the libwayland-client code would be
checking that the wl_display and the wl_event_queue would actually be
related. Would be a good thing to add more checks to the public API
functions for that.

I think I would go with wl_display_roundtrip_queue() here. Would you
agree?


Thanks,
pq

> >> > On Mon, Jul 14, 2014 at 7:15 AM, Giulio Camuffo
> >> > <giuliocamuffo at gmail.com>
> >> > wrote:
> >> >>
> >> >> wl_display_roundtrip() works on the default queue. Add a parallel
> >> >> wl_display_roundtrip_queue().
> >> >> ---
> >> >>
> >> >> v3: fixed dispatch call in place of dispatch_queue
> >> >>     documented the queue parameter
> >> >>  src/wayland-client.c | 24 +++++++++++++++++++++---
> >> >>  src/wayland-client.h |  2 ++
> >> >>  2 files changed, 23 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/src/wayland-client.c b/src/wayland-client.c
> >> >> index e8aab7e..d2c1b5c 100644
> >> >> --- a/src/wayland-client.c
> >> >> +++ b/src/wayland-client.c
> >> >> @@ -834,15 +834,16 @@ static const struct wl_callback_listener
> >> >> sync_listener = {
> >> >>  /** Block until all pending request are processed by the server
> >> >>   *
> >> >>   * \param display The display context object
> >> >> + * \param queue The queue on which to run the roundtrip
> >> >>   * \return The number of dispatched events on success or -1 on failure
> >> >>   *
> >> >>   * Blocks until the server process all currently issued requests and
> >> >> - * sends out pending events on all event queues.
> >> >> + * sends out pending events on the event queue.
> >> >>   *
> >> >>   * \memberof wl_display
> >> >>   */
> >> >>  WL_EXPORT int
> >> >> -wl_display_roundtrip(struct wl_display *display)
> >> >> +wl_display_roundtrip_queue(struct wl_display *display, struct
> >> >> wl_event_queue *queue)
> >> >>  {
> >> >>         struct wl_callback *callback;
> >> >>         int done, ret = 0;
> >> >> @@ -851,9 +852,10 @@ wl_display_roundtrip(struct wl_display *display)
> >> >>         callback = wl_display_sync(display);
> >> >>         if (callback == NULL)
> >> >>                 return -1;
> >> >> +       wl_proxy_set_queue(callback, queue);
> >> >>         wl_callback_add_listener(callback, &sync_listener, &done);
> >> >>         while (!done && ret >= 0)
> >> >> -               ret = wl_display_dispatch(display);
> >> >> +               ret = wl_display_dispatch_queue(display, queue);
> >> >>
> >> >>         if (ret == -1 && !done)
> >> >>                 wl_callback_destroy(callback);
> >> >> @@ -861,6 +863,22 @@ wl_display_roundtrip(struct wl_display *display)
> >> >>         return ret;
> >> >>  }
> >> >>
> >> >> +/** Block until all pending request are processed by the server
> >> >> + *
> >> >> + * \param display The display context object
> >> >> + * \return The number of dispatched events on success or -1 on failure
> >> >> + *
> >> >> + * Blocks until the server process all currently issued requests and
> >> >> + * sends out pending events on the default event queue.
> >> >> + *
> >> >> + * \memberof wl_display
> >> >> + */
> >> >> +WL_EXPORT int
> >> >> +wl_display_roundtrip(struct wl_display *display)
> >> >> +{
> >> >> +       wl_display_roundtrip_queue(display, &display->default_queue);
> >> >> +}
> >> >> +
> >> >>  static int
> >> >>  create_proxies(struct wl_proxy *sender, struct wl_closure *closure)
> >> >>  {
> >> >> diff --git a/src/wayland-client.h b/src/wayland-client.h
> >> >> index 2a32785..4377207 100644
> >> >> --- a/src/wayland-client.h
> >> >> +++ b/src/wayland-client.h
> >> >> @@ -163,6 +163,8 @@ int wl_display_dispatch_pending(struct wl_display
> >> >> *display);
> >> >>  int wl_display_get_error(struct wl_display *display);
> >> >>
> >> >>  int wl_display_flush(struct wl_display *display);
> >> >> +int wl_display_roundtrip_queue(struct wl_display *display,
> >> >> +                               struct wl_event_queue *queue);
> >> >>  int wl_display_roundtrip(struct wl_display *display);
> >> >>  struct wl_event_queue *wl_display_create_queue(struct wl_display
> >> >> *display);
> >> >>
> >> >> --
> >> >> 2.0.1
> >> >>
> >> >> _______________________________________________
> >> >> wayland-devel mailing list
> >> >> wayland-devel at lists.freedesktop.org
> >> >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >> >
> >> >
> >> _______________________________________________
> >> wayland-devel mailing list
> >> wayland-devel at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel