[server-core] Add signal for creation of clients.

[Pekka Paalanen]

On Wed, 10 Feb 2016 17:35:40 +0200
Pekka Paalanen <ppaalanen at gmail.com> wrote:

> Hi,
> 
> the subject should be:
> [PATCH wayland] server: add listener API for new clients
> 
> On Tue, 9 Feb 2016 17:31:27 +0200
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> 
> > Hi,
> > 
> > thanks, i think this was long overdue. I have a few comments below.  
> 
> Indeed.
> 
> > 2016-02-04 11:59 GMT+02:00  <nicesj at nicesj.com>:  
> > >     Using display object, if a new client is created, emit a signal.
> > >
> > >     In the server-side, we can get the destroy event of a client,    
> > 
> > The indentation is weird from here on.  
> 
> Yeah, this looks like its been copied from git-log or git-show output.
> 
> > >     But there is no way to get the client create event.
> > >     Of course, we can get the client object from the global registry
> > >     binding callbacks.
> > >     But it can be called several times with same client object.
> > >     And even if A client creates display object,
> > >     (so there is a connection), The server could not know that.
> > >     There could be more use-cases not only for this.
> > >
> > >     Please review this and if possible, merge it to the stream.    
> > 
> > I think this line is for the list, it should not be in the commit message.
> >   
> > >
> > >     Signed-off-by: Sung-jae Park <nicesj at nicesj.com>
> > >
> > > diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> > > index e8e1e9c..cb72981 100644
> > > --- a/src/wayland-server-core.h
> > > +++ b/src/wayland-server-core.h
> > > @@ -156,6 +156,21 @@ void
> > >  wl_display_add_destroy_listener(struct wl_display *display,
> > >                                 struct wl_listener *listener);
> > >
> > > +/** Add a listener for getting a notification of creation of clients.
> > > + *  If you add a listener, server will emits a signal if a new client
> > > + *  is created.
> > > + *
> > > + *  \ref wl_client_create
> > > + *  \ref wl_display
> > > + *  \ref wl_listener
> > > + *
> > > + * \param display The display object
> > > + * \param listener Signal handler object  
> 
> Please document what argument will be passed to the notification
> function when the signal is emitted.
> 
> > > + */    
> > 
> > The documentation for the function should be in the .c file.
> >   
> > > +void
> > > +wl_display_add_create_client_listener(struct wl_display *display,
> > > +                                       struct wl_listener *listener);    
> > 
> > I think i would prefer wl_display_add_client_connected_listener, it
> > flows better and makes it more clear that this is about when a client
> > connects to the display. Also i would reword the docs a bit, how about
> > " Registers a listener for the client connection signal.
> >   When a client connects to the display the \a listener will be
> > notified, carrying
> >   a pointer to the new wl_client object."
> >   
> 
> Agreed.
> 
> Another name could be wl_display_add_client_created_listener(). We have
> two cases: one is when clients connect through a socket, and the other
> is when clients get created by calling wl_client_create() directly with
> an fd from socketpair(). "created" would match wl_client_create()
> better.
> 
> > > +
> > >  struct wl_listener *
> > >  wl_display_get_destroy_listener(struct wl_display *display,
> > >                                 wl_notify_func_t notify);
> > > diff --git a/src/wayland-server.c b/src/wayland-server.c
> > > index ae9365f..0eff8f6 100644
> > > --- a/src/wayland-server.c
> > > +++ b/src/wayland-server.c
> > > @@ -96,6 +96,7 @@ struct wl_display {
> > >         struct wl_list client_list;
> > >
> > >         struct wl_signal destroy_signal;
> > > +       struct wl_signal create_client_signal;
> > >
> > >         struct wl_array additional_shm_formats;
> > >  };
> > > @@ -448,6 +449,8 @@ wl_client_create(struct wl_display *display, int fd)
> > >
> > >         wl_list_insert(display->client_list.prev, &client->link);
> > >
> > > +       wl_signal_emit(&display->create_client_signal, client);
> > > +
> > >         return client;
> > >
> > >  err_map:
> > > @@ -864,6 +867,7 @@ wl_display_create(void)
> > >         wl_list_init(&display->registry_resource_list);
> > >
> > >         wl_signal_init(&display->destroy_signal);
> > > +       wl_signal_init(&display->create_client_signal);
> > >
> > >         display->id = 1;
> > >         display->serial = 0;
> > > @@ -1353,6 +1357,13 @@ wl_display_add_destroy_listener(struct wl_display *display,
> > >         wl_signal_add(&display->destroy_signal, listener);
> > >  }
> > >
> > > +WL_EXPORT void
> > > +wl_display_add_create_client_listener(struct wl_display *display,
> > > +                                       struct wl_listener *listener)
> > > +{
> > > +       wl_signal_add(&display->create_client_signal, listener);
> > > +}
> > > +
> > >  WL_EXPORT struct wl_listener *
> > >  wl_display_get_destroy_listener(struct wl_display *display,
> > >                                 wl_notify_func_t notify)  
> 
> The idea seems good to me.

Hi,

one more thing I forgot. Could you write at least one test for this new
feature, please?

I think you could use TEST(post_error_to_one_client) in
tests/display-test.c as an example. You don't have to create any
globals, and the client function only needs to connect and disconnect.
That should allow you to excercise the new listener.

That would be follow-up patch.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160210/36738967/attachment.sig>