Merge lp:~brandontschaefer/compiz/restore-orig-pos-lp.892012-fix into lp:compiz/0.9.10

[Brandon Schaefer]

If we receive a stateNotifyChange, we check if we are in a max state. If so, overwrite the xwc with either X | W, or Y | H depending on the semi max state with the orig position before we went into a semi max state. This way when we leave any sort of max state the orig position is restored.

If the window is being grabbed we do not overwrite the position, which it gets its own value at that point (instead of the orig pos).

If we receive a stateNotifyChange, we check if we are in a max state. If so, overwrite the xwc with either X | W, or Y | H depending on the semi max state with the orig position before we went into a semi max state. This way when we leave any sort of max state the orig position is restored.

If the window is being grabbed we do not overwrite the position, which it gets its own value at that point (instead of the orig pos).

[Sam Spilsbury (smspillaz)]

On the right track, a few suggestions:

1. It might be good to leave saveWc ().x / y / width / height alone unless they were not already set (eg, saveMask () & CWX | CWY | CWWidth | CWHeight)
2. Style Issues:

16 + else if (isGridVertMaximized)
17 + {

s/spaces/tab-4spaces/

Space needed between identifier and ()

Vertical whitespace needed between saveMask () assignment and saveWc () assignment if there is no horizontal alignment.

3. It might make sense to unset the CWY | CWHeight / CWX | CWWidth bits on the keyboard-placed window if the window is grabbed later (eg GridWindow::grabNotify)

review: Needs Fixing

[Brandon Schaefer (brandontschaefer)]

If I don't do saveWc ().x /y/w/h then we run into the original problem. In window.cpp those fields are being overwritten with the x/y/w/h is for the semi maxed window. Im getting around this by updating the x/y/w/h with orig position ... if we are not a grab notify, otherwise the grab notify is getting the normal info from window.cpp when a grab happens.

I could be missing something, but thats how it seems to be working atm...also gnome-terminal is weird.

[Brandon Schaefer (brandontschaefer)]

> 2. Style Issues:
>
> 16 + else if (isGridVertMaximized)
> 17 + {
>
> s/spaces/tab-4spaces/
>
> Space needed between identifier and ()
>
> Vertical whitespace needed between saveMask () assignment and saveWc ()
> assignment if there is no horizontal alignment.
>

Fixed.

[MC Return (mc-return)]

Brandon, gnome-terminal is completely weird. It is not a good program for testing ;)

[MC Return (mc-return)]

Give me a few minutes to test this.

[MC Return (mc-return)]

Hmm -> this fixes the linked bug reliably, but unfortunately introduces a regression also.

Try this to trigger the regression:

1. Disable Grid->Resize Actions->"Snap Windows Back To Original Size"
2. Semi-maximize a window
3. Drag the window away from it's semi-maximized position

Make sure it does not change it's size. Unfortunately it does.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

> If I don't do saveWc ().x /y/w/h then we run into the original problem. In window.cpp those fields are being overwritten with the x/y/w/h is for the semi maxed window. Im getting around this by updating the x/y/w/h with orig position ... if we are not a grab notify, otherwise the grab notify is getting the normal info from window.cpp when a grab happens.
>

Indeed, so the relevant check is:

if (!(window->saveMask () & CWX))
{
    window->saveWc ().x = ...
    window->saveMask () |= CWX;
}

This just prevents us from messing with other saved positions that
there might be.

--
Sam Spilsbury

[MC Return (mc-return)]

Probably adding a && optionGetSnapbackWindows () to if (!window->grabbed ()) might fix the regression, but then this fix won't work if "Snapback Windows To Original Size" is disabled, so it would not be perfect either...

[Brandon Schaefer (brandontschaefer)]

@ MC

Thanks not my branches doing... when I comment out my changes it still does that...

@ Sam

Right, if I do it that way, the x value is getting overwritten when you switch to a full max window, and since CWX is already part of the mask, it doesn't change its value. So when you restore from a full max window it doesn't restore to its orig position :(

[Brandon Schaefer (brandontschaefer)]

s/thanks/thats, but thanks for reviewing :)

[MC Return (mc-return)]

Brandon, you are right. -> Urgh -> I could swear that this was already working in 0.9.10, but probably I just did not test these cases...

Sorry for blaming your branch for it, we can fix this later.

[Brandon Schaefer (brandontschaefer)]

MC No worries, its easy to do :), thanks for going through those options though. I had forgot to check those out :)

[MC Return (mc-return)]

Approve from me then, as the linked bug gets fixed by this branch reliably and I could not find any other problems and the one I found was already there before.

review: Approve

[MC Return (mc-return)]

One note, Brandon:
Maybe you could add some comments to the code, it would make it easier for the future, if we know what we are doing and why we are doing it.
Given the complexity of Grid, comments can be very helpful ;)

Another note:
I've added Sami to the reviewers as he is a Grid expert and has analyzed this part of the code extensively in the past, maybe he also has some comments regarding this and can also help with testing it (it works here).
More eyes == better code.

Note to Sami:
Thanks in advance !

[MC Return (mc-return)]

Another note:
I filed bug #1195969 about the non-working not-snapping-back-to-original-size for semi-maximized Grid windows.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3747
http://jenkins.qa.ubuntu.com/job/compiz-ci/227/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/268
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/40
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/40
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/40

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/227/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3748
http://jenkins.qa.ubuntu.com/job/compiz-ci/229/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/270/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/42
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/42
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/42

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/229/rebuild

review: Needs Fixing (continuous-integration)

[Sami Jaktholm (sjakthol)]

Seems complicated.

Here's a regression I noticed:
1) Move window to position 1.
2) Semi-maximize it vertically (Ctrl+Super+Left).
3) Restore it by clicking the window decoration.
4) Move it to position 2.
5) Maximize fully and restore:
 What happens: window has X and width of position 1, Y and height of position 2.
 What should happen: window has X, Y, width and height of position 2.
6) Move it to position n.
7) Maximize fully and restore
 What happens: window has X and width of position 1, Y and height of position n.
 What should happen: window has X, Y, width and height of position n.

If you semi-maximize the window horizontally, the result is Y and height of position 1, X and width of position n.

I can't reproduce the original bug with or without this though (the behavior in raring version is broken though but not the way the bug describes). Odd...

review: Needs Fixing

[Sami Jaktholm (sjakthol)]

Scratch most of that. Can't reproduce the issues I described anymore. Probably broke something myself :(

Only truth in that comment was: "I can't reproduce the original bug with or without this though (the behavior in raring version is broken but not the way the bug describes)."

Abstain for now...

review: Abstain

[Brandon Schaefer (brandontschaefer)]

Im also not sure why this merged...still needed a green light from Sam... he has a different way of doing it, but it wasn't working for me hmm

[MC Return (mc-return)]

Brandon, this never merged: http://bazaar.launchpad.net/~compiz-team/compiz/0.9.10/changes

I've changed the status back to "Needs review".

Sometimes Launchpad changes the status to "Merged" without reason -> happened to me several times in the past also...

[MC Return (mc-return)]

Another note:
I filed bug #1198140 about the "disabled-Grid->Resize Actions->"Snap Windows Back To Original Size"-does-not-work-with-grid-semi-maximized-windows"-bug.

[MC Return (mc-return)]

One note, Brandon:
Maybe you could add some comments to the code, it would make it easier for the
future, if we know what we are doing and why we are doing it ?
Given the complexity of Grid, comments can be very helpful ;)

Note to Sam:
For me this works and fixes the problem reliably, but of course I'll wait for your green light here...

[Brandon Schaefer (brandontschaefer)]

@MC

Sorry, been busy with some critical bugs in unity. Yes I can comment the code, but last time I talked with Sam he had a better approach. The only problem is it wasn't working for me :(. If you are able to confirm it fixes it for you that would be awesome :).

http://paste.ubuntu.com/5809468/

Ill get a chance to get back at this hopefully by tomorrow!

I would much prefer to get Sams code in and working because his bit is doing it at the right time :). Also it doesn't overwrite bits that it shouldn't, and my code has a chance to overwrite x/y/w/h position that could be set. Even though I've not seen a bug because of it, best to not overwrite data :)

Thanks again for keeping up on this and testing!

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3749
http://jenkins.qa.ubuntu.com/job/compiz-ci/257/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/298
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/70/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/70/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/70/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/257/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3750
http://jenkins.qa.ubuntu.com/job/compiz-ci/258/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/299
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/71/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/71/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/71/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/258/rebuild

review: Needs Fixing (continuous-integration)

[Sami Jaktholm (sjakthol)]

This doesn't work either: Grid a window to the left and click on the titlebar >> X and width are restored right away.

This happens because addWindowSizeChanges always calls restoreGeometry (CWX | CWWidth) if window is not horizontally maximized. If grid pushes CWX and CWWidth to saveMask when window is only vertically maximized, restoreGeometry actually restores X and width the first time addWindowSizeChanges is called.

The behavior is the same for Y and height of horizontally maximized windows.

Can't really still comment on the original bug though...

review: Needs Fixing

[Brandon Schaefer (brandontschaefer)]

Yeah... dam... I wonder if I should just go back to the fix I had...as I didn't run into any problem with, and this way Im running into bugs with gnome-terminal and the one you just pointed out!

Thanks for looking at the branch :)

[Brandon Schaefer (brandontschaefer)]

Im going to go regression hunting with my current fix, if anyone can find any let me know! The only problem I see is gnome-terminal doesn't return back to its orig position, but it seems gnome-terminal does some strange things... all other apps are working, and I haven't seen the face that I could be overwriting X/Y/W/H saved cords to cause any problems.

While I did like sames approach better, it seems to have some bugs, which I can also take a look at...

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3751
http://jenkins.qa.ubuntu.com/job/compiz-ci/259/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/300
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/72/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/72/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/72/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/259/rebuild

review: Needs Fixing (continuous-integration)

[Sami Jaktholm (sjakthol)]

I was finally able to reproduce the original bug and your solution seems to fix it indeed. However I'm still not sure whether saveMask needs to be changed. If the bug only happens when a maximized window is fully restored, saveMask should already have all required bits set to restore all values from saveWc.

I haven't noticed any regressions by this version and can't really think of anything other than some other bug messing originalSize or isGrid*Maximized up.

There's one thing I started to wonder while looking at the code.

As the bug you're fixing is only triggered when restoring fully maximized windows, why would either isGridHorzMaximized or isGridVertMaximized be true? It appears that if a window goes to one maximized state to another, the grid maximization state doesn't get correctly updated. stateChangeNotify only has code for handling cases in which either current or last maximize state is 0 - not when both new and old state contain a maximized state flag.

If isGridHorzMaximized or isGridVertMaximized were to be true when a window was actually restored, this would cause few issues:
1) originalSize won't be saved when the window is gridded the next time
2) validateResizeRequest will deny resize request

So this fix is relying on a broken behavior. Another solution would be to catch the maximization state change and update the values of saveWc at that moment. But that would be equally complicated and even more error-prone than the current proposal.

It seems that if you fix one corner case in grid few other pop up...

review: Approve

[MC Return (mc-return)]

No comments still, but I hope that we remember what we did here and why we did it... ;)
My opinion - Let's get this off the table. Another Approve from me.

review: Approve

[MC Return (mc-return)]

Sami, for most of the other Grid troubles we already have solutions. I just had no time to implement those yet and wanted this to land first also.
For example we use xwc in Grid twice without initialization (Coverity bugs already triaged).
Also we still have the wrong positioning of the mousepointer when wobbly is enabled, for which we engineered a hack or have analyzed other workarounds in our minds already.
The thing with the semi-maximized windows ignoring the do-not-restore-window-size setting is something that seems simple to fix as well.
The thing with gnome-terminal acting weird has to be analyzed, but my idea is that it *could* have to do with gnome-terminal character-size settings or with uninitialized stuff, because it happens *sometimes* only.
We should find out if it happens with xterm also to exclude that it is gnome-terminal specific for example...

One thing to note also here is that a Grid-maximize via "Put Center" is *not* the same as a Compiz+Unity+globalmenu Maximize as the window border will still be intact when former is used, which could also be responsible for strange behaviour (needs investigation still, just an idea)...

Everything that just happens "sometimes" needs special attention as this is something that should never happen with good code.

Overall I think that Grid currently acts better than ever before, has more features and the fact that Compiz finally learned how to snap off horizontally maximized windows is a big step forward. We are on a good way :)

Just my 2 cents.
Just my 5 cent.

[Sam Spilsbury (smspillaz)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-autolanding/121/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-autolanding/33/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-autolanding/33/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-autolanding/33/console
    SUCCESS: http://s-jenkins:8080/job/generic-cleanup-mbs/815

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'plugins/grid/src/grid.cpp'
--- plugins/grid/src/grid.cpp	2013-06-29 07:07:12 +0000
+++ plugins/grid/src/grid.cpp	2013-07-12 19:06:25 +0000
@@ -1011,6 +1011,22 @@
     {
 	lastTarget = GridUnknown;
 
+	if (!window->grabbed ())
+	{
+	    if (isGridHorzMaximized)
+	    {
+ 		window->saveMask ()      |= CWY | CWHeight;
+  		window->saveWc ().y      = originalSize.y ();
+  		window->saveWc ().height = originalSize.height ();
+	    }
+	    else if (isGridVertMaximized)
+	    {
+  		window->saveMask ()     |= CWX | CWWidth;
+  		window->saveWc ().x     = originalSize.x ();
+  		window->saveWc ().width = originalSize.width ();
+	    }
+	}
+
 	if ((isGridHorzMaximized &&
 	     (lastState & MAXIMIZE_STATE) == CompWindowStateMaximizedHorzMask) ||
 	    (isGridVertMaximized &&