Code review comment for lp:~canonical-dx-team/unity/unity.intellihide

Sam Spilsbury (smspillaz) wrote :

Hi Jason,

I'm not able to test since nux doesn't work on both of my systems at the moment *COUGH* but some small ideas for improvement before you merge.

1) ::resizeNotify will only tell you of the new window geometry once the geometry has actually changed, not while the window is resizing, in the case that we are using the "outline, rectangle or stretch" modes. In that case you'll want to wrap handle event and listen for the "_COMPIZ_RESIZE_NOTIFY" atom and use that to determine the *current* resize geometry. Have a look at group.cpp:1827 on how this is done.

2) Your current approach wraps resizeNotify and moveNotify for every single window in the stack. This is a little bit inefficient IMO - maybe you should wrap grabNotify () to determine which window was grabbed in the first place and then enable moveNotify and resizeNotify on the window (moveNotifySetEnabled, resizeNotifySetEnabled) depending on the grab type. Also enabled ungrabNotify at this point and when that comes you can disable all of those functions. That way you aren't being pinged all the time every single time a window moves or resizes for other reasons, eg because the wall plugin shifted the screen to the left or something.

3) You should not use w->frameRegion (). Use w->inputRegion () instead. w->frameRegion () includes only the geometry of the window frame itself, eg a big region with a hole cut in the middle. In situations where the frame size is bigger than the dock size, this could lead to an awkward race condition where the two regions won't intersect because of that hole.

4) For unit testing it might not be such a good idea to bring Compiz specific definitions into Launcher.cpp such as CompScreen and CompWindow. Maybe instead of checking the entire window list there you can just call OnWindowMoved or OnWindowResized with the X Region of the inputRegion for the window on moveNotify or grabNotify and save a count of "currently intersecting regions". (To get the X region you can call someRegion->handle ()). That way you can just XIntersectRegion like normal.

Other than that it looks good.

review: Needs Fixing

« Back to merge proposal