> > 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 should > > check the ABI of those too. > > > You are right - I will add those 2 also. > > I want to change the style of the ABI checks for all of them... > Why ? > Because the new version is faster. > We expect the outcome of the ABI-check to be true, otherwise the plugin won't > load - so the return value will be true for all plugins in normal usage. It won't be any faster. Every plugin that loads still needs to check that it was compiled against and runs against the same ABI as every other plugin that it depends on. So there will still be three calls to checkPluginABI in the normal case. For example: if (checkPluginABI () && checkPluginABI () && checkPluginABI ()) return true; return false; There are always three checks here, because all three must pass in order to return true. Likewise: if (!checkPluginABI () || !checkPluginABI () || !checkPluginABI ()) return false; return true; There are also three checks here in the normal case. For each branch that is false (eg !true), we must proceed to the next to find another that evaluates to true. None do, so we return true. > > 1. We do not have to negate in the if-condition checks. > 2. We exit the function immediately, while still being in the if-condition > check. > > Minor optimization, but we have 50 plugins, so a lot of those ABI checks... 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 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 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 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 400502: 80 7d fe 00 cmpb $0x0,-0x2(%rbp) 400506: 74 0d je 400515 400508: 80 7d ff 00 cmpb $0x0,-0x1(%rbp) 40050c: 74 07 je 400515 The latter version is technically faster, because you can directly compare the booleans without negating them. 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). 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. I just don't think its worth the one hour of effort spent to write it.