Merge lp://staging/~smspillaz/compiz-core/compiz-core.work_923683 into lp://staging/compiz-core/0.9.8
- compiz-core.work_923683
- Merge into 0.9.8
Status: | Merged |
---|---|
Merged at revision: | 3110 |
Proposed branch: | lp://staging/~smspillaz/compiz-core/compiz-core.work_923683 |
Merge into: | lp://staging/compiz-core/0.9.8 |
Prerequisite: | lp://staging/~smspillaz/compiz-core/compiz-core.fix_969101 |
Diff against target: |
2394 lines (+509/-779) 19 files modified
plugins/composite/src/window.cpp (+15/-35) plugins/decor/src/decor.cpp (+66/-53) plugins/decor/src/decor.h (+2/-3) plugins/move/src/move.cpp (+2/-4) plugins/opengl/src/paint.cpp (+11/-18) plugins/opengl/src/privates.h (+7/-1) plugins/opengl/src/window.cpp (+39/-17) plugins/place/src/constrain-to-workarea/src/constrain-to-workarea.cpp (+4/-6) plugins/place/src/place.cpp (+19/-24) plugins/resize/src/resize.cpp (+1/-11) plugins/resize/src/resize.h (+0/-2) plugins/wobbly/src/wobbly.cpp (+5/-4) src/event.cpp (+1/-1) src/privatewindow.h (+7/-8) src/screen.cpp (+4/-2) src/window.cpp (+270/-543) src/window/geometry/include/core/windowgeometry.h (+6/-0) src/window/geometry/tests/window-geometry/src/test-window-geometry.cpp (+10/-0) src/windowgeometry.cpp (+40/-47) |
To merge this branch: | bzr merge lp://staging/~smspillaz/compiz-core/compiz-core.work_923683 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Daniel van Vugt | Approve | ||
Alan Griffiths | Approve | ||
Sam Spilsbury | Pending | ||
Review via email:
|
This proposal supersedes a proposal from 2012-04-19.
Commit message
Description of the change
Resubmitted now that lp:~smspillaz/compiz-core/compiz-core.fix_969108.2 and lp:~smspillaz/compiz-core/compiz-core.fix_969101 are active
Always use the asynchronous codepaths in core. This commit changes the following:
* CompWindow::move is now just a wrapper around CompWindow:
* CompWindow:
* moveNotify and resizeNotify are only ever called pre-request in the case of
managed windows (SubstructureRe
the right place) and post ConfigureNotify for override redirect windows
* resizeNotify is always called post ConfigureNotify regardless of whether
or not the window is managed or not
* various const correctness where it made sense
* cleaned up the whole geometry.width () + geometry.border () * 2 nonsense
* removed priv->width and priv->height , which was just redundant data used for
window shading and honestly, is a ticking time bomb for future maintainers.
* CompWindow:
* Removed the majority of PrivateWindow:
of redundant code which PrivateWindow:
there was to send synthetic ConfigureNotify events to ourselves to notify windows
moving within frame windows that don't move themselves. Its safer to use
reconfigureX
We should look into removing the other update* functions.
Testing plan?
I would imagine here that the PendingConfigur

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
824 -
825 - if (!pendingConfig
826 - {
827 - /* Tell plugins its ok to start doing stupid things again but
828 - * obviously FIXME */
829 - CompOption::Vector options;
830 - CompOption::Value v;
831 -
832 - options.push_back (CompOption ("window", CompOption:
833 - v.set ((int) id);
834 - options.back ().set (v);
835 - options.push_back (CompOption ("active", CompOption:
836 - v.set ((int) 0);
837 - options.back ().set (v);
838 -
839 - /* Notify other plugins that it is unsafe to change geometry or serverGeometry
840 - * FIXME: That API should not be accessible to plugins, this is a hack to avoid
841 - * breaking ABI */
842 -
843 - screen-
844 - }
DIE
924 -bool
925 -PrivateWindow:
926 -{
927 - if (pendingConfigu
928 - {
929 - /* FIXME: This is a hack to avoid performance regressions
930 - * and must be removed in 0.9.6 */
931 - compLogMessage ("core", CompLogLevelWarn, "failed to receive ConfigureNotify event on 0x%x\n",
932 - id);
933 - pendingConfigur
934 - pendingConfigur
935 - }
936 -
937 - return false;
938 -}
939 -
940 void
941 compiz:
942 {
943 @@ -2454,21 +2148,6 @@
944 mValueMask (valueMask),
945 mXwc (*xwc)
946 {
947 - CompOption::Vector options;
948 - CompOption::Value v;
949 -
950 - options.push_back (CompOption ("window", CompOption:
951 - v.set ((int) w);
952 - options.back ().set (v);
953 - options.push_back (CompOption ("active", CompOption:
954 - v.set ((int) 1);
955 - options.back ().set (v);
956 -
957 - /* Notify other plugins that it is unsafe to change geometry or serverGeometry
958 - * FIXME: That API should not be accessible to plugins, this is a hack to avoid
959 - * breaking ABI */
960 -
961 - screen-
962 }
DIE

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
GOOD IN THEORY:
* Most of the description sounds like steps in the right direction.
* Simplified: Replacing 559 lines of code with 247.
UNSURE ABOUT THE THEORY:
* "composite and opengl now use the geometry last sent to the server"
This should give maximum performance, sure, but what if (for some reason) the server rejects the configure request as being somehow invalid? Would you ever encounter that, like moving a window to an invalid location?
For stability and correctness (which should be the primary goal right now) don't we want "composite and opengl now use the geometry last received from the server"?
* Caveat #1 is not really a caveat. It's the right (intended) design. But if you want to see a (buggy) example of how to solve the chattiness look at the timer code here:
https:/
This was slightly buggy though as it broke keyboatrd-initiated moves (incorrect pointer warp). Simple to fix, probably.
* Still no XFlush's to ensure XConfigureWindow happens immediately. Or are there adequate XSyncs already?
BAD IN THEORY:
* The diff is too big for a mere mortal (who hasn't worked on all the code before) to fully load into their mental interpreter and evaluate its theoretical correctness. Please read that sentence again; it's important.
GOOD IN PRACTICE:
* Window movement is smooth and predictable, but not really better than in oneiric.
BAD IN PRACTICE:
* The bug introduced is worse than the bug being fixed; Window contents offset from the frame/decorations whenever a window is created or maximized.
* All that code, but we still haven't reduced the lag below oneiric-levels. Still not as good as Gnome Shell.
CONCLUSION:
The theory is mostly good but there is at least one serious bug that needs fixing. We probably shouldn't risk 0.9.7.0 any more with large changes like this. We should aim to get this code into a later release when it's more stable, not 0.9.7.0.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> GOOD IN THEORY:
>
> * Most of the description sounds like steps in the right direction.
>
> * Simplified: Replacing 559 lines of code with 247.
>
>
> UNSURE ABOUT THE THEORY:
>
> * "composite and opengl now use the geometry last sent to the server"
> This should give maximum performance, sure, but what if (for some reason) the
> server rejects the configure request as being somehow invalid? Would you ever
> encounter that, like moving a window to an invalid location?
> For stability and correctness (which should be the primary goal right now)
> don't we want "composite and opengl now use the geometry last received from
> the server"?
See http://
Since we have SubstructureRed
The only error cases I can think of is where you ask for a window to be of a size < 0 or > MAXSHORT. Ideally we should check for that case.
>
> * Caveat #1 is not really a caveat. It's the right (intended) design. But if
> you want to see a (buggy) example of how to solve the chattiness look at the
> timer code here:
> https:/
> This was slightly buggy though as it broke keyboatrd-initiated moves
> (incorrect pointer warp). Simple to fix, probably.
>
There are some optimizations that can be done here - for example, if a window is grabbed, you can withold sending positions to the server until it is ungrabbed, or there is some other reason why you would want to do that. When that happens you can send it all at once.
> * Still no XFlush's to ensure XConfigureWindow happens immediately. Or are
> there adequate XSyncs already?
The GLib mainloop work introduced an XFlush as soon as we have finished dispatching the X event source.
>
>
> BAD IN THEORY:
>
> * The diff is too big for a mere mortal (who hasn't worked on all the code
> before) to fully load into their mental interpreter and evaluate its
> theoretical correctness. Please read that sentence again; it's important.
Indeed. The good thing is that its mostly all deletion and consolidation.
>
>
> GOOD IN PRACTICE:
>
> * Window movement is smooth and predictable, but not really better than in
> oneiric.
>
>
> BAD IN PRACTICE:
>
> * The bug introduced is worse than the bug being fixed; Window contents offset
> from the frame/decorations whenever a window is created or maximized.
Alright. I haven't seen this yet, but I have an idea of what might cause it. Will have a look into it when I get some time.
>
> * All that code, but we still haven't reduced the lag below oneiric-levels.
> Still not as good as Gnome Shell.
This will be fixed once the chattyness goes away. (as above). Watch your X.org CPU usage as you move windows - it skyrockets because at the moment we flood it with requests.
>
>
> CONCLUSION:
>
> The theory is mostly good but there is at least one serious bug that needs
> fixing. We probably shouldn't risk 0.9.7.0 any more with large changes like
> this. We should aim to get this code into a later r...

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
OK, I'm still wading through the changes. But I feel like a rant...
1307 CompSize
1308 CompWindow::size () const
1309 {
1310 - return CompSize (priv->width + priv->geometry.
1311 - priv->height + priv->geometry.
1312 + return CompSize (priv->
1313 + priv->geometry.
1314 }
As a change this doesn't look too bad - but it ignores a horrid design!
1. Chained operations like "priv->
That is "priv->width ()" would reflect less coupling.
2. You're tracing the path "priv->geometry..." many times, which suggests that the logic belongs in "geometry".
"return priv->geometry.size ()" or "return CompSize(
So, assuming (because borders may be optional?) that there's an unambiguous mapping from CompWindow:
a. Add a constructor to CompSize: "explicit CompSize(
b. Add an inline method "CompSize PrivateWindow::size () const { return CompSize(
c. Rewrite the above as "CompSize CompWindow::size () const { "return priv->size ()"; }"

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
264 - /* been shaded */
265 + /* been shaded
266 if (w->priv->height == 0)
267 {
268 if (w->id () == priv->activeWindow)
269 w->moveInputFocusTo ();
270 - }
271 + }*/
Comments are not for storing old versions of the code - that's what we use bzr for. ;)

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
There's a lot of "<blah>.width () + <blah>.border () * 2" and "<blah>.height () + <blah>.border () * 2" around. Surely CompWindow:
I'm tempted by
template<Typename T>
inline int heightWithBorders(T const& blah) { return blah.height () + blah.border () * 2; }

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> OK, I'm still wading through the changes. But I feel like a rant...
>
> 1307 CompSize
> 1308 CompWindow::size () const
> 1309 {
> 1310 - return CompSize (priv->width + priv->geometry.
> 1311 - priv->height + priv->geometry.
> 1312 + return CompSize (priv->
> * 2,
> 1313 + priv->geometry.
> 1314 }
>
> As a change this doesn't look too bad - but it ignores a horrid design!
>
> 1. Chained operations like "priv->
> of the details of "priv".
> That is "priv->width ()" would reflect less coupling.
Probably, although priv is just a private member with implementation details, I don't really see this as a large design problem.
>
> 2. You're tracing the path "priv->geometry..." many times, which suggests that
> the logic belongs in "geometry".
> "return priv->geometry.size ()" or "return CompSize(
> reflect better coherence.
>
> So, assuming (because borders may be optional?) that there's an unambiguous
> mapping from CompWindow:
>
> a. Add a constructor to CompSize: "explicit CompSize(
> const& g) : mWidth(g.width () + g.border () * 2) ..."
> b. Add an inline method "CompSize PrivateWindow::size () const { return
> CompSize(
> c. Rewrite the above as "CompSize CompWindow::size () const { "return
> priv->size ()"; }"
+1 for all three

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> 264 - /* been shaded */
> 265 + /* been shaded
> 266 if (w->priv->height == 0)
> 267 {
> 268 if (w->id () == priv->activeWindow)
> 269 w->moveInputFocusTo ();
> 270 - }
> 271 + }*/
>
> Comments are not for storing old versions of the code - that's what we use bzr
> for. ;)
Indeed, this is a small portion of the code and I'm not yet sure what to do with it. When I get around to revisiting this section (when I actually get time to look at this again, wanted to get it up for review early) I'll see what can be done.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> There's a lot of "<blah>.width () + <blah>.border () * 2" and "<blah>.height
> () + <blah>.border () * 2" around. Surely CompWindow:
> "widthWithBorders" method - or maybe a free function?
>
> I'm tempted by
>
> template<Typename T>
> inline int heightWithBorders(T const& blah) { return blah.height () +
> blah.border () * 2; }
I'm not really sure ?
The complexity here comes from a legacy part of the X Server coming into play - a window could have fixed dimentions, but also specify a "border" which would be exclusive of the fixed dimentions, so you'd have to take this into account for all positioning operations (Xterm comes to mind here).
I definitely agree that geom.width () + geom.border () * 2 feels fragile and indeed, that has tripped me up many times before. Maybe a default parameter with a bitflag makes sense here, eg IncludeBorder , IncludeBorderFirst , IncludeBorderSecond (as it is on both sides)

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
I'll note that the above isn't *really* all that relevant to this merge though, but good things to keep in mind in any case. I'd rather not see the diff get any better except for adding tests.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Please resubmit for target branch lp:compiz-core (0.9.7)

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
A lot of the above is about making the code better (which the proposal doesn't attempt). However, in a previous version I pointed out that:
> 264 - /* been shaded */
> 265 + /* been shaded
> 266 if (w->priv->height == 0)
> 267 {
> 268 if (w->id () == priv->activeWindow)
> 269 w->moveInputFocusTo ();
> 270 - }
> 271 + }*/
>
> Comments are not for storing old versions of the code - that's what we use bzr for. ;)
I still think that needs fixing.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Alright, I've updated this so that there's no distorted windows on decoration size change. Was (ironically) a race condition.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
For the sake of not making this diff any bigger, I'm not going to introduce unit tests here.
As for testing this I'd say the following is appropriate:
* Add testcase for PendingConfigur
* Add testcase for rectsToRegion (serverGeometry should really be DI'd here)
Notes for the future:
* As alan has said - the whole priv->geometry ().width () + priv->geometry ().border () * 2 is /really/ awkward and error prone, but we need it to support windows like xterm that still use this (stupid, legacy) attribute on their windows. I would say that the role of compiz:
- it should also encapsulate priv->region and really, geometry::x and friends should be made with reference to that. Ideally we'll store two regions, one with borders and one without. That sucks, since its a little memory hungry, but at least the cost is only really born whenever those regions need to be updated (::translate on a region with one rectangle is basically free, slightly more complicated on regions with multiple rects (eg, shaped window). The default should be to return the size /without/ borders, and have a default-parameter enum to get the size with borders.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Unfortunately still fails basic testing. The same bug remains;
* The bug introduced is worse than the bug being fixed; Window contents offset from the frame/decorations whenever a window is created or maximized.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Also... I would have thought/hoped that a simplified/stable solution would eliminate "serverGeometry" completely.
/**
* Geometry retrieved from the
* last ConfigureNotify event received
*/
Geometry & geometry () const;
/**
* Geometry last sent to the server
*/
Geometry & serverGeometry () const;
I understand why we have, and why we might want, serverGeometry. However it is an optimization which only makes sense to attempt if the code is stable and bug-free to begin with.
While serious bugs remain, I think the first goal should be to simplify the logic down to just using "geometry" and remove or stub "serverGeometry".

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> Also... I would have thought/hoped that a simplified/stable solution would
> eliminate "serverGeometry" completely.
>
> /**
> * Geometry retrieved from the
> * last ConfigureNotify event received
> */
> Geometry & geometry () const;
>
> /**
> * Geometry last sent to the server
> */
> Geometry & serverGeometry () const;
>
> I understand why we have, and why we might want, serverGeometry. However it is
> an optimization which only makes sense to attempt if the code is stable and
> bug-free to begin with.
>
> While serious bugs remain, I think the first goal should be to simplify the
> logic down to just using "geometry" and remove or stub "serverGeometry".
There are some usescases for when we need to know what the last /received/ geometry from the server is. That's why the whole geometry/

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Also, I'm not seeing this offset problem - could you give me some steps to reproduce it ?

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> > Also... I would have thought/hoped that a simplified/stable solution would
> > eliminate "serverGeometry" completely.
> >
> > /**
> > * Geometry retrieved from the
> > * last ConfigureNotify event received
> > */
> > Geometry & geometry () const;
> >
> > /**
> > * Geometry last sent to the server
> > */
> > Geometry & serverGeometry () const;
> >
> > I understand why we have, and why we might want, serverGeometry. However it
> is
> > an optimization which only makes sense to attempt if the code is stable and
> > bug-free to begin with.
> >
> > While serious bugs remain, I think the first goal should be to simplify the
> > logic down to just using "geometry" and remove or stub "serverGeometry".
>
>
> There are some usescases for when we need to know what the last /received/
> geometry from the server is. That's why the whole geometry/
> thing exists.
Hmm, we could make it so that serverGeometry is returned for managed windows and geometry is returned for override redirect windows on the public API. That of course means that we have to break the public API and update all the plugins.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
The practical problem:
Just start compiz --replace composite opengl move resize decor
and gtk-window-
Then every window will have its contents horizontally shifted around 15 pixels from the correct location relative to its decorations. The shift occurs whenever a window is created, maximized or restored. The shift goes away (corrects itself) when you start moving the window. On a positive note, the lag *appears* totally fixed and dragging windows is performing as well as Gnome Shell now.
The theoretical problem:
You can ignore my comments about geometry/

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Screenshot of the bug introduced by this branch:
https:/

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Alright, I'll have another look into it ? (Not seeing it here :( )

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
And it gets worse:
* The usual lag has come back. I have no idea how or why it was fixed when I ran it the first time.
* When dragging windows around, the area of desktop recently exposed flashes white.
So there is no improvement in performance and 2 serious regressions introduced :(

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
//XSynchronize (dpy, TRUE);
:) Haven't merged your other branch

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Yikes. That would be another regression to be missing:
src/screen.cpp: XSynchronize (dpy, synchronousX ? True : False);

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Perhaps remember to pull from trunk more often.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
where trunk == lp:compiz-core

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Why doesn't the removal of XSynchronize show up in the below diff? It's obvious if I merge the branch myself. Maybe LaunchPad needs a kick to update the below diff?

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I think the horizontal shift bug is coming from upstream lp:compiz-core, and is exposed moreso by this branch. I can reproduce the same kind of horizontal shift jitter using just lp:compiz-core and resizing a window rapidly.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
> The default should be to return the size /without/ borders,
> and have a default-parameter enum to get the size with borders.
There are very few designs where default parameters are better than multiple functions. This isn't one.
auto sizeWithoutBorders = geometry.size();
auto sizeWithBorders = geometry.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
OK, the horizontal shifting bug does not seem to be caused by this branch. Just made worse by this branch.
Sam, please review that nasty issue first: bug 928173.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
This branch will incidentally help with a lot of the resizing issues, so I think it should be merged first.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I would prefer not to approve this branch so long as the offset/shift bug is as bad as it is. If absolutely necessary, we could release without a fix for bug 928173 because it is hidden by the default Ubuntu config. But introducing this branch makes the bug unacceptably worse, as the screenshot showed.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
To clarify, "unacceptably worse" means that the bug will no longer be hidden in Ubuntu and will occur on every new window that opens. At least on my machine (and presumably others).

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
.... I'm still not even seeing this issue.
I can look into resizing tonight if you really think that this is a blocker.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I think I may have confused the situation by suggesting the regression introduced by this branch is bug 928173. The symptoms are actually slightly different. It's only a theory that it's the same bug, because the size of the horizontal offset looks similar.
The bug I see with this branch would be worthy of a new bug report (a critical regression) that I don't want to log. And we won't have to log that bug so long as it's fixed before this branch is merged.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
On Wed, 8 Feb 2012, Daniel van Vugt wrote:
> I think I may have confused the situation by suggesting the regression introduced by this branch is bug 928173. The symptoms are actually slightly different. It's only a theory that it's the same bug, because the size of the horizontal offset looks similar.
>
> The bug I see with this branch would be worthy of a new bug report (a critical regression) that I don't want to log. And we won't have to log that bug so long as it's fixed before this branch is merged.
> --
> https:/
> You are the owner of lp:~smspillaz/compiz-core/compiz-core.work_923683.
>
Alright.
I am looking into the decor plugin for a way to resolve this.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Despite the success of the fix for #928173: lp:~smspillaz/compiz-core/compiz-core.decor_928173
the offset bug introduced by this branch persists.
This confirms that the horizontal offset/shift problems introduced (or exposed) by this branch are certainly not the same as bug 928173.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
I still can't even observe this problem that you're referring to :(
Can you specify, exactly what windows this occurs on and /exact/ steps to reproduce it ?

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Steps to reproduce the issue:
1. Start with an exact clone of lp:compiz-core
2. Merge in lp:~smspillaz/compiz-core/compiz-core.work_923683
3. Build and install it.
4. Run compiz --replace composite opengl move resize decor
5. Run gtk-window-
Now every window will be corrupted. Not just the existing ones, but any new ones you open too. The corruption persists during window resizing, but vanishes as soon as you move a window.
Here is a new screenshot:
https:/

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Is the window itself shifted (eg are the input regions correctly lined up) or is it just the image that is shifted ?

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
The window itself is shifted. The input regions correctly line up with the image still.
However, it looks like the damage events are not shifted. This causes some redraw problems.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Can you post the xwininfo -all of the window, xwininfo -all -id of the parent and xwininfo -all -parent of the parent of that ?

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Attachments sent via email. This is interesting though;
$ diff correct.xwininfo shifted.xwininfo
11c11
< 0x2c03ca7 (has no name): () 1x1+-1+-1 +45+69
---
> 0x2c03ca7 (has no name): () 1x1+-1+-1 +34+69
13c13
< Absolute upper-left X: 46
---
> Absolute upper-left X: 35
31,32c31,32
< Corners: +46+70 -1232+70 -1232-720 +46-720
< -geometry 80x24+35+41
---
> Corners: +35+70 -1243+70 -1243-720 +35-720
> -geometry 80x24+34+41
It looks like geometry/

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
And why do I have so many 1x1 pixel windows?! I swear occasionally I see them on screen too.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
On Sun, 12 Feb 2012, Daniel van Vugt wrote:
> And why do I have so many 1x1 pixel windows?!
Applications use them as a means of doing IPC. Gtk+ is a serial offender
here.
> I swear occasionally I see them on screen too.
Under what circumstances?
> --
> https:/
> You are the owner of lp:~smspillaz/compiz-core/compiz-core.work_923683.
>

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
On Sun, 12 Feb 2012, Daniel van Vugt wrote:
The frame window is in the wrong position. This means that serverGeometry
isn't the last thing sent to the server. serverGeometry is still writable
in this branch, so it could be $evilplugin doing the wrong thing here.
I don't want to spend any more time on this until wednesday. So I'll pick
it up then.
(compiz run with --debug will have some logs which can confirm this btw)
Incidentally, you don't see any of those "unmatched ConfigureNotify"
warnings do you? They're all gone here but they indicate the first sign of
trouble.
> Attachments sent via email. This is interesting though;
>
> $ diff correct.xwininfo shifted.xwininfo
> 11c11
> < 0x2c03ca7 (has no name): () 1x1+-1+-1 +45+69
> ---
>> 0x2c03ca7 (has no name): () 1x1+-1+-1 +34+69
> 13c13
> < Absolute upper-left X: 46
> ---
>> Absolute upper-left X: 35
> 31,32c31,32
> < Corners: +46+70 -1232+70 -1232-720 +46-720
> < -geometry 80x24+35+41
> ---
>> Corners: +35+70 -1243+70 -1243-720 +35-720
>> -geometry 80x24+34+41
>
> It looks like geometry/
> --
> https:/
> You are the owner of lp:~smspillaz/compiz-core/compiz-core.work_923683.
>

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
It's random and very rare. But sometimes I see 1-pixel windows (with shadow). Can't ever seem to get xwininfo for them. Sometimes I also get very large white anonymous windows blocking my view. But it's all very hard to reproduce. And off-topic.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Correct. I did stop getting "Warn: failed to receive ConfigureNotify event on ..." when using this branch.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Well, you should stop getting "failed to receive" since I've removed the timeout which checks for unmatched events :) (A necessary hack coming up to the oneiric release).
I'm more interested in warnings that say "unmatched ConfigureNotify"

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Could you have a look again to see if the issue is still occurring for you ? I've just re-synced with trunk, so it may be fixed.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
OK, I found the issue. Its actually a bug that's been in the code for quite some time which would cause the client to not move within the frame when the frame was updated until the client position itself was updated.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
There's still one more issue here though to do with override redirect windows not getting their paint regions updated, so that needs to be looked into as well. And tests.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
^ Those fix the offset issue confirmed here (was finally able to reproduce it with qtcreator)

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
1. Confirmed the shifted window regression is now fixed. Seems to work without any obvious bugs now.
2. I'm not entirely sure about replacing geometry with serverGeometry. I thought it would be a good idea to do the opposite. There would be a slight performance penalty, but using just "geometry" would guarantee that compiz always agrees with the actual server geometry, instead of guessing/assuming that serverGeometry is accepted by the server (which does not always seem to be true, hence bug 886192).
3. There are two instances of double semicolons ";;" in this proposal.
4. I suspect this proposal will conflict just slightly with the proposal for bug 940139. But it should be minor.
5. NEW REGRESSION:
375 - XSynchronize (dpy, synchronousX ? True : False);
376 +// priv->connection = XGetXCBConnection (priv->dpy);
6. Why always two spaces before "* 2" ?

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> 2. I'm not entirely sure about replacing geometry with serverGeometry. I
> thought it would be a good idea to do the opposite. There would be a slight
> performance penalty, but using just "geometry" would guarantee that compiz
> always agrees with the actual server geometry, instead of guessing/assuming
> that serverGeometry is accepted by the server (which does not always seem to
> be true, hence bug 886192).
>
serverGeometry will always be "accepted" by the server as long as the window is managed and the requested window geometry will not generate a BadValue error (eg, 0 < 1 < MAXINT) (eg, it is not override redirect and it is a child of a window for which we have a SubstructureRed
Bug 886192 is not an example of this. In fact, the behaviour exhibited by bug 886192 is primarily /because/ we use the geometry last received from the server rather than geometry last sent, and the latency of which explains the reason why the window movement lags the cursor, because the server is "catching up".
The only case where you can't guaruntee that a similar update for geometry is going to be delivered by the server for serverGeometry as stored on XConfigureWindow is either in the case that A) Another client has SubstructureRed
> 3. There are two instances of double semicolons ";;" in this proposal.
Fix
>
> 4. I suspect this proposal will conflict just slightly with the proposal for
> bug 940139. But it should be minor.
Yes, fixable
>
> 5. NEW REGRESSION:
> 375 - XSynchronize (dpy, synchronousX ? True : False);
> 376 +// priv->connection = XGetXCBConnection (priv->dpy);
Please elaborate further on why this is a regression.
>
> 6. Why always two spaces before "* 2" ?
Most likely copy-and-paste errors

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
#5 is a regression because it will make "--sync" be ignored. Line 375 is important and should not be deleted.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
On Mon, 27 Feb 2012, Daniel van Vugt wrote:
> Review: Needs Fixing
>
> #5 is a regression because it will make "--sync" be ignored. Line 375 is important and should not be deleted.
Thank you. Fixing.
> --
> https:/
> You are the owner of lp:~smspillaz/compiz-core/compiz-core.work_923683.
>

Mikkel Kamstrup Erlandsen (kamstrup) wrote : Posted in a previous version of this proposal | # |
> > #5 is a regression because it will make "--sync" be ignored. Line 375 is
> important and should not be deleted.
>
> Thank you. Fixing.
Be sure to add a comment there explaining why that call needs to be there - this review demonstrates that it should have been there in the first place :-)

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Comments are good. Though the string "synchronousX" is unique so its use should be quite clear already without a comment.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Looks like a mistake:
710 - if (x2 > priv->width)
711 - x2 = priv->width;
712 - if (y2 > priv->height)
713 - y2 = priv->height;
714 + if (x2 > priv->serverGeo
715 + x2 = priv->serverGeo
716 + if (y2 > priv->serverGeo
717 + y2 = priv->serverGeo

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Also, there seems to be a HUGE performance regression when moving windows with this new branch. Try running under callgrind. With lp:compiz-core it's nice and fast. However with this branch it is painfully slow moving windows.
About 75% of the time is spent in:
775,164,942 /home/dan/
772,734,309 /home/dan/
771,062,144 /home/dan/
636,443,803 /home/dan/
636,266,731 /home/dan/

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
The main paths causing this massive performance regression are:
70% of the time:
CompWindow::move
DecorWindow:
DecorWindow:
18% of the time:
CompWindow::move
CompWindow:
PrivateWindow:

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Hmm, I would imagine this is the throttling of motion events that we mentioned earlier. I think in lp:compiz-core we would have just spent all that time in CompWindow::move although I'll conceed that configureXWindow is a bit more of an ... involved function (although I haven't noticed any *noticable* performance problems).

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I also recommend "kcachegrind" to display callgrind output in a pretty graphical way.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Try using valgrind/callgrind to simulate a slow machine. You will see a monumental difference in performance when dragging windows. Even my i7 (running callgrind) can't keep up with dragging windows using this branch. But it is almost perfectly fluid without this branch.
Maybe it is the fact that all motion events are getting though now. But regardless, people with slow-to-regular machines will be noticeably impacted. We need a fix before accepting this.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
From what I can tell, the problem is that moveNotify is called much more often with this branch. However moveNotify is far too expensive to call so often, mostly due to DecorWindow:
Does DecorWindow:

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
One small caveat is that I haven't merged lp:compiz-core into this one today, which includes the region fixes.
> Does DecorWindow:
Yes it does. There are some optimizations we can do there to make it only compute regions on certain windows. I've done them before, so its not too hard. Not for this branch.
> From what I can tell, the problem is that moveNotify is called much more often with this branch. However moveNotify is far too expensive to call so often, mostly due to DecorWindow:
So I think there are three optimizations which can be applied here, all of which are quite involved, so I'll need to put them in separate branches.
Firstly, we need to throttle motion events, so they aren't dispatched spuriously. That's easy enough to do.
The second which is a bit more complicated is to also throttle dispatch of serverGeometry updates / moveNotify / XConfigureWindow as well. There are some plugins which need immediate move notify updates but probably not immediate move notify updates 1000 times a second.
Another optimization is to only call XConfigureWindow when we actually need to update the server side position and not necessarily all the time. This does mean that serverGeometry will be "ahead" of whats being sent to the server, but that's OK since we can just flush the changes on demand (and besides, its meant to be ahead, which is why its there).
Incidentally, I'm not able to get such high readings in callgrind for window movement, but I guess this is a case-by-case thing.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Window movement accounts for all my CPU because that's all I am testing since I noticed the problem;
Start compiz
Move windows around lots
Stop compiz
With this branch I see movement account for 60-80% of compiz' time and is incredibly slow and stuttering. Without this branch, it only uses maximum 20% of compiz' time and is visually much smoother.

Daniel van Vugt (vanvugt) : Posted in a previous version of this proposal | # |

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
I merged lp:compiz-core from today. Could you give it another try and let me know how it goes ?
Also some thoughts on those optimizations would be nice :)
Currently I'm testing using the entire unity stack, but I can reduce it down to just compiz and a basic set of plugins.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I was already testing this branch merged with the latest lp:compiz-core (including yesterday's optimizations) all along. Which makes the regression from this branch more worrying.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
tOn Tue, 28 Feb 2012, Daniel van Vugt wrote:
> I was already testing this branch merged with the latest lp:compiz-core (including yesterday's optimizations) all along. Which makes the regression from this branch more worrying.
> --
> https:/
> You are the owner of lp:~smspillaz/compiz-core/compiz-core.work_923683.
>
OK, well, I know there are some code paths which can be slow, so lets look
at optimizing. Do you have any thoughts about the ideas I raised ?

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I don't have the encyclopaedic knowledge of CompWindow or the time to research your suggestions right now. So no comment.
But I'm happy to test/profile any further additions to this proposal... tomorrow.
(Daniel logs off)

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
I don't see anything obviously wrong, but would like to hear the results of Daniel's profiling.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Tested revision 2994. Moving windows is still so slow it's unusable (under valgrind). We should fix this because it's a good indication that the same would occur in the wild on a slow machine.
compiz is spending 70% of it's CPU in or below moveHandleMotio
(73% of the 70%)
moveHandleMotio
CompWindow::move
DecorWindow:
DecorWindow:
(16% of the 70%)
moveHandleMotio
CompWindow::move
CompWindow:
PrivateWindow:

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
And no, combining this branch with lp:~smspillaz/compiz-core/compiz-core.optimization-inlining
does not solve or change the performance problem.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Indeed, that's not meant to completely fix it, but it does fix some hotspots and other inefficiencies I observed :)

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
I'm concerned that the code still looks "fragile" (to use Sam's word). For example:
50 + y1 = geom.height () + geom.border ();
...
59 + y2 = geom.height () - geom.border ();
It isn't explicit why border is added in one case and subtracted in the other. I suspect that there's an abstraction missing (or at least suitably named functions that derive the appropriate result from geom). Something like "internalHeight = internalHeight(
I've not tested the latest version enough to be confident in it. Will aim to do so on Thursday.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> I'm concerned that the code still looks "fragile" (to use Sam's word). For
> example:
>
> 50 + y1 = geom.height () + geom.border ();
> ...
> 59 + y2 = geom.height () - geom.border ();
>
> It isn't explicit why border is added in one case and subtracted in the other.
> I suspect that there's an abstraction missing (or at least suitably named
> functions that derive the appropriate result from geom). Something like
> "internalHeight = internalHeight(
>
> I've not tested the latest version enough to be confident in it. Will aim to
> do so on Thursday.
Ack, I'll add something like that to compiz:

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Alan: see https:/

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Tested this branch by itself and merged with lp:~smspillaz/compiz-core/compiz-core.fix_969108.2
By itself, this branch still makes window movement unacceptably slow. With the other branch designed to make movement more efficient, windows now often don't move at all when dragged.
Unfortunately performance is getting worse, not better.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Its probably a small typo during refactoring. Please don't panic

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I got this branch performing somewhat better by combining it with:
https:/
However I would say still noticeably slower than lp:compiz-core. :(

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Confirmed the performance problems with this branch do seem to be fixed by:
https:/

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Please resubmit with prerequisite:
lp:~smspillaz/compiz-core/compiz-core.fix_969101

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
This branch does appear to fix bug 862430.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I was so close to approving and merging this today. But in final testing I noticed it triggers window border/shadow artefacts when resizing. They are easiest to see in Normal resize mode, but still happen much less noticeably in Rectangle resize mode too.
Just put a Nautilus window in front of a large white window and resize the Nautilus window rapidly from its top-right corner. Ugly artefacts will be left on the window behind.
I actually suspect the resize plugin being to blame for this, because I have encountered similar bugs with it before. And read that the GLES branch seems to have too.
However right now, it's being triggered by this branch. So I can't approve it just yet.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I can't reproduce the same resize artefacts with my experimental branch, only with this one.
Perhaps we didn't notice the artefacts before because they are the result of the latest compiz-core commit r3086 combining with this branch...

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Resubmitted again.
LP #923520 is also fixed in this branch. I'm not sure if the artefacts are fixed "for good" but I can't reproduce them anymore since fixing that one.
(I cannot remove that fix from this branch, since its actually dependent on the fact that we can update window regions before sending geometry to the server, which wouldn't be possible in core at the moment)

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> (I cannot remove that fix from this branch, since its actually dependent on
> the fact that we can update window regions before sending geometry to the
> server, which wouldn't be possible in core at the moment)
Clarifying ambiguous words: I can't separate out that fix.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Typo : bug 932520

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
The intent has always looked sensible. The latest code looks the tidiest. And I've been running it all morning without noticing anything untoward.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
Probably a reason not to merge, but running with "wobbly windows", and dragging a window across others there are repaint issues on the "behind" windows that don't resolve until the moved window is released.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
> Probably a reason not to merge, but running with "wobbly windows", and
> dragging a window across others there are repaint issues on the "behind"
> windows that don't resolve until the moved window is released.
That should be "Probably /not/ a reason not to merge"

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> Probably a reason not to merge, but running with "wobbly windows", and
> dragging a window across others there are repaint issues on the "behind"
> windows that don't resolve until the moved window is released.
I'll resolve this anyhow since it could impact other plugins.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Please remove the ABI changes (include/

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Also, when I maximize and restore a window, it moves left by about 1 pixel each time. So I don't think we can declare this branch as having fixed bug 892012.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
And confirmed the regression with wobbly windows this branch introduces. Although not officially enabled by default, enough people do use wobbly windows for us to want to avoid introducing such a regression.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Confirmed the flicker bug 862430 is fixed. So that's some good news.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Both issues fixed, please re-review.
Will look into the 1px shift later.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
Confirm wobbly windows fixed. They are rather cute!

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I'm still testing the various bugs, and want to continue testing for tomorrow at least.
So far I can't see any regressions. And the 1px movement issue is actually not related to this branch, so forget that for now.
Once I'm happy it runs OK and further fixes are not required, then I'll start reviewing the code again in detail.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I sense a slight performance regression when moving windows compared to trunk. Trunk is slightly smoother. But it's only noticeable when running under callgrind and only slight.
I have confirmed 3 of the 4 bugs are fixed, and the other 1 I can't reproduce.
All good so far. But I won't review the rather large diff in detail tonight.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
Only problem I've seen today is that when I replace the graphics stack all windows end up in 1st workspace. I don't think that is this branch - but haven't got to the bottom of it yet.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
* priv->frameRegion -= infiniteRegion
should be:
priv->frameRegion = emptyRegion;
* Gererally speaking, x1 + width != x2
That's an off-by-one mistake. It should usually be: x1 + width - 1 == x2
However I can't prove that's causing any problems right now and it's not new in this branch.
* Regression: The resize artefacts reported on 5 April are still present. I thought they were meant to be fixed?
I really really don't want to see any more revisions of this branch. But the regression is kind of a problem.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Thanks, the regression is now fixed (or at least worked around by damageScreen).
And after 10 weeks(!) of revisions on this branch, I'm sure I'm not the only one who is fed up looking at it.
As far as I can tell, it fixes all the bugs linked to it. And there are no visible regressions now. Let's land it, and move on.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
As above.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
LGTM

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
REGRESSION:
When resizing a window's top or left edges, the opposite edge jitters about (in Normal resize move). I know I mentioned this bug yesterday but I only just realized it's a regression that's unique to this branch.
Sounds like that's the cause of the resize artefacts too. So if we fix the regression then we can remove the damageScreen workaround from the resize plugin.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
And please wait till Monday at least before proposing any new revisions. It would ruin my weekend to see this branch come up in weekend email :)

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Make that Tuesday. You're meant to have Monday off, I believe :)

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> And please wait till Monday at least before proposing any new revisions. It
> would ruin my weekend to see this branch come up in weekend email :)
Nobody is making you look at this .......

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Okay.
Normal resize mode was not something I was planning on supporting in precise. Nevertheless, I've merged in some work I did a few weeks ago to essentially fix it up so that there's no tearing and windows don't appear to jump around when you resize them.
There is still a race condition in the window decorators where pixmaps can be freed server side before they are bound by the decorator. Getting around that race condition requires some synchronization protocol which I've prototyped and works fine with gtk-window-
The damageScreen call in DecorWindow:
I hope this is the last revision we need to do of this.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Contains a trivial conflict in src/window.cpp, but simple to fix and carry on testing.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
Breaks with wobby windows - moving windows distorted, semi-maximized windows placed wrongly

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Regression: Vertically maximizing a window while wobbly is enabled causes it to over-shoot either the top or bottom of the screen, instead of fitting to the top and bottom.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Erm, I think we both just said the same thing :)

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
> Erm, I think we both just said the same thing :)
Maybe we're both right?
;)

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Issues with the wobbly plugin fixed.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
That being said, please keep in mind that for ubuntu we really don't care about plugins and options that we don't enable by default. All upgraders get their settings reset.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
[Still using wobbly of course] largely fixed.
If I drag to LHS (say) the orange highlight looks fine, but the window first paints and wobbles a few pixels to left before jumping to the correct position.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
The glitch with wobbly is minor (and now doesn't happen consistently) other issues appear fixed.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Regression: Weirdly, this branch (r3017) causes a regression of bug 981703. So Alan's fix wasn't the root cause, just a trigger. But we knew that already.
Also contains conflicts.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Regression #2: This branch causes artefacts/gaps in the switcher window. Screenshot emailed to smspillaz.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Regressions fixed.
The big change in this branch is here:
CompWindow:
CompWindow:
{
- return priv->serverGeo
+ return priv->attrib.
}
CompWindow:
CompWindow:
{
- return priv->geometry;
+ return priv->attrib.
}
We're now making all calls to both geometry () and serverGeometry () for managed windows return geometry last sent to server. The reason is that for managed windows, configureXWindow can never fail, so the geometry last sent is always the most up to date and the most accurate.
Override redirect windows cant be configured so we used the geometry last received from the server from that.
In my testing, this fixed practically all the flakiness.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
I have no reason to believe it is doing the wrong thing, but expressions like the following don't parse easily:
2706 + return CompRect (geometry ().xMinusBorder () - priv->border.left,
2707 + geometry ().yMinusBorder () - priv->border.top,
why are we subtracting a border from "*MinusBorder"? why isn't that what "MinusBorder" does?
2708 + geometry ().widthIncBorders () +
2709 priv->border.left + priv->border.right,
2711 + geometry ().heightIncBorders () +
2712 priv->border.top + priv->border.
why are we adding borders to "*IncBorders"? why isn't that what "IncBorders" does?
(I suspect it's a naming thing, or a missing abstraction.)

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Regression: Window is offset from its decorations (vertically this time).
Test case : Open a terminal, minimize it, restore it using switcher.
Outcome : Window is offset vertically from its decorations.
I've emailed a screenshot to smpsillaz.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
The branch also contains more complex conflicts now that need fixing.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Regression #2: Shading is very broken. Windows shade/unshade without their decorations changing (until the window is moved). And often I can't unshade windows at all.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Lines 2537..2547: This is the same as Alan's branch and ideally shouldn't be in this one too. I will reintroduce that code to trunk soon, when my new fix for bug 981703 is merged.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
And I spy more pointless "-= infiniteRegion" statements.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
Regressions fixed.
Alan - now that server* is effectively no longer necessary, we can proceed to bulk-rename things in the next series when we get all the code in one place. Wdyt ?

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
Not seen any problems over the last couple of hours. Will have a look at the code tomorrow (unless there are more changes by then - in which case I might swear).

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Regression #1: Windows lose their borders (and most shadows) during shading animation. Try this with a Terminal so it's more visible.
Regression #2: "failed to bind pixmap to texture" warnings and flashing white decorations during resize are now much more common in this branch than with trunk. Annoyingly so.
Regression #3: Moving wobbly windows in expo (esp. between workspaces) jump a significant distance after the wobble.
Regression #4: Vertically maximized wobbly windows sometimes jump up/down by one pixel after the wobble, making the bottom border visible/invisible. Try Alt+dragging a vertically maximized window up a little, and sometimes it will move up ~1px. Normally the borders of semi-maximized windows overlap adjacent workspaces by 1px. That's a bug in trunk too, but this regression makes it worse (2px) and unpredictable.
I dare say it might be time to start again, using multiple smaller proposals rather than this one big one. I don't believe a single large proposal is the only way to solve the four related bugs.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
> Regression #1: Windows lose their borders (and most shadows) during shading
> animation. Try this with a Terminal so it's more visible.
Insignificant.
>
> Regression #2: "failed to bind pixmap to texture" warnings and flashing white
> decorations during resize are now much more common in this branch than with
> trunk. Annoyingly so.
I'm not supporting normal resize mode. See my previous comments as to the flashing white borders and the solution to that.
>
> Regression #3: Moving wobbly windows in expo (esp. between workspaces) jump a
> significant distance after the wobble.
This is the only one worth attention. I will look into it.
>
> Regression #4: Vertically maximized wobbly windows sometimes jump up/down by
> one pixel after the wobble, making the bottom border visible/invisible. Try
> Alt+dragging a vertically maximized window up a little, and sometimes it will
> move up ~1px. Normally the borders of semi-maximized windows overlap adjacent
> workspaces by 1px. That's a bug in trunk too, but this regression makes it
> worse (2px) and unpredictable.
Why is this a bad thing ? Why does this justify rejecting this branch. This feels very insignificant.
>
> I dare say it might be time to start again, using multiple smaller proposals
> rather than this one big one. I don't believe a single large proposal is the
> only way to solve the four related bugs.
Not possible.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Please don't say something's impossible. It will only motivate me (or other people) to prove you wrong, and that would be a duplication of effort and waste of resources.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
Everyone is getting fed up with this branch.
IMO the prop-review cycle is getting in the way. This sort of problem is better addressed by a pairing session. (Not so easy when not co-located, but still possible.)

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal | # |
I'm resubmitting this into the development branch now.
Please Please Please.
We will not be supporting plugins like wobbly and friends in later versions of ubuntu. As such, minor regressions in those plugins are not a good reason to reject this branch.
Normal resize mode is another can of worms. Please don't talk about it either.
The only valid reasons to block this branch are
1) Bad code
2) A regression in the actual supported ubuntu usecases

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
I can imagine regressions #1, #2 and #4 being accepted and ignored indefinitely. But #3 is something I would not want to see in 0.9.8.0. It's a commonly used feature (albeit unofficial) that has worked perfectly until now. If you don't want to fix it then either myself or Alan will be motivated to, because it's annoying us personally.
Please try to fix regression #3 before we merge this branch.

Sam Spilsbury (smspillaz) wrote : | # |
Regressions 1, 3 and 4 fixed (even if they were tiny)
If you want to fix regression 2 (even though its not really a regression, just a race condition that has always existed), then please check out lp:~smspillaz/compiz-core/compiz-core.decor_synchronization . That adds some synchronization primitives to the decoration protocol which fixes races where pixmaps were being bound after destruction.
I've also cleaned up the code a bit now that we don't need to use server* in client code, so the diff size should be smaller.

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
Can we rename this merge "Groundhog Day"?

Alan Griffiths (alan-griffiths) wrote : | # |
Not seen any problems over the last few of hours of use (but not been running wobbly). Will have a look at the code tomorrow (unless there are more changes by then - in which case I might swear).

Daniel van Vugt (vanvugt) wrote : | # |
Pointless use of abs(): All uses of the abs() function in the diff appear to be numerically pointless;
if (abs (foo))
is the same as:
if (foo)
Pointless temporary creation/
1345 + priv->region = priv->inputRegion = CompRegion ();
Should just assign emptyRegion I think.

Daniel van Vugt (vanvugt) wrote : | # |
But the good news is that it's running nicely so far.
- 3032. By Sam Spilsbury
-
Fix logic errors

Daniel van Vugt (vanvugt) : | # |
- 3033. By Sam Spilsbury
-
!= 0 not necessary

Alan Griffiths (alan-griffiths) wrote : | # |
Since grabbing latest revision and switching to wobbly I'm seeing misplacement of half-maximized windows.
- 3034. By Sam Spilsbury
-
Merge lp:compiz-core

Daniel van Vugt (vanvugt) wrote : | # |
Alan, can you elaborate?

Alan Griffiths (alan-griffiths) wrote : | # |
> Alan, can you elaborate?
E.g. drag a window to RHS and get half max highlight. Release and the window goes to a position that is not at the side of the screen.

Daniel van Vugt (vanvugt) wrote : | # |
Hmm, I can't reproduce any such regressions with wobbly+grid plugins and this branch.

Alan Griffiths (alan-griffiths) wrote : | # |
> Hmm, I can't reproduce any such regressions with wobbly+grid plugins and this
> branch.
Odd. Am testing something else just now, but will retest shortly.

Alan Griffiths (alan-griffiths) wrote : | # |
Looks good now. (Can't reproduce friday's problems.)

Daniel van Vugt (vanvugt) wrote : | # |
Double-checked all the header (ABI) changes against plugins-main, plugins-extra and unity. Confirmed the affected classes are not used in any of the major plugin packages.
The diff is big and scary, but its not too complicated. I'll explain each separate part
1 === modified file 'include/ core/window. h' core/window. h 2012-01-20 15:20:44 +0000 core/window. h 2012-01-31 17:12:59 +0000 tes);
2 --- include/
3 +++ include/
4 @@ -398,9 +398,9 @@
5
6 bool hasUnmapReference ();
7
8 - bool resize (XWindowAttribu
9 + bool resize (const XWindowAttributes &);
10
11 - bool resize (Geometry);
12 + bool resize (const Geometry &);
13
14 bool resize (int x, int y, int width, int height,
15 int border = 0);
const correctness
31 int x1, x2, y1, y2; :Geometry geom = priv->window- >geometry (); >output (); :Geometry &geom = priv->window- >serverGeometry (); >output ();
32
33 - CompWindow:
34 - CompWindowExtents output = priv->window-
35 + const CompWindow:
36 + const CompWindowExtents &output = priv->window-
more const correctness
35 + const CompWindow: :Geometry &geom = priv->window- >serverGeometry (); >output ();
36 + const CompWindowExtents &output = priv->window-
Lots of this - in paint code we want to use serverFoo because it was the last thing sent to the server. And this is always guarunteed to come back to us.
166 - unsigned int width = window->size ().width ();
167 - unsigned int height = window->size ().height ();
168 + unsigned int width = window->geometry ().width ();
169 + unsigned int height = window->geometry ().height ();
CompWindow::size was just returning CompSize (priv->width, priv->height) which was just redundant data which should be removed. Replaced it with geometry () for clarity
299 + void move (int dx, int dy, bool sync); :Geometry &g);
300 + bool resize (int dx, int dy, int dwidth, int dheight, int dborder);
301 + bool resize (const CompWindow:
302 + bool resize (const XWindowAttributes &attrib);
303 +
Added PrivateWindow::move and PrivateWindow: :resize - to be called whenever a new position / size comes back from the server. (cut'n'paste code from CompWindow::move and CompWindow::resize
335 - Request, NULL); >syncPosition (); border () * 2; width () + serverInput.left + serverInput.right + bw; height () + serverInput.top + serverInput.bottom + bw; metry.x () == xwc.x) etry.setX (xwc.x); width (), serverGeometry. height ()); >sendConfigureN otify (); >windowNotify (CompWindowNoti fyFrameUpdate. ..
336 - gettimeofday (&lastConfigure
337 - /* Flush any changes made to serverFrameGeometry or serverGeometry to the server
338 - * since there is a race condition where geometries will go out-of-sync with
339 - * window movement */
340 -
341 - window-
342 -
343 - if (serverInput.left || serverInput.right || serverInput.top || serverInput.bottom)
344 - {
345 - int bw = serverGeometry.
346 -
347 - xwc.x = serverGeometry.x () - serverInput.left;
348 - xwc.y = serverGeometry.y () - serverInput.top;
349 - xwc.width = serverGeometry.
350 - if (shaded)
351 - xwc.height = serverInput.top + serverInput.bottom + bw;
352 - else
353 - xwc.height = serverGeometry.
354 -
355 - if (shaded)
356 - height = serverInput.top + serverInput.bottom;
357 -
358 - if (serverFrameGeo
359 - valueMask &= ~(CWX);
360 - else
361 - serverFrameGeom
362 -
*snip*
616 - XMoveResizeWindow (screen->dpy (), id, 0, 0,
617 - serverGeometry.
618 - window-
619 - window-