[PATCH] Add a new wl_proxy_set_listener function
Pekka Paalanen
ppaalanen at gmail.com
Fri Nov 25 14:47:38 UTC 2016
On Fri, 17 Jun 2016 13:38:25 +0300
Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Sat, 19 Mar 2016 18:14:35 +0200
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
> > The wl_proxy_add_listener function is ill-named, because it suggests
> > that a proxy can have many listeners. Instead it can only have one so
> > deprecate the old function and add a new wl_proxy_set_listener. The
> > implementation of the new function is exactly the same implementation
> > wl_proxy_add_listener had, and that is now implemented by calling
> > wl_proxy_set_listener.
> > Also make the scanner output new <interface>_set_listener functions.
>
> Hi,
>
> a good idea.
>
> Missing S-o-b. I suppose this is the latest patch? It doesn't say v2.
> I marked the other one as superseded.
>
> This patch has an easy to solve conflict with the proxy wrappers patch,
> and now there is also this:
>
> tests/queue-test.c: In function ‘client_test_queue_proxy_wrapper’:
> tests/queue-test.c:242:2: warning: ‘wl_callback_add_listener’ is deprecated (declared at ./protocol/wayland-client-protocol.h:1123) [-Wdeprecated-declarations]
> wl_callback_add_listener(callback, &sync_listener_roundtrip, &done);
> ^
> tests/queue-test.c: In function ‘client_test_queue_set_queue_race’:
> tests/queue-test.c:289:2: warning: ‘wl_callback_add_listener’ is deprecated (declared at ./protocol/wayland-client-protocol.h:1123) [-Wdeprecated-declarations]
> wl_callback_add_listener(callback, &sync_listener_roundtrip, &done);
> ^
>
>
> > ---
> > src/scanner.c | 20 ++++++++++++++++++--
> > src/wayland-client-core.h | 12 +++++++++++-
> > src/wayland-client.c | 23 +++++++++++++++++++++--
> > tests/display-test.c | 2 +-
> > tests/queue-test.c | 12 ++++++------
> > tests/test-compositor.c | 4 ++--
> > 6 files changed, 59 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/scanner.c b/src/scanner.c
> > index 04747e3..441ab1f 100644
> > --- a/src/scanner.c
> > +++ b/src/scanner.c
> > @@ -1267,10 +1267,10 @@ emit_structs(struct wl_list *message_list, struct interface *interface, enum sid
> >
> > if (side == CLIENT) {
> > printf("static inline int\n"
> > - "%s_add_listener(struct %s *%s,\n"
> > + "%s_set_listener(struct %s *%s,\n"
> > "%sconst struct %s_listener *listener, void *data)\n"
> > "{\n"
> > - "\treturn wl_proxy_add_listener((struct wl_proxy *) %s,\n"
> > + "\treturn wl_proxy_set_listener((struct wl_proxy *) %s,\n"
> > "%s(void (**)(void)) listener, data);\n"
> > "}\n\n",
> > interface->name, interface->name, interface->name,
> > @@ -1278,6 +1278,22 @@ emit_structs(struct wl_list *message_list, struct interface *interface, enum sid
> > interface->name,
> > interface->name,
> > indent(37));
> > + printf("#ifndef WL_HIDE_DEPRECATED\n");
>
> We cannot use WL_HIDE_DEPRECATED, because people may already be using
> it, and then this change would break their code. I think we should look
> into some other projects on how to deal with deprecation properly.
> WL_HIDE_DEPRECATED worked only once, and no more functions can be moved
> behind it.
>
> > + printf("static inline int\n"
> > + "%s_add_listener(struct %s *%s,\n"
> > + "%sconst struct %s_listener *listener, void *data) WL_DEPRECATED;\n",
> > + interface->name, interface->name, interface->name,
> > + indent(14 + strlen(interface->name)), interface->name);
>
> We can't use function attributes on definitions, only on declarations?
>
> > + printf("static inline int\n"
> > + "%s_add_listener(struct %s *%s,\n"
> > + "%sconst struct %s_listener *listener, void *data)\n"
> > + "{\n"
> > + "\treturn %s_set_listener(%s, listener, data);\n"
> > + "}\n",
> > + interface->name, interface->name, interface->name,
> > + indent(14 + strlen(interface->name)), interface->name,
> > + interface->name, interface->name);
> > + printf("#endif //WL_HIDE_DEPRECATED\n\n");
> > }
> > }
> >
>
> Otherwise the change to scanner looks fine.
>
> We should probably think of a standard procedure in making deprecations
> to the API. Who would like to take that task?
>
> It would be three parts: compile time warnings, hiding API from
> libwayland headers, and removing API from scanner-generated headers. I
> think we might need some kind of a running version form of
> WL_HIDE_DEPRECATED, one for each release, plus an option for scanner to
> "stop emitting wrappers for things deprecated up to and including
> release 1.x". Or should we just forget about hiding/removing deprecated
> API?
>
> Please start a new thread about this.
>
> > diff --git a/src/wayland-client-core.h b/src/wayland-client-core.h
> > index 91f7e7b..f748a42 100644
> > --- a/src/wayland-client-core.h
> > +++ b/src/wayland-client-core.h
> > @@ -159,7 +159,7 @@ void
> > wl_proxy_destroy(struct wl_proxy *proxy);
> >
> > int
> > -wl_proxy_add_listener(struct wl_proxy *proxy,
> > +wl_proxy_set_listener(struct wl_proxy *proxy,
> > void (**implementation)(void), void *data);
> >
> > const void *
> > @@ -251,6 +251,16 @@ wl_display_read_events(struct wl_display *display);
> > void
> > wl_log_set_handler_client(wl_log_func_t handler);
> >
> > +
> > +/* Deprecated functions below */
> > +#ifndef WL_HIDE_DEPRECATED
>
> Again, cannot do this.
>
> > +
> > +int
> > +wl_proxy_add_listener(struct wl_proxy *proxy,
> > + void (**implementation)(void), void *data) WL_DEPRECATED;
> > +
> > +#endif
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index 297c7a5..fe729b0 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -458,7 +458,7 @@ wl_proxy_destroy(struct wl_proxy *proxy)
> > * \memberof wl_proxy
> > */
> > WL_EXPORT int
> > -wl_proxy_add_listener(struct wl_proxy *proxy,
> > +wl_proxy_set_listener(struct wl_proxy *proxy,
> > void (**implementation)(void), void *data)
> > {
> > if (proxy->object.implementation || proxy->dispatcher) {
> > @@ -1094,7 +1094,7 @@ wl_display_roundtrip_queue(struct wl_display *display, struct wl_event_queue *qu
> > if (callback == NULL)
> > return -1;
> > wl_proxy_set_queue((struct wl_proxy *) callback, queue);
> > - wl_callback_add_listener(callback, &sync_listener, &done);
> > + wl_callback_set_listener(callback, &sync_listener, &done);
>
> Here is the easy conflict with the proxy wrappers changes.
>
> > while (!done && ret >= 0)
> > ret = wl_display_dispatch_queue(display, queue);
> >
> > @@ -1960,3 +1960,22 @@ wl_log_set_handler_client(wl_log_func_t handler)
> > {
> > wl_log_handler = handler;
> > }
> > +
> > +
> > +/** \cond */ /* Deprecated functions below. */
> > +
> > +/** Deprecated. Use \a wl_proxy_set_listener instead.
> > + */
> > +WL_EXPORT int
> > +wl_proxy_add_listener(struct wl_proxy *proxy,
> > + void (**implementation)(void), void *data)
> > +{
> > + return wl_proxy_set_listener(proxy, implementation, data);
> > +}
> > +
> > +/** \endcond */
> > +
> > +/* Functions at the end of this file are deprecated. Instead of adding new
> > + * code here, add it before the comment above that states:
> > + * Deprecated functions below.
> > + */
>
> Seems fine to me.
>
> > diff --git a/tests/display-test.c b/tests/display-test.c
> > index f9f8160..d7e1fd8 100644
> > --- a/tests/display-test.c
> > +++ b/tests/display-test.c
> > @@ -161,7 +161,7 @@ client_get_seat_with_info(struct client *c, struct handler_info *hi)
> >
> > assert(hi);
> > hi->seat = NULL;
> > - wl_registry_add_listener(reg, ®istry_listener, hi);
> > + wl_registry_set_listener(reg, ®istry_listener, hi);
> > wl_display_roundtrip(c->wl_display);
> > assert(hi->seat);
> >
> > diff --git a/tests/queue-test.c b/tests/queue-test.c
> > index 02865ae..48927a4 100644
> > --- a/tests/queue-test.c
> > +++ b/tests/queue-test.c
> > @@ -67,7 +67,7 @@ client_test_proxy_destroy(void)
> >
> > registry = wl_display_get_registry(display);
> > assert(registry != NULL);
> > - wl_registry_add_listener(registry, ®istry_listener,
> > + wl_registry_set_listener(registry, ®istry_listener,
> > &counter);
> > assert(wl_display_roundtrip(display) != -1);
> >
> > @@ -121,12 +121,12 @@ client_test_multiple_queues(void)
> > state.done = false;
> > callback1 = wl_display_sync(state.display);
> > assert(callback1 != NULL);
> > - wl_callback_add_listener(callback1, &sync_listener, &state);
> > + wl_callback_set_listener(callback1, &sync_listener, &state);
> > wl_proxy_set_queue((struct wl_proxy *) callback1, queue);
> >
> > state.callback2 = wl_display_sync(state.display);
> > assert(state.callback2 != NULL);
> > - wl_callback_add_listener(state.callback2, &sync_listener, NULL);
> > + wl_callback_set_listener(state.callback2, &sync_listener, NULL);
> > wl_proxy_set_queue((struct wl_proxy *) state.callback2, queue);
> >
> > wl_display_flush(state.display);
> > @@ -172,12 +172,12 @@ client_test_queue_roundtrip(void)
> > /* arm a callback on the default queue */
> > callback1 = wl_display_sync(display);
> > assert(callback1 != NULL);
> > - wl_callback_add_listener(callback1, &sync_listener_roundtrip, &done1);
> > + wl_callback_set_listener(callback1, &sync_listener_roundtrip, &done1);
> >
> > /* arm a callback on the other queue */
> > callback2 = wl_display_sync(display);
> > assert(callback2 != NULL);
> > - wl_callback_add_listener(callback2, &sync_listener_roundtrip, &done2);
> > + wl_callback_set_listener(callback2, &sync_listener_roundtrip, &done2);
> > wl_proxy_set_queue((struct wl_proxy *) callback2, queue);
> >
> > /* roundtrip on default queue must not dispatch the other queue. */
> > @@ -191,7 +191,7 @@ client_test_queue_roundtrip(void)
> > done1 = false;
> > callback1 = wl_display_sync(display);
> > assert(callback1 != NULL);
> > - wl_callback_add_listener(callback1, &sync_listener_roundtrip, &done1);
> > + wl_callback_set_listener(callback1, &sync_listener_roundtrip, &done1);
> >
> > wl_display_roundtrip_queue(display, queue);
> > assert(done1 == false);
> > diff --git a/tests/test-compositor.c b/tests/test-compositor.c
> > index b01e8af..9170957 100644
> > --- a/tests/test-compositor.c
> > +++ b/tests/test-compositor.c
> > @@ -429,7 +429,7 @@ registry_handle_globals(void *data, struct wl_registry *registry,
> > c->tc = wl_registry_bind(registry, id, &test_compositor_interface, ver);
> > assert(c->tc && "Failed binding to registry");
> >
> > - wl_proxy_add_listener((struct wl_proxy *) c->tc,
> > + wl_proxy_set_listener((struct wl_proxy *) c->tc,
> > (void *) &tc_listener, c);
> > }
> >
> > @@ -452,7 +452,7 @@ struct client *client_connect()
> > * registry so that client can define it's own listener later */
> > reg = wl_display_get_registry(c->wl_display);
> > assert(reg);
> > - wl_registry_add_listener(reg, ®istry_listener, c);
> > + wl_registry_set_listener(reg, ®istry_listener, c);
> > wl_display_roundtrip(c->wl_display);
> > assert(c->tc);
> >
>
> The rest is trivially correct.
>
> So, maybe we could have this patch without the WL_HIDE_DEPRECATED bits
> now, and then figure out how we are going to deprecate and break the
> API (not ABI) in a tractable way?
>
> (I think libwayland-client is a low-level enough library that we can
> never break ABI, especially considering you cannot use different
> versions of it in the same process and EGL implementation use it and so
> forth, so even statically linking it won't help forgotten binary-only
> programs.)
Hi Emil,
if you ever have the time to work out a good deprecation and new
API/ABI management scheme for Wayland, like we've talked for Weston
IIRC, the change in this patch might be a nice complicated test subject.
This patch is deprecating library ABI and API, and changing what
symbols wayland-scanner generates which also affects libwayland API
(not ABI). One would need to handle not only the usual stuff with a
library, but the scanner changing what it requires from the library and
what it provides.
That said, I'm not sure when I'd be able to join reviewing such patches.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161125/38a2b4a3/attachment.sig>
More information about the wayland-devel
mailing list