[PATCH weston 01/10] Use fullscreen-shell.xml from wayland-protocols

[Jonas Ådahl]

On Thu, Nov 05, 2015 at 09:00:21PM -0800, Bryce Harrington wrote:
> On Fri, Nov 06, 2015 at 10:51:09AM +0800, Jonas Ådahl wrote:
> > On Thu, Nov 05, 2015 at 11:46:46AM -0800, Bryce Harrington wrote:
> > > On Wed, Nov 04, 2015 at 04:49:50PM +0800, Jonas Ådahl wrote:
> > > > Use the fullscreen-shell protocol XML from the wayland-protocols
> > > > installation, and remove the one we provide ourself.
> > > > 
> > > > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > > >
> > > > diff --git a/clients/fullscreen.c b/clients/fullscreen.c
> > > > index 4fcca3d..be316d0 100644
> > > > --- a/clients/fullscreen.c
> > > > +++ b/clients/fullscreen.c
> > > > @@ -35,7 +35,7 @@
> > > >  #include <linux/input.h>
> > > >  #include <wayland-client.h>
> > > >  #include "window.h"
> > > > -#include "fullscreen-shell-client-protocol.h"
> > > > +#include "fullscreen-shell-unstable-v1-client-protocol.h"
> > > 
> > > Angle brackets should be used here and elsewhere, since with this change
> > > the header file now is located externally from the system rather than
> > > being locally present in the codebase.
> > > 
> > >   #include <fullscreen-shell-unstable-v1-client-protocol.h>
> > 
> > Good point.
> > 
> > >   
> > > >  struct fs_output {
> > > >  	struct wl_list link;
> > > > @@ -46,8 +46,8 @@ struct fullscreen {
> > > >  	struct display *display;
> > > >  	struct window *window;
> > > >  	struct widget *widget;
> > > > -	struct _wl_fullscreen_shell *fshell;
> > > > -	enum _wl_fullscreen_shell_present_method present_method;
> > > > +	struct zwl_fullscreen_shell1 *fshell;
> > > > +	enum zwl_fullscreen_shell1_present_method present_method;
> > > >  	int width, height;
> > > >  	int fullscreen;
> > > >  	float pointer_x, pointer_y;
> > > > @@ -293,10 +293,10 @@ key_handler(struct window *window, struct input *input, uint32_t time,
> > > >  		if (fullscreen->current_output)
> > > >  			wl_output = output_get_wl_output(fullscreen->current_output->output);
> > > >  		fullscreen->present_method = (fullscreen->present_method + 1) % 5;
> > > > -		_wl_fullscreen_shell_present_surface(fullscreen->fshell,
> > > > -						     window_get_wl_surface(fullscreen->window),
> > > > -						     fullscreen->present_method,
> > > > -						     wl_output);
> > > > +		zwl_fullscreen_shell1_present_surface(fullscreen->fshell,
> > > > +						      window_get_wl_surface(fullscreen->window),
> > > > +						      fullscreen->present_method,
> > > > +						      wl_output);
> > > >  		window_schedule_redraw(window);
> > > >  		break;
> > > >  
> > > > @@ -308,8 +308,8 @@ key_handler(struct window *window, struct input *input, uint32_t time,
> > > >  		wl_output = fsout ? output_get_wl_output(fsout->output) : NULL;
> > > >  
> > > >  		/* Clear the current presentation */
> > > > -		_wl_fullscreen_shell_present_surface(fullscreen->fshell, NULL,
> > > > -						     0, wl_output);
> > > > +		zwl_fullscreen_shell1_present_surface(fullscreen->fshell, NULL,
> > > > +						      0, wl_output);
> > > 
> > > Hmm.  With l's and 1's looking so similar in certain fonts, "shell1" is
> > > going to look like a typo to some users.  IMO it would be better to
> > > distinguish this version number with at least an underscore.
> > > "_shell_v1_" would feel more consistent with the scheme being used in
> > > the header file name, protocol file name, macro definitions, etc.
> > 
> > A slight implicit point of it is to make it a bit awkward, so that it's
> > ever so slightly more obvious that this is intended to be temporary.
> > 
> > The other more important reason is I think we already have very long
> > names, and I tried to minimize the extra name length needed.
> 
> Being intentionally awkward seems like an anti-pattern to me.  But I
> would point out that if it is a goal, then making the names overly long
> certainly would achieve that.
> 
> Increasing the length of the names does seem like a valid problem,
> though.  I almost mentioned it but in thinking it out, I figure: a) the
> symbols are intentionally temporary, and making them intentionally
> longer might even encourage making the base names slightly shorter,
> which is in everyone's interest; b) the scheme is already adding 2
> characters, we'd be adding 4 instead, which doesn't seem *that*
> excessive; c) clarity is more important to me than brevity; d) making
> the names more consistent with header, macro names, etc. seems way more
> important than typing convenience.

I see your points. I suppose zwp_..._vN does have more benefits than
zwp_...N, so I'm up for changing.

> 
> If increasing the length is still considered a problem, Bill's
> suggestion to move the version to the prefix might be worth further
> consideration.  It addresses making things clearer without increasing
> character count, although I'm uncertain about its usability since it'll
> impact name searching and sorting.

Yea, not so sure about putting it in the beginning for the reasons you
already pointed out.


Jonas

>  
> > I guess I can be convinced otherwise, but personally I prefer the way it
> > is.
> 
> Well, we all are going to have to live with the scheme...
> 
> > > > diff --git a/configure.ac b/configure.ac
> > > > index e5afbc0..cfac579 100644
> > > > --- a/configure.ac
> > > > +++ b/configure.ac
> > > > @@ -181,6 +181,13 @@ fi
> > > >  PKG_CHECK_MODULES(LIBINPUT_BACKEND, [libinput >= 0.8.0])
> > > >  PKG_CHECK_MODULES(COMPOSITOR, [$COMPOSITOR_MODULES])
> > > >  
> > > > +PKG_CHECK_MODULES(WAYLAND_PROTOCOLS, [wayland-protocols >= 0.1.0],
> > > 
> > > Please also update RELEASING with a sentence or two about needing to
> > > check and update this protocol package version number.
> > 
> > Will do.
> > 
> > 
> > Jonas
> 
> Bryce