Merge lp://qastaging/~mc-return/compiz/compiz.merge-grid-small-cleanup into lp://qastaging/compiz/0.9.10

the xml cleanup is fine.

957 - ${GMOCK_LIBRARY}
958 - ${GMOCK_MAIN_LIBRARY})
959 + ${GMOCK_LIBRARY})

GMOCK_MAIN_LIBRARY is necessary here.

1019 +/*
1020 if (cw == mGrabWindow)
1021 {
1022 - /* TODO: Remove these magic numbers */
1023 - xwc.x = workarea.x () + 50;
1024 - xwc.y = workarea.y () + 50;
1025 + int snapoffThreshold = optionGetSnapoffThreshold ();
1026 +
1027 + xwc.x = workarea.x () + snapoffThreshold;
1028 + xwc.y = workarea.y () + snapoffThreshold;
1029 xwc.width = workarea.width ();
1030 xwc.height = workarea.height ();
1031 cw->configureXWindow (CWX | CWY, &xwc);
1032 }
1033 +*/

I think this ensures the window is moved back on-screen. Not sure though.

Gotta run, will do the rest of the review later.

So I think the intention of that code was to ensure that the window maximizes on the same monitor that the pointer is on. In order to do that correctly, the best way is really to just move it to 0,0 on that output, eg:

if (cw == mGrabWindow)
{
    xwc.x = workarea.x ();
    xwc.y = workarea.y ();
    cw->configureXWindow (CWX | CWY, &xwc);
}

Though, this behavior feels a bit strange - If you want to change it so that keybinding triggers always maximize the window on its current monitor then this should be done in a separate branch. Nevertheless, commenting out the code is incorrect in its current state.

1009 - if ((cw->state () & MAXIMIZE_STATE) &&
1010 - (resize || optionGetSnapoffMaximized ()))
1011 - /* maximized state interferes with us, clear it */
1012 + if ((cw->state () & MAXIMIZE_STATE) && resize)

...

1298 - xwc.x = pointerX - (gw->originalSize.width () / 2);
1299 - xwc.y = pointerY + (cw->border ().top / 2);
1300 + /* The windows x-center is different in this case. */
1301 + if (optionGetSnapbackWindows ())
1302 + {
1303 + xwc.x = pointerX - (gw->originalSize.width () / 2);
1304 + xwc.y = pointerY + (cw->border ().top / 2);
1305 + }
1306 + else /* the user does not want the original size back */
1307 + {
1308 + /* this one is quite tricky to get right */
1309 + xwc.x = pointerX - gw->pointerBufDx - gw->currentSize.width () / 2;
1310 + xwc.y = pointerY - gw->pointerBufDy + cw->border ().top / 2;
1311 + }

No need to change it here, but please do substantive changes separately to cleanups.

1259 + else if (!gw->isGridResized &&
1260 + gw->isGridHorzMaximized &&

These are not aligned.

403 - if (!CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
1404 - return false;
1405 + if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
1406 + return true;
1407
1408 - return true;
1409 + return false;

This is not consistent with the rest of the style of ABI checks and is wrong anyways.

The ABI checks always go like this:

/* Something bad happened */
if (!checkPluginABI ("foo", FOO_ABI) ||
    !checkPluginABI ("bar", BAR_ABI))
    return false;

return true;

For consistency's sake it should also be the same with just the single check.

That being said, this plugins also links in composite and opengl and should check the ABI of those too.

This is going to cranky, but please do not mix any further substantive and formal changes. They make reviewing more time-consuming than it needs to be and unnecessarily causes proposals to be blocked on unrelated items.

> So I think the intention of that code was to ensure that the window maximizes
> on the same monitor that the pointer is on. In order to do that correctly, the
> best way is really to just move it to 0,0 on that output, eg:
>
> if (cw == mGrabWindow)
> {
> xwc.x = workarea.x ();
> xwc.y = workarea.y ();
> cw->configureXWindow (CWX | CWY, &xwc);
> }
>
> Though, this behavior feels a bit strange - If you want to change it so that
> keybinding triggers always maximize the window on its current monitor then
> this should be done in a separate branch. Nevertheless, commenting out the
> code is incorrect in its current state.
>
The problem here is IMHO bug #776435.
It should be changed to maximize windows on the monitor the mousepointer is on.

Restoring Grid windows without the commented code is fine and feels correct, maybe we should restore windows below the unity-panel, which currently is not happening, but the x-coordinate of the window is much better now.
Moving to 50, 50 feels completely off.

> 1009 - if ((cw->state () & MAXIMIZE_STATE) &&
> 1010 - (resize || optionGetSnapoffMaximized ()))
> 1011 - /* maximized state interferes with us, clear it */
> 1012 + if ((cw->state () & MAXIMIZE_STATE) && resize)
>
> ...
>
> 1298 - xwc.x = pointerX - (gw->originalSize.width () / 2);
> 1299 - xwc.y = pointerY + (cw->border ().top / 2);
> 1300 + /* The windows x-center is different in this case. */
> 1301 + if (optionGetSnapbackWindows ())
> 1302 + {
> 1303 + xwc.x = pointerX - (gw->originalSize.width () / 2);
> 1304 + xwc.y = pointerY + (cw->border ().top / 2);
> 1305 + }
> 1306 + else /* the user does not want the original size back */
> 1307 + {
> 1308 + /* this one is quite tricky to get right */
> 1309 + xwc.x = pointerX - gw->pointerBufDx -
> gw->currentSize.width () / 2;
> 1310 + xwc.y = pointerY - gw->pointerBufDy + cw->border ().top /
> 2;
> 1311 + }
>
> No need to change it here, but please do substantive changes separately to
> cleanups.
>
The problem is that I do not have space for 20 compiled Compiz branches on my SSD, so I have one work branch I backport from...

> 1259 + else if (!gw->isGridResized &&
> 1260 + gw->isGridHorzMaximized &&
>
> These are not aligned.
>
I'll look into that.

> 403 - if (!CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
> 1404 - return false;
> 1405 + if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
> 1406 + return true;
> 1407
> 1408 - return true;
> 1409 + return false;
>
> This is not consistent with the rest of the style of ABI checks and is wrong
> anyways.
>
> The ABI checks always go like this:
>
> /* Something bad happened */
> if (!checkPluginABI ("foo", FOO_ABI) ||
> !checkPluginABI ("bar", BAR_ABI))
> return false;
>
> return true;
>
> For consistency's sake it should also be the same with just the single check.
>
> That being said, this plugins also links in composite and opengl and ...

> > So I think the intention of that code was to ensure that the window
> maximizes
> > on the same monitor that the pointer is on. In order to do that correctly,
> the
> > best way is really to just move it to 0,0 on that output, eg:
> >
> > if (cw == mGrabWindow)
> > {
> > xwc.x = workarea.x ();
> > xwc.y = workarea.y ();
> > cw->configureXWindow (CWX | CWY, &xwc);
> > }
> >
> > Though, this behavior feels a bit strange - If you want to change it so that
> > keybinding triggers always maximize the window on its current monitor then
> > this should be done in a separate branch. Nevertheless, commenting out the
> > code is incorrect in its current state.
> >
> The problem here is IMHO bug #776435.
> It should be changed to maximize windows on the monitor the mousepointer is
> on.
>
> Restoring Grid windows without the commented code is fine and feels correct,
> maybe we should restore windows below the unity-panel, which currently is not
> happening, but the x-coordinate of the window is much better now.
> Moving to 50, 50 feels completely off.
>
> > 1009 - if ((cw->state () & MAXIMIZE_STATE) &&
> > 1010 - (resize || optionGetSnapoffMaximized ()))
> > 1011 - /* maximized state interferes with us, clear it */
> > 1012 + if ((cw->state () & MAXIMIZE_STATE) && resize)
> >
> > ...
> >
> > 1298 - xwc.x = pointerX - (gw->originalSize.width () / 2);
> > 1299 - xwc.y = pointerY + (cw->border ().top / 2);
> > 1300 + /* The windows x-center is different in this case. */
> > 1301 + if (optionGetSnapbackWindows ())
> > 1302 + {
> > 1303 + xwc.x = pointerX - (gw->originalSize.width () / 2);
> > 1304 + xwc.y = pointerY + (cw->border ().top / 2);
> > 1305 + }
> > 1306 + else /* the user does not want the original size back */
> > 1307 + {
> > 1308 + /* this one is quite tricky to get right */
> > 1309 + xwc.x = pointerX - gw->pointerBufDx -
> > gw->currentSize.width () / 2;
> > 1310 + xwc.y = pointerY - gw->pointerBufDy + cw->border ().top
> /
> > 2;
> > 1311 + }
> >
> > No need to change it here, but please do substantive changes separately to
> > cleanups.
> >
> The problem is that I do not have space for 20 compiled Compiz branches on my
> SSD, so I have one work branch I backport from...
>
> > 1259 + else if (!gw->isGridResized &&
> > 1260 + gw->isGridHorzMaximized &&
> >
> > These are not aligned.
> >
> I'll look into that.
>
> > 403 - if (!CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
> > 1404 - return false;
> > 1405 + if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
> > 1406 + return true;
> > 1407
> > 1408 - return true;
> > 1409 + return false;
> >
> > This is not consistent with the rest of the style of ABI checks and is wrong
> > anyways.
> >
> > The ABI checks always go like this:
> >
> > /* Something bad happened */
> > if (!checkPluginABI ("foo", FOO_ABI) ||
> > !checkPluginABI ("bar", BAR_ABI))
> > return false;
> >
> > return true;
> ...

> > So I think the intention of that code was to ensure that the window
> maximizes
> > on the same monitor that the pointer is on. In order to do that correctly,
> the
> > best way is really to just move it to 0,0 on that output, eg:
> >
> > if (cw == mGrabWindow)
> > {
> > xwc.x = workarea.x ();
> > xwc.y = workarea.y ();
> > cw->configureXWindow (CWX | CWY, &xwc);
> > }
> >
> > Though, this behavior feels a bit strange - If you want to change it so that
> > keybinding triggers always maximize the window on its current monitor then
> > this should be done in a separate branch. Nevertheless, commenting out the
> > code is incorrect in its current state.
> >
> The problem here is IMHO bug #776435.
> It should be changed to maximize windows on the monitor the mousepointer is
> on.
>
> Restoring Grid windows without the commented code is fine and feels correct,
> maybe we should restore windows below the unity-panel, which currently is not
> happening, but the x-coordinate of the window is much better now.
> Moving to 50, 50 feels completely off.
>

I meant to reply to this bit :)

If you think it makes sense to remove the commented code, then just remove it rather than leaving it commented out.

>
> Test program:
>
> int main ()
> {
> bool a = true;
> bool b = true;
> bool c = true;
>
> if (!a || !b || !c)
> return 1;
>
> return 0;
> }
>
> g++ -O0 assembly:
>
> 4004fc: 0f b6 45 fd movzbl -0x3(%rbp),%eax
> 400500: 83 f0 01 xor $0x1,%eax
> 400503: 84 c0 test %al,%al
> 400505: 75 16 jne 40051d <main+0x31>
> 400507: 0f b6 45 fe movzbl -0x2(%rbp),%eax
> 40050b: 83 f0 01 xor $0x1,%eax
> 40050e: 84 c0 test %al,%al
> 400510: 75 0b jne 40051d <main+0x31>
> 400512: 0f b6 45 ff movzbl -0x1(%rbp),%eax
> 400516: 83 f0 01 xor $0x1,%eax
> 400519: 84 c0 test %al,%al
> 40051b: 74 07 je 400524 <main+0x38>
>
> int main ()
> {
> bool a = false;
> bool b = false;
> bool c = false;
>
> if (a && b && c)
> return 1;
>
> return 0;
> }
>
> Assembly:
>
> 4004fc: 80 7d fd 00 cmpb $0x0,-0x3(%rbp)
> 400500: 74 13 je 400515 <main+0x29>
> 400502: 80 7d fe 00 cmpb $0x0,-0x2(%rbp)
> 400506: 74 0d je 400515 <main+0x29>
> 400508: 80 7d ff 00 cmpb $0x0,-0x1(%rbp)
> 40050c: 74 07 je 400515 <main+0x29>
>
> The latter version is technically faster, because you can directly compare the
> booleans without negating them.
>
Do not forget that we expect almost always true, in fact it will be true in 100% of cases if
the plugin is okay, which it normally should be.

> However, this is a micro-optimization really. It saves 6 instructions and for
> 50 plugins would save 300 instructions total, which would make for a
> theoretical 4 one-hundred-millionths of a second optimization on a Pentium 4
> system (at 6500 MIPS).
>
If I do one such optimization per week, in a year it is 15000 instructions less to
execute, times say 5 million computers running Compiz will save a lot of energy ;)

> It probably makes more sense to keep it consistent with the other code. Though
> that's not to say that I wouldn't accept a best-case 4 one-hundred-millionth
> of a second optimization.

Yes, I agree 100%. It should be consistent in all cases. We also should report
to the console in the case something goes wrong, which just a few of the plugins
currently do.
Also water will currently not correctly execute without FBOs being enabled, but
there is not even a log message about it...

> I just don't think its worth the one hour of effort
> spent to write it.
It took about 30 minutes to fix it for all of the plugins here locally.
The improved error reporting will take another while of course and also to check that all ABI dependencies,
that each plugin has, get checked...

>
> If you think it makes sense to remove the commented code, then just remove it
> rather than leaving it commented out.

I did not remove it yet, because the problem is not 100% fixed - it should remind me that there is a TODO there...
The y-restore postion could be better, namely below the unity/gnome/whatever-panel...

>
> >
> > If you think it makes sense to remove the commented code, then just remove
> it
> > rather than leaving it commented out.
>
> I did not remove it yet, because the problem is not 100% fixed - it should
> remind me that there is a TODO there...
> The y-restore postion could be better, namely below the unity/gnome/whatever-
> panel...

In that case, it should either be fixed here or replaced with a TODO note in the source code.

If you want to place a window below the panel, the correct way to do it is to place it at the x position of the current output work-area plus the left and right window borders, for example:

CompOutput *currentOutput = &screen->outputDevs ()[screen->outputForGeometry (w->geometry ())];
const CompRect &workArea (currentOutput->workArea ());

int x = workArea.x () + w->border.left
int y = workArea.y () + w->border.top

> the xml cleanup is fine.
>
> 957 - ${GMOCK_LIBRARY}
> 958 - ${GMOCK_MAIN_LIBRARY})
> 959 + ${GMOCK_LIBRARY})
>
> GMOCK_MAIN_LIBRARY is necessary here.
>

Good catch. Committed accidentially, because my work branch has yet to be rebased on trunk. Fixed in r3693.

Ok, all fixed now.
Also made sure grid-maximizing will never overwrite the windows' original size.

The good news:
Recent testing shows the window restores below the panel anyway in all cases, except when a window was first grid-maximized, then grid-resized via keyboard and then restored, because the original size was stored when the window was maximized in this special case.
I have fixed this also now, so a maximized window will never overwrite the original size...

The other changes are good. Can you please revert this one?

1394 - if (!CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
1395 - return false;
1396 + if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
1397 + return true;
1398
1399 - return true;
1400 + return false;

As mentioned before, I'd far prefer that the ABI check logic be kept consistent with the other plugins. If you think it would be worthwhile to change it, then it should be changed all at once in all the plugins in a separate MP.

> The other changes are good. Can you please revert this one?
>
TBH, I would rather like to invest my time in adding composite and opengl here, which you correctly identified as missing...

> 1394 - if (!CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
> 1395 - return false;
> 1396 + if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
> 1397 + return true;
> 1398
> 1399 - return true;
> 1400 + return false;
>
> As mentioned before, I'd far prefer that the ABI check logic be kept
> consistent with the other plugins.

I am for consistency also.

> If you think it would be worthwhile to
> change it, then it should be changed all at once in all the plugins in a
> separate MP.

Hmm, would mean 30 minutes of additional work - most probably this costs less time than the discussions necessary to get those changes in on a per-plugin basis...

Looks great now, thanks!

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-autolanding/36/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-amd64-autolanding/36/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-armhf-autolanding/36/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-i386-autolanding/36/console

Ah - nice, another quilt patch I forgot:

Hunk #1 FAILED at 23.
Hunk #2 FAILED at 68.
2 out of 2 hunks FAILED -- rejects in file plugins/grid/grid.xml.in

:(

I will quilt-fix this one, because here it is inevitable...
- but I want to note that I hate it -> the steps needed would be many:

1. First I have to install quilt, then setup the quilt environment variables in .quiltrc or .bashrc.
2. Then I have to throw away this branch and check out a new lp:compiz branch, because I have to know in advance of hacking if the quilt patching will fail - there is no way I can go from here...
3. Next I have to "quilt push -a" in my fresh lp:compiz branch to apply the patches, while doing that I read things like "Hunk #1 succeeded at 557 with fuzz 1 (offset 95 lines).", which help a lot to increase my confidence in quilt.
4. Now I would have to revert the grid change, refresh the patch and "quilt pop -a".
5. Then is the time to copy the new version over.
6. Later I can "quilt push -a" again.
7. Afterwards I have to search the stuff the quilt patch changed in the first place to mimic this behaviour manually, which means I have to add/remove the stuff quilt should remove, just that I have to do that manually... I am happy I know what exactly has been changed by quilt, because "patching file plugins/ezoom/ezoom.xml.in" is all the info quilt dared to gave me before. Thanks, such log messages make me a lot wiser.
8. To make sure what has been changed by quilt, ubuntu-config.patch (the old version) has to be checked in a editor.
9. I am now sure what has been changed and can edit plugins/grid/grid.xml.in.
10. After I have done all that I can now try to "quilt refresh", which I can do "as often as I like" \o/ HURRA !!!
11. As last step I can now do a "qilt pop -a" to update my patch and hopefully finish this nightmarish patching experience...

Less than 12 steps -> Easy, isn't it ?

I guess I will hack ubuntu-config.patch manually instead, should be much easier...
Or is there some quilt magic I do not know about ?

The problems you've mentioned are pretty easy to deal with:

0. export QUILT_PATCHES=debian/patches
1. You don't need to check out a new branch. If a patch fails to apply
it will create .rej and .orig files, but those aren't checked in
2. I'm not sure what you mean by "revert the grid change". The correct
approach to deal with a failing quilt patch is to look at the .rej
file and apply the changes there by-hand. Once that's done you run
quilt refresh

Also - I'd place a bit more confidence in quilt. Its like any other
merge tool - it just looks for a block of text surrounded by a certain
amount of context in a file and then applies a change to that block.
It either works or it doesn't - if it can't find the exact bit that
you want it to change, it will create patch reject files and you just
need to apply the change by hand.

On Fri, May 10, 2013 at 10:15 PM, MC Return <email address hidden> wrote:
> I will quilt-fix this one, because here it is inevitable...
> - but I want to note that I hate it -> the steps needed would be many:
>
> 1. First I have to install quilt, then setup the quilt environment variables in .quiltrc or .bashrc.
> 2. Then I have to throw away this branch and check out a new lp:compiz branch, because I have to know in advance of hacking if the quilt patching will fail - there is no way I can go from here...
> 3. Next I have to "quilt push -a" in my fresh lp:compiz branch to apply the patches, while doing that I read things like "Hunk #1 succeeded at 557 with fuzz 1 (offset 95 lines).", which help a lot to increase my confidence in quilt.
> 4. Now I would have to revert the grid change, refresh the patch and "quilt pop -a".
> 5. Then is the time to copy the new version over.
> 6. Later I can "quilt push -a" again.
> 7. Afterwards I have to search the stuff the quilt patch changed in the first place to mimic this behaviour manually, which means I have to add/remove the stuff quilt should remove, just that I have to do that manually... I am happy I know what exactly has been changed by quilt, because "patching file plugins/ezoom/ezoom.xml.in" is all the info quilt dared to gave me before. Thanks, such log messages make me a lot wiser.
> 8. To make sure what has been changed by quilt, ubuntu-config.patch (the old version) has to be checked in a editor.
> 9. I am now sure what has been changed and can edit plugins/grid/grid.xml.in.
> 10. After I have done all that I can now try to "quilt refresh", which I can do "as often as I like" \o/ HURRA !!!
> 11. As last step I can now do a "qilt pop -a" to update my patch and hopefully finish this nightmarish patching experience...
>
> Less than 12 steps -> Easy, isn't it ?
>
> I guess I will hack ubuntu-config.patch manually instead, should be much easier...
> Or is there some quilt magic I do not know about ?
> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-grid-small-cleanup/+merge/161330
> You are reviewing the proposed merge of lp:~mc-return/compiz/compiz.merge-grid-small-cleanup into lp:compiz.

--
Sam Spilsbury

> The problems you've mentioned are pretty easy to deal with:
>
> 0. export QUILT_PATCHES=debian/patches
> 1. You don't need to check out a new branch. If a patch fails to apply
> it will create .rej and .orig files, but those aren't checked in

Okay, so by "quilt push -a -f" I can enforce the patching and get the file
plugins/grid/grid.xml.in, with this content then:

--- plugins/grid/grid.xml.in
+++ plugins/grid/grid.xml.in
@@ -23,17 +23,16 @@
   <option name="put_center_key" type="key">
       <_short>Put Center</_short>
       <_long>Move window to the center</_long>
- <default>&lt;Control&gt;&lt;Alt&gt;KP_5</default>
   </option>
   <option name="put_left_key" type="key">
       <_short>Put Left</_short>
       <_long>Move window to the left edge</_long>
- <default>&lt;Control&gt;&lt;Alt&gt;KP_4</default>
+ <default>&lt;Control&gt;&lt;Super&gt;Left</default>
   </option>
   <option name="put_right_key" type="key">
       <_short>Put Right</_short>
       <_long>Move window to the right edge</_long>
- <default>&lt;Control&gt;&lt;Alt&gt;KP_6</default>
+ <default>&lt;Control&gt;&lt;Super&gt;Right</default>
   </option>
   <option name="put_top_key" type="key">
       <_short>Put Top</_short>
@@ -68,7 +67,6 @@
   <option name="put_maximize_key" type="key">
       <_short>Maximize</_short>
       <_long>Maximize window</_long>
- <default>&lt;Control&gt;&lt;Alt&gt;KP_0</default>
   </option>
   <option name="put_restore_key" type="key">
       <_short>Restore</_short>

> 2. I'm not sure what you mean by "revert the grid change". The correct
> approach to deal with a failing quilt patch is to look at the .rej
> file and apply the changes there by-hand. Once that's done you run
> quilt refresh
>
How can I easily change this "by hand" or how do I have to change it in
this case ?
All this steps are not necessary -> I could manipulate the ubuntu-config.patch
itself from the beginning if I have to do it manually anyway...

> Also - I'd place a bit more confidence in quilt. Its like any other
> merge tool - it just looks for a block of text surrounded by a certain
> amount of context in a file and then applies a change to that block.
> It either works or it doesn't - if it can't find the exact bit that
> you want it to change, it will create patch reject files and you just
> need to apply the change by hand.
>
Well, most of the quilt patches just change some shortcuts for the Ubuntu
Compiz version - it is not exactly the best and easiest way to do that.
I could think of several better solutions to change default shortcuts
for Ubuntu, like a separated config file with separated defaults that get
imported for Ubuntu for example...

*the file is of course called plugins/grid/grid.xml.in.rej

On Fri, May 10, 2013 at 11:06 PM, MC Return <email address hidden> wrote:
>> The problems you've mentioned are pretty easy to deal with:
>>
>> 0. export QUILT_PATCHES=debian/patches
>> 1. You don't need to check out a new branch. If a patch fails to apply
>> it will create .rej and .orig files, but those aren't checked in
>
> Okay, so by "quilt push -a -f" I can enforce the patching and get the file
> plugins/grid/grid.xml.in, with this content then:
>
> --- plugins/grid/grid.xml.in
> +++ plugins/grid/grid.xml.in
> @@ -23,17 +23,16 @@
> <option name="put_center_key" type="key">
> <_short>Put Center</_short>
> <_long>Move window to the center</_long>
> - <default>&lt;Control&gt;&lt;Alt&gt;KP_5</default>
> </option>
> <option name="put_left_key" type="key">
> <_short>Put Left</_short>
> <_long>Move window to the left edge</_long>
> - <default>&lt;Control&gt;&lt;Alt&gt;KP_4</default>
> + <default>&lt;Control&gt;&lt;Super&gt;Left</default>
> </option>
> <option name="put_right_key" type="key">
> <_short>Put Right</_short>
> <_long>Move window to the right edge</_long>
> - <default>&lt;Control&gt;&lt;Alt&gt;KP_6</default>
> + <default>&lt;Control&gt;&lt;Super&gt;Right</default>
> </option>
> <option name="put_top_key" type="key">
> <_short>Put Top</_short>
> @@ -68,7 +67,6 @@
> <option name="put_maximize_key" type="key">
> <_short>Maximize</_short>
> <_long>Maximize window</_long>
> - <default>&lt;Control&gt;&lt;Alt&gt;KP_0</default>
> </option>
> <option name="put_restore_key" type="key">
> <_short>Restore</_short>
>
>> 2. I'm not sure what you mean by "revert the grid change". The correct
>> approach to deal with a failing quilt patch is to look at the .rej
>> file and apply the changes there by-hand. Once that's done you run
>> quilt refresh
>>
> How can I easily change this "by hand" or how do I have to change it in
> this case ?
> All this steps are not necessary -> I could manipulate the ubuntu-config.patch
> itself from the beginning if I have to do it manually anyway...

So, the idea is that you apply the patch, and then edit the files to
which the patch applies should a part of the patch fail. For example:

x.patch patches foo.c and bar.c. A hunk on foo.c fails. This generates
foo.c.orig, a partially patched foo.c and foo.c.rej . Quilt is
"tracking" changes to foo.c at this point, so you examine foo.c.rej
and apply the remaining changes to foo.c manually. Once quilt refresh
is run, this will regenerate x.patch based on the correct data.

>>
> Well, most of the quilt patches just change some shortcuts for the Ubuntu
> Compiz version - it is not exactly the best and easiest way to do that.
> I could think of several better solutions to change default shortcuts
> for Ubuntu, like a sep...

>
> So, the idea is that you apply the patch, and then edit the files to
> which the patch applies should a part of the patch fail. For example:
>
> x.patch patches foo.c and bar.c. A hunk on foo.c fails. This generates
> foo.c.orig, a partially patched foo.c and foo.c.rej . Quilt is
> "tracking" changes to foo.c at this point, so you examine foo.c.rej
> and apply the remaining changes to foo.c manually. Once quilt refresh
> is run, this will regenerate x.patch based on the correct data.
>
Okay, I get the idea ;) - but the correct version to do it would nevertheless
need the 11-step plan...
I will do a partially hacked version now - I think it will be the easiest way:

1. I will first remove grid from ubuntu-config.patch completely so the hunk won't fail.
2. Then I'll "quilt push -a" to apply the rest of the patch to the source
3. Now I'll do the changes to grid.xml.in.
4. Refresh
5. "qilt pop -a"

This should save me the complicated step of manual patch adjustment...

Just a note to myself:

quilt push -a
quilt add -P debian/patches/ubuntu-config.patch plugins/grid/grid.xml.in
make changes...
quilt refresh debian/patches/ubuntu-config.patch

Take care, cause this branch could explode your computer ;)

Just joking - all done. I'll closed my eyes and put my trust in quilt ;)

quilt and me are very good friends now :)

Note: I also fixed bug #1172641, which needed quilt involvement on the way...

Thanks for refreshing the patches.

1653 +<<<<<<< TREE
1654 if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
1655 return true;
1656 +=======
1657 + if (!CompPlugin::checkPluginABI ("composite", CORE_ABIVERSION) ||
1658 + !CompPlugin::checkPluginABI ("core", CORE_ABIVERSION) ||
1659 + !CompPlugin::checkPluginABI ("opengl", CORE_ABIVERSION))
1660 + return false;
1661 +>>>>>>> MERGE-SOURCE

Merge conflict.

   <option name="snapback_windows" type="bool">
    <_short>Snap Windows Back To Original Size</_short>
    <_long>Snaps windows back to their original size if dragged away from their gridded position.</_long>
    <default>true</default>
   </option>
   <option name="cycle_sizes" type="bool">
    <_short>Cycle Through Multiple Sizes</_short>
    <_long>Cycle through multiple different sizes by using the same keyboard shortcut multiple times in a row.</_long>
    <default>false</default>
   </option>
  <subgroup>

On lines 492-503 of grid.xml.in, if the subgroup tag has been removed then the options also need to be unindented

> Thanks for refreshing the patches.
>
> 1653 +<<<<<<< TREE
> 1654 if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION))
> 1655 return true;
> 1656 +=======
> 1657 + if (!CompPlugin::checkPluginABI ("composite", CORE_ABIVERSION)
> ||
> 1658 + !CompPlugin::checkPluginABI ("core", CORE_ABIVERSION) ||
> 1659 + !CompPlugin::checkPluginABI ("opengl", CORE_ABIVERSION))
> 1660 + return false;
> 1661 +>>>>>>> MERGE-SOURCE
>
> Merge conflict.
>
Noooooooooooooooooooooooooooo
Didn't I look at the diff ???
Looking now how to best fix that mess...

> <option name="snapback_windows" type="bool">
> <_short>Snap Windows Back To Original
> Size</_short>
> <_long>Snaps windows back to their original
> size if dragged away from their gridded position.</_long>
> <default>true</default>
> </option>
> <option name="cycle_sizes" type="bool">
> <_short>Cycle Through Multiple Sizes</_short>
> <_long>Cycle through multiple different sizes
> by using the same keyboard shortcut multiple times in a row.</_long>
> <default>false</default>
> </option>
> <subgroup>
>
> On lines 492-503 of grid.xml.in, if the subgroup tag has been removed then the
> options also need to be unindented

I have to investigate what happened here...

/offtopic on

Regarding correct indentation of the .xml files -> they are all pretty messed up -> could vera++ help us here also ?

/offtopic off

I did not see the merge conflict, because I had to merge lp:compiz for it to appear...

Note:
Please let us fix the .xml indentation issue in some follow-up... it is really time for this to land now.

> >
> > On lines 492-503 of grid.xml.in, if the subgroup tag has been removed then
> the
> > options also need to be unindented

F*ck it. Fixed

Note:
The known Grid bugs left (after this MP has landed):

Bug #1115341 - Grid resize: Placing maximized windows on the top edge using shortcuts does not take the panel into account

Bug #1172923 - Grid: Window movement animations missing, when grid-keyboard-resizing from a non-semi-maximized state

Sam, if you have any ideas on how to fix those 2 bugs, please give me a hint ;)
Especially getting the missing animations one would be a "nice to have" ;)

Additional note:
Bug #776435 - Window maximizes and semi-maximizes on the wrong workspace

can also be triggered by Grid and needs to be finally fixed in 0.9.10.
Unfortunately this involves manipulating the core, which makes testing the fix properly a bit harder without fully installing the self-compiled Compiz...
Nevertheless the fix itself should not be hard to implement -> the logic just needs to be changed to always semi-maximize/maximize on the output device containing the pointer, not the output device, on which the biggest part of the window resides when it is semi-maximized/maximized.
For plugins using this function this change could be problematic, maybe the best solution would be to create a new function with the old behaviour and make all plugins call that instead, so the behaviour for them won't change, while maximizing and semimaximizing would use the new function...

> Additional note:
> Bug #776435 - Window maximizes and semi-maximizes on the wrong workspace
>
> can also be triggered by Grid and needs to be finally fixed in 0.9.10.
> Unfortunately this involves manipulating the core, which makes testing the fix
> properly a bit harder without fully installing the self-compiled Compiz...

If you are still having trouble with this I'd suggest poking me on IRC about it, I should be able to help you out.

> Nevertheless the fix itself should not be hard to implement -> the logic just
> needs to be changed to always semi-maximize/maximize on the output device
> containing the pointer, not the output device, on which the biggest part of
> the window resides when it is semi-maximized/maximized.

This can be easily worked around from within the grid plugin - just move the window so that it is on the monitor completely before it is maximized. I'd rather not have that behavioural change in core, because it creates a strong disconnect between the window location and the monitor that the window is maximized on - especially for windows may request to maximize themselves or triggering a maximize by keybinding.

In any case this branch is good for now.