Created attachment 99512 [details] [review]
Merge patch

Iain writes:


Attached is the patch from the branch to trunk. We need to add Keith
Packard's xcompmgr licence somewhere, I dunno where it should go really,
here's the relevant blurb from xcompmgr.c:

/*
 * $Id$
 *
 * Copyright © 2003 Keith Packard
 *
 * Permission to use, copy, modify, distribute, and sell this software
and its
 * documentation for any purpose is hereby granted without fee, provided
that
 * the above copyright notice appear in all copies and that both that
 * copyright notice and this permission notice appear in supporting
 * documentation, and that the name of Keith Packard not be used in
 * advertising or publicity pertaining to distribution of the software
without
 * specific, written prior permission.  Keith Packard makes no
 * representations about the suitability of this software for any
purpose.  It
 * is provided "as is" without express or implied warranty.
 *
 * KEITH PACKARD DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
 * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN
NO
 * EVENT SHALL KEITH PACKARD BE LIABLE FOR ANY SPECIAL, INDIRECT OR
 * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
OF USE,
 * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR
OTHER
 * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 * PERFORMANCE OF THIS SOFTWARE.
 */

What all is included in the patch:
 * Basic compositing using Xrender extension that can be turned on and
off.
 * Drop shadows
 * Translucency
 * Funky previews on the alt-tab dialog.

Now we need to work out:
a) What effects we want the compositor to do
  - such as, minimising, fading, hilighting, things like that. Should
the panel have a drop shadow or not. Should it just cast the shadow onto
the desktop? Could we have a shorter drop shadow offset for tooltip
windows (so that the apparent distance between windows is larger than
the apparent distance between tooltips and their parent)
Not that I'm condoning any or all of these

b) How we should make Metacity take advantage of the compositor
  - such as, should we make the alt-tab dialogs translucent and all
alpha blingy like the volume control onscreen display...actually thats
all I can think of at the moment :)

Anyway, There's definitely some minor further work to be done on the
compositor without answering these...would you prefer if I kept working
in the branch or should I move to trunk once you commit this patch?

Is this license GPL compatible as metacity is GPL?  Do we need to contact Keith Packard to see if he would license under the GPL?

Well, I'm not a lawyer, but it's my belief that what we see above is the BSD licence with the advertising clause removed (what the FSF calls the "modified BSD licence"). The FSF explicitly say that this is compatible with the GPL here: <http://www.gnu.org/licenses/gpl-faq.html> and further that this means that it can be combined with GPL code, the result also being GPL.

I didn't bother looking at the compositor.[ch] or c-*.[ch] files, since I've never really looked at them anyway and just let Havoc or Soeren or Thomas handle it.  All the bits in other files look fine to me; we should definitely get the copyright bits straight before committing.  Once that's done and Thomas okays it, let's merge it into trunk.

Thanks Iain!

I checked out the branch and I get an error compiling:

screen.c:822: error: MetaScreen has no member named wm_cm_selection_window

03:51 < marnanel> Meanwhile: Can anyone who knows about licences look at bug 499081 and tell me whether the code in the patch from xcompmgr (which appears to me to be non-advertising-clause BSD) can be merged with metacity, which is GPL?
03:51 < marnanel> The licence block from the merging code is in the bug.
03:56 <@jdub> marnanel: yes, you can include the BSD bits
03:57 <@jdub> marnanel: you'll have to keep the copyright of course
03:57 < davyd> linking BSD code and GPL code is acceptable
03:57 < davyd> just keep the BSD licensed files BSD licensed
04:08 < marnanel> okay, loveliness.

IMHO that is not correct,

From section 2 of the GPL:

b)  You must cause any work that you distribute or publish, that in whole or in part contains or is derived from the Program or any part thereof, to be licensed as a whole at no charge to all third parties under the terms of this License.

and later in section 2:

These requirements apply to the modified work as a whole. If identifiable sections of that work are not derived from the Program, and can be reasonably considered independent and separate works in themselves, then this License, and its terms, do not apply to those sections when you distribute them as separate works. But when you distribute the same sections as part of a whole which is a work based on the Program, the distribution of the whole must be on the terms of this License, whose permissions for other licensees extend to the entire whole, and thus to each and every part regardless of who wrote it.

Alex: As mentioned at the link Thomas gave above, namely:
  http://www.gnu.org/philosophy/license-list.html
The FSF and their lawyers have explicitly stated that the BSD license is compatible with the GPL and that "[compatibility] means you can combine a module which was released under that license with a GPL-covered module to make one larger program."

If the FSF says its so, it's good enough for me.  If you believe the FSF is wrong, you should probably tell them to fix their page.  If anyone is worried, we could ping Keith...or ping Luis once he's on the board about whether we should feel free to follow http://www.gnu.org/philosophy/license-list.html.

(In reply to comment #6)
> I checked out the branch and I get an error compiling:
> 
> screen.c:822: error: MetaScreen has no member named wm_cm_selection_window

Well found; this is indeed a bug (though a minor one). The problem is twofold:

1) configure.in still turns compositing off unless it's asked explicitly to turn it on, even if it finds you can run it:

if test x$enable_compositor = xyes; then
   have_xcomposite=yes
   echo "CompositeExt support forced on"
elif test x$enable_compositor = xauto; then
   echo "Not building compositing manager by default now, must enable explicitly to get it."
   have_xcomposite=no
else
   have_xcomposite=no
fi

This needs to be addressed, but it's a trivial fix.

2) screen.c:822 refers to the field wm_cm_selection_window which doesn't exist unless compositing is enabled. It needs to be wrapped in "#ifdef HAVE_COMPOSITE_EXTENSIONS". Again, trivial fix, and if I don't find anything more major wrong with the patch I'll do it myself.

Iain: While we're on the subject of those lines in screen.c:

3) the line

  g_print ("Not managing our own windows\n");

should be

  meta_warning ("Not managing our own windows\n");

4) and I get it every time I attempt to do anything; what's up with that?

Aha, reading over the code this morning I see that this is generated when we attempt to manage our own windows. Are you intending this code to be run whether or not we have a compositor? In that case, we should just "continue" there and not print an error, or (better) we should only pass windows in on the converse of the "if" condition. Either way, we shouldn't g_print on a perfectly normal condition.

Created attachment 101233 [details] [review]
Merge patch

There were a few nitpicks involving use of g_print or g_warning when they should have been meta_verbose or meta_warning; I fixed these. The criteria I specified were met and I have merged; it looks lovely; closing this bug now.

(In reply to comment #11)
> Iain: While we're on the subject of those lines in screen.c:
> 
> 3) the line
> 
>   g_print ("Not managing our own windows\n");
> 
> should be
> 
>   meta_warning ("Not managing our own windows\n");
> 
> 4) and I get it every time I attempt to do anything; what's up with that?
> 

Oh, I never realised this bug existed...sorry for not replying earlier.

In that case it was my fault for failing to cc you!

No, it seems I'm on the cc, I just never noticed it...