[PATCH weston v3 6/8] ivi-shell: Support tracking active surfaces

[Pekka Paalanen]

On Fri, 3 Jun 2016 19:24:40 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Thu, May 26, 2016 at 06:02:00PM +0300, Pekka Paalanen wrote:
> > On Thu,  7 Apr 2016 16:44:21 -0700
> > Bryce Harrington <bryce at osg.samsung.com> wrote:
> >   
> > > Activity for ivi-shell follows either click or touch.  Only a single
> > > surface can be active in the shell at a time.
> > > 
> > > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > > ---
> > >  Makefile.am           |  4 ++-
> > >  configure.ac          |  2 ++
> > >  ivi-shell/ivi-shell.c | 10 ++++++
> > >  ivi-shell/ivi-shell.h |  2 ++
> > >  src/compositor.c      | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  5 files changed, 107 insertions(+), 2 deletions(-)  
> > 
> > Hi,
> > 
> > this looks like commit squashing accident, I would not expect to find
> > the generic server implementation to be found in this patch. Please
> > split.

> > > diff --git a/src/compositor.c b/src/compositor.c
> > > index 8e01d38..c89e443 100644
> > > --- a/src/compositor.c
> > > +++ b/src/compositor.c
> > > @@ -51,6 +51,8 @@
> > >  #include <time.h>
> > >  #include <errno.h>
> > >  
> > > +#include <idle-inhibit-unstable-v1-server-protocol.h>  
> > 
> > Use "" as this header is local, not from an installed location.
> >   
> > > +
> > >  #include "timeline.h"
> > >  
> > >  #include "compositor.h"
> > > @@ -2412,7 +2414,7 @@ weston_output_inhibited_outputs(struct weston_compositor *compositor)
> > >  			continue;
> > >  
> > >  		/* Does the view's surface inhibit this output? */
> > > -		if (!view->surface->inhibit_screensaving)
> > > +		if (!view->surface->inhibit_idling)  
> > 
> > Wait, there is a typo in an earlier patch?
> > Please squash this hunk in the appropriate patch, otherwise I think
> > there is a commit that won't build.
> >   
> > >  			continue;
> > >  
> > >  		inhibited_outputs_mask |= view->output_mask;
> > > @@ -4631,6 +4633,89 @@ compositor_bind(struct wl_client *client,
> > >  				       compositor, NULL);
> > >  }
> > >  
> > > +struct weston_idle_inhibitor {
> > > +	struct weston_surface *surface;
> > > +};
> > > +
> > > +static void
> > > +destroy_idle_inhibitor(struct wl_resource *resource)
> > > +{
> > > +	struct weston_idle_inhibitor *inhibitor = wl_resource_get_user_data(resource);
> > > +
> > > +	inhibitor->surface = NULL;
> > > +	free(inhibitor);
> > > +}
> > > +
> > > +static void
> > > +idle_inhibitor_destroy(struct wl_client *client, struct wl_resource *resource)
> > > +{
> > > +	struct weston_idle_inhibitor *inhibitor = wl_resource_get_user_data(resource);
> > > +
> > > +	assert(inhibitor);
> > > +
> > > +	inhibitor->surface->inhibit_idling = false;  
> > 
> > What if the surface had several inhibitor objects? Here destroying just
> > the first one would cause the inhibit to go away, while other inhibitor
> > objects remained.  
> 
> Why would a surface have several inhibitor objects?

Your protocol spec allows them, AFAICS, so someone will surely do
that. I think is also intuitive on what should happen with multiple
inhibitors on the same surface, rather than some murky corner-case
where you can't even decide what the correct behaviour should be.


Thanks,
pq


> 
> > I think you need to maintain a list or a refcount instead.
> > 
> > The destruction is missing.
> >   
> > > +}
> > > +
-------------- 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/20160606/c28554b4/attachment-0001.sig>