[PATCH weston 1/2] compositor: add weston_surface_set_description()

[Giulio Camuffo]

2014-11-27 12:42 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> On Thu, 27 Nov 2014 10:17:21 +0200
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> Just a quick comment below, I haven't looked at it carefully yet.
>>
>> --
>> Giulio
>>
>> 2014-11-27 9:07 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
>> > From: Pekka Paalanen <pq at iki.fi>
>> >
>> > When printing out logs from Weston's actions, mainly for debugging, it
>> > can be very difficult to identify the different surfaces.  Inspecting
>> > the configure function pointer is not useful, as the configure functions
>> > may live in modules.
>> >
>> > Add vfunc get_description to weston_surface, which will produce a short,
>> > human-readable description of the surface, which allows identifying it
>> > better, rather than just looking at the surface size, for instance.
>> >
>> > Set the description from most parts of Weston, to identify cursors and
>> > drag icons, and panels, backgrounds, screensavers and lock surfaces, and
>> > the desktop shell's application surfaces.
>> >
>> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>> > ---
>> >  desktop-shell/input-panel.c |   9 ++++
>> >  desktop-shell/shell.c       | 101 ++++++++++++++++++++++++++++++++++++++++++++
>> >  src/compositor.c            |  17 ++++++++
>> >  src/compositor.h            |   7 +++
>> >  src/data-device.c           |  19 +++++++++
>> >  src/input.c                 |  10 +++++
>> >  6 files changed, 163 insertions(+)
>
> ...
>
>> > diff --git a/src/compositor.c b/src/compositor.c
>> > index 17e930c..ea5cc14 100644
>> > --- a/src/compositor.c
>> > +++ b/src/compositor.c
>> > @@ -2736,6 +2736,13 @@ weston_subsurface_parent_commit(struct weston_subsurface *sub,
>> >                 weston_subsurface_synchronized_commit(sub);
>> >  }
>> >
>> > +static int
>> > +subsurface_get_description(struct weston_surface *surface,
>> > +                          char *buf, size_t len)
>> > +{
>> > +       return snprintf(buf, len, "sub-surface");
>> > +}
>> > +
>> >  static void
>> >  subsurface_configure(struct weston_surface *surface, int32_t dx, int32_t dy)
>> >  {
>> > @@ -2819,6 +2826,14 @@ weston_surface_set_role(struct weston_surface *surface,
>> >         return -1;
>> >  }
>> >
>> > +WL_EXPORT void
>> > +weston_surface_set_description(struct weston_surface *surface,
>>
>> I find the naming a bit confusing, I'd expect this to set the
>> description of the surface, that is a char*. The get_description vfunc
>> on the other hand doesn't return the function passed here, but the
>> char*. I'd call it something like
>> weston_surface_set_description_func(), or maybe
>> weston_surface_set_descriptor().
>
> Yes, weston_surface_set_get_description_func() it would be indeed, but
> it was getting a bit long, especially when the argument is usually
> a fairly long word too.
>
> Descriptor smells a bit like an fd thing, though.
>
> weston_surface_set_... eh... umm
>
> What if I did s/description/label/g?
>
> So we'd have e.g.:
> weston_surface_set_label_func(surface, shell_surface_get_label)
> and weston_surface::get_label field.
>
> How's that?

I think that's ok.

>
>
> Thanks,
> pq
>
>
>>
>> > +                              int (*desc)(struct weston_surface *,
>> > +                                          char *, size_t))
>> > +{
>> > +       surface->get_description = desc;
>> > +}
>> > +