[PATCH v2 1/2] shell & compositor: add parameters for set_fullsceen

[Pekka Paalanen]

On Wed, 11 Jan 2012 15:01:37 +0800
Juan Zhao <juan.j.zhao at linux.intel.com> wrote:

> Thanks very much for your review.
> 
> On Tue, 2012-01-10 at 13:41 +0200, Pekka Paalanen wrote:
> > > +     struct wl_list *tlist=(struct wl_list *)ec->surface_list.next;
> > > +     int count=0;
> > > +
> > > +     while((es->type != WESTON_SURFACE_TYPE_GENERAL) && count
> > <1024){
> > > +             tlist=tlist->next;
> > > +             if((tlist == (struct wl_list *)ec->surface_list.next)
> > &&
> > > +                count!=0)
> > > +                     break;
> > > +             switch(es->type){
> > > +                     case WESTON_SURFACE_TYPE_CURSOR:
> > > +                             cursorsurface = es;
> > > +                             break;
> > > +                     case WESTON_SURFACE_TYPE_PANEL:
> > > +                             panelsurface = es;
> > > +                             break;
> > > +                     default:
> > > +                             fprintf(stderr,"unkown surface type!
> > \n");
> > > +                             return;
> > > +             }
> > > +             es = container_of(tlist->next, struct weston_surface,
> > link);
> > > +             count++;
> > > +     }
> > > +     if(count == 1024) {
> > > +             fprintf(stderr,"too many surfaces!\n");
> > > +             return -1;
> > > +     }
> > 
> > What is this supposed to do?
> add the surface type for cursor/panel/general is to clarify the surface
> stack:
> -|--cursor
> -|--panel
> -|--general
> -|--general
> 
> so that, we can figure out the window surfaces and handle different
> request for them.
> 
> to differentiate
> 1.
> -|--cursor
> -|--panel
> -|--general(fullscreen)
> -|--general
> and
> 2.
> -|--cursor
> -|--panel
> -|--general
> -|--general(fullscreen)
> 
> We only hope to only change mode in case 1. So we need to find out the
> first general surface. If we only use (es->fs_output=output), we will
> seldom go to the fullscreen handling path, because most of the time, the
> first surface is that "cursor" type.
> 
> > Why is there an arbitrary limit on 'count'?
> Just hope to limit the supported surface number, 1024 is just some
> number for reference, we can add some macro to define it. Because, in
> some case, too many surfaces may cause system too busy to work. 
> But if you think it is unnecessary, we can remove this part.

We iterate the list of all surfaces in several places already, adding a
random failure case here is not going to make things any better.

> > Are you forgetting there can be several cursors?
> Yeah, good concern. We should consider this. Will modify the code.
> > What is PANEL doing here?
> For the tip fullscreen surface, we will draw only that surface and the
> cursor part. Now it is only useful to figure out the general part and
> cursor part. And no further usage.

Yes, so it all really is just for working around the data structures
that have become insufficient to handle the surface stack.

It needs to change at some point, but I'm not suggesting you would need
to do it now. Jesse Barnes' sprite code would benefit from saner data
structures, too, I think.

I would say, that don't use too much effort on supporting multiple
cursors or even multiple outputs (meaning multiple panels, backgrounds)
for now, since the surface list needs to be changed into something else.
After that, doing the right thing will be much cleaner.

Kristian, do you agree?

> > 
> > Maybe all this just stems from the fact that a single list of rendered
> > surfaces is not sufficient, and we should use a more appropriate data
> > structures supporting layers?
> > 
> 
> > >  
> > > diff --git a/src/compositor.h b/src/compositor.h
> > > index 031b7d4..9907abb 100644
> > > --- a/src/compositor.h
> > > +++ b/src/compositor.h
> > > @@ -32,6 +32,15 @@
> > >  #include <EGL/egl.h>
> > >  #include <EGL/eglext.h>
> > >  
> > > +#define WESTON_SURFACE_FULLSCREEN_NONE 0
> > > +#define WESTON_SURFACE_FULLSCREEN_SCALE 1
> > > +#define WESTON_SURFACE_FULLSCREEN_FORCE 2
> > > +#define WESTON_SURFACE_FULLSCREEN_FILL 3
> > > +#define WESTON_SURFACE_TYPE_CURSOR 0
> > > +#define WESTON_SURFACE_TYPE_PANEL 1
> > > +#define WESTON_SURFACE_TYPE_GENERAL 2
> > 
> > Why is PANEL here? Panel as a concept should not exist here, it is a
> > shell thing.
> We need some type to figure out that is not the general one. For current
> usage, it is panel.

Well, if we follow the current design, where all surfaces are in a
single list, the fullscreen surfaces should be on top of the panels,
and you don't need to special-case the panels. But it is very difficult
to maintain the proper order in the surface list.

I guess it's ok for now.

> 
> And for other parts, agree, will modify the codes.
> 
> 
> Thanks a lot,
> Juan
> 

Cheers,
pq