On Fri, 17 Jun 2016 13:38:25 +0300 Pekka Paalanen <ppaala...@gmail.com> wrote:
> On Sat, 19 Mar 2016 18:14:35 +0200
> Giulio Camuffo <giuliocamu...@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
pgpQEWF0RjWVF.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
