Merge lp://staging/~brandontschaefer/compiz/fix-lp.892012 into lp://staging/compiz/0.9.10

Could you post a way how to reproduce the problem ? I cannot reproduce it in trunk.

FAILED: Continuous integration, rev:3738
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~brandontschaefer/compiz/fix-lp.892012/+merge/168519/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/compiz-ci/197/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/238
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/10

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

@ MC

Yeah. Soo:

1) Open a terminal make sure its a normal sized window (no maxed). Take not of the windows X position.
2) Press Ctrl+Super+Right to do a right semi max. The terminal should now be on the right side of the screen.
3) Press Ctrl+Super+Up to maximize the window. The terminal should now be maximized.
4) Press the restore button (or Ctrl+Super+Down). to restore the window, now the X position has been changed to where it was during the semi max position.

If that doesn't make sense let me know and I can make a video :)!

FAILED: Continuous integration, rev:3739
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~brandontschaefer/compiz/fix-lp.892012/+merge/168519/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/compiz-ci/198/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/239/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/11
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/11
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/11

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

> @ MC
>
> Yeah. Soo:
>
> 1) Open a terminal make sure its a normal sized window (no maxed). Take not of
> the windows X position.
> 2) Press Ctrl+Super+Right to do a right semi max. The terminal should now be
> on the right side of the screen.
> 3) Press Ctrl+Super+Up to maximize the window. The terminal should now be
> maximized.
> 4) Press the restore button (or Ctrl+Super+Down). to restore the window, now
> the X position has been changed to where it was during the semi max position.
>
> If that doesn't make sense let me know and I can make a video :)!

Ah -> yes, now I can reproduce it :)
I have to use the "General Options/Key bindings/Maximize window" shortcut...

Review:
Please initialize the new class member unsigned int lastState in the PrivateWindow::PrivateWindow () : constructor

You can remove the brackets here:
39 + if (state & CompWindowStateMaximizedHorzMask)
40 + {
41 + saveGeometry (CWY | CWHeight);
42 + }
43 + else
44 + {
45 + saveGeometry (CWX | CWY | CWWidth | CWHeight);
46 + }

here:
56 + if (state & CompWindowStateMaximizedHorzMask)
57 + {
58 + mask |= restoreGeometry (xwc, CWY | CWHeight);
59 + }
60 + else
61 + {
62 + mask |= restoreGeometry (xwc, CWX | CWY | CWWidth | CWHeight);
63 + }

here:

69 + if (!(lastState & CompWindowStateMaximizedVertMask))
70 + {
71 + saveGeometry (CWX | CWWidth);
72 + }

and here:

82 + if ((!(state & CompWindowStateMaximizedVertMask)) &&
83 + !(lastState & CompWindowStateMaximizedVertMask))
84 + {
85 + mask |= restoreGeometry (xwc, CWX | CWWidth);
86 + }

(just style issues)

Tip: If you have troubles woth the X11 Compiz indentation, please try Qt Creator with this added config file, you won't regret trying it out ;)
https://code.launchpad.net/~mc-return/compiz/compiz.merge-added-qtcreator-config-file-for-contributors/+merge/166939

*with

I was having indent problems cause vim was stripping the tabs out :(. Plus I was to lazy to switch my vimrc info...now its all fixed.. :).

As far as the brackets, if Im doing an if/else I prefer to keep them on, and if I do a cond statement that has to go on a new line I also like the bracket...So i've just kept the brackets :).

Right, forgot to init it in the ctor for private window :)

> I was having indent problems cause vim was stripping the tabs out :(. Plus I
> was to lazy to switch my vimrc info...now its all fixed.. :).
>
I was aware that I probably won't get you to leave vim, but it was nevertheless worth a try ;)

FAILED: Continuous integration, rev:3740
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~brandontschaefer/compiz/fix-lp.892012/+merge/168519/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/compiz-ci/199/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/240/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/12

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

Brandon, could you please initialize the new class member unsigned int lastState in the PrivateWindow::PrivateWindow () : constructor, so this can land ?

Opps, sorry forgot to push that change!

Thanks. +1.
LGTM, but waiting for Sam for the final green light.

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

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

(I have a few thoughts on this one, but I'm pretty busy at the moment. I'll get to it in the next few days).

Cool, and thanks...I wasn't 100% sure if this was the correct way to fix this but it seems to be the only place the x/y/w/h is saved which is whats causing the bug. Also take your time! (Get that studying done :)

Okay, I've had a more detailed look into this.

Its definitely on the right track, but not quite there yet. Ideally, it'd need to handle the same type of situation for horizontally maximized windows, and there's a much nicer way to handle the previously-fully-maximized-now-partly-maximized case.

I've done up a quick draft that seems to work in all those cases here:

http://paste.ubuntu.com/5786206/

Some things to notice:

 1. We can detect if we were previously maximized if lastState & MAXIMIZE_STATE == MAXIMIZE_STATE (eg, both CompWindowStateMaximizedHorz and CompWindowStateMaximizedVert were set)
 2. We want to save the Y and Height components for a vertical maximize in the case that we're already horizontally maximized (which this branch does already), and then all the components on the initial vertical maximize (ditto).
 3. Same thing applies inter-alia for horizontal maximization
 4. The above logic would catch the case where we move from a fully maximized window to a partly maximized window. In that case, we don't want to save all the components, so we need to detect where we were previously fully maximized and ignore saving in that case
 5. Where components are already saved, saveGeometry will not overwrite them.

Furthermore related things:

 a. addWindowSizeChanges is getting pretty big. I think it would be good to split out the two if/else brackets for the vertical maximize and horizontal maximize cases into separate nonmember nonfriend functions. Since they call saveGeometry and restoreGeometry, what might make sense is to take "mask", "xwc" and a new variable "nextSaveMask" by reference and just update them (eg, updating nextSaveMask in place of saveGeometry). Then when the function returns call saveGeometry with nextSaveMask first and then restoreGeometry. i.e:

    nextSaveMask = 0;
    nextRestoreMask = 0;

    mask =| cw::applyVerticalMaxmization (xwc, nextSaveMask, nextRestoreMask);
    saveGeometry (nextSaveMask);
    mask |= restoreGeometry (nextRestoreMask);

    nextSaveMask = 0;
    nextRestoreMask = 0;

    mask =| cw::applyHorizontalMaxmization (xwc, nextSaveMask, nextRestoreMask);
    saveGeometry (nextSaveMask);
    mask |= restoreGeometry (nextRestoreMask);

 b. While I initially thought that acceptance-testing might be appropriate here, in my testing of this branch and my own adjustments, I remembered there's a bug that makes that more annoying than its worth. Basically the window moves by a very slight offset depending on the size difference between decorations upon changing state. Its something we'll need to fix eventually but not right now.
 c. As such, it would make more sense to take those two split-out functions and provide unit tests for them. The existing unit testing code inside of compiz would provide some insight on how to do that.

You know, I didn't know about horz max only it makes sense though... A full screen max is a vert + horz max...haha. Yes, I didn't take into account horz only, thanks a lot for the comment and suggestions! Ill take a look into fixing this up :)

Hmm it seems to overwrite them :). For example with the draft you've made:

If out last state was not maxed, and we go into a V Max we save X,Y,W,H. This should be our orig position.

Then we go to a full max (V + H), in the Vert if statement we re-save Y, H (since are state is also a Horz max), and in the Horz Max we re-save the X, W causing the new position to be save where it was in the Vert Max position ie. the bug :). Sooo I suppose I should look into why saving is overwriting the window cords?

Also some strange thing be happening with your changes, when I move to a vert maxed the window slowly decreases its width? Its actually pretty cool looking...but something must be off...

On Mon, Jun 24, 2013 at 5:56 PM, Brandon Schaefer
<email address hidden> wrote:
> Hmm it seems to overwrite them :). For example with the draft you've made:
>
> If out last state was not maxed, and we go into a V Max we save X,Y,W,H. This should be our orig position.
>
> Then we go to a full max (V + H), in the Vert if statement we re-save Y, H (since are state is also a Horz max), and in the Horz Max we re-save the X, W causing the new position to be save where it was in the Vert Max position ie. the bug :). Sooo I suppose I should look into why saving is overwriting the window cords?

As I mentioned in (5) PrivateWindow::saveGeometry will only save
positions that have not yet been saved. This is because we only save
bits which are in mask & ~saveMask. Any bits in saveMask already will
be changed to 0 by ~. So if you call saveGeometry with CWX | CWWidth,
it will only overwrite them they were not already added to saveMask.

>
> Also some strange thing be happening with your changes, when I move to a vert maxed the window slowly decreases its width? Its actually pretty cool looking...but something must be off..

Indeed, as I mentioned in (b), there is a different bug which is
causing an incorrect offset move. Hence the reason why I'm getting
some acceptance tests around the decor plugin so that we can fix that.

Yes, you are correct :). It turns out when doing a vert max the X pos was being re-saved 3-4 times and the last time is where the vert max window ends up...causing the window to save the X at the wrong pos... Strange, but something Ill have to look into...possibly Im forcing to many updates through? (Ill have to figure out what is causing all the updates...)

Also this is what is causing the width to constly resize... http://paste.ubuntu.com/5800016/

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

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