Merge lp:~vorlon/compiz/lp.763148 into lp:compiz/0.9.10

Proposed by Steve Langasek on 2013-04-07
Status: Merged
Approved by: Sam Spilsbury on 2013-04-07
Approved revision: 3651
Merged at revision: 3646
Proposed branch: lp:~vorlon/compiz/lp.763148
Merge into: lp:compiz/0.9.10
Diff against target: 466 lines (+204/-126)
3 files modified
plugins/place/src/place.cpp (+3/-1)
plugins/place/src/screen-size-change/src/screen-size-change.cpp (+10/-37)
plugins/place/src/screen-size-change/tests/screen-size-change/src/test-place-screen-size-change.cpp (+191/-88)
To merge this branch: bzr merge lp:~vorlon/compiz/lp.763148
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration 2013-04-07 Approve on 2013-04-07
MC Return 2013-04-07 Abstain on 2013-04-07
Sam Spilsbury 2013-04-07 Approve on 2013-04-07
Review via email: mp+157537@code.launchpad.net

This proposal supersedes a proposal from 2013-04-07.

Commit message

Fix for bug #763148 (with added test cases): when the desktop is resized,
windows should stay on their original workspace.

Description of the change

Fix for bug #763148 (with added test cases): when the desktop is resized,
windows should stay on their original workspace.

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:3647
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/~vorlon/compiz/lp.763148/+merge/157534/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/compiz-ci/122/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/159/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/511/console

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

review: Needs Fixing (continuous-integration)
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Hey, thanks so much for doing this, and also adding test cases.

A quick note: I'm marking this as "resubmit" as there are merge conflicts. It appears that you've taken lp:compiz/raring, made changes there, and then submitted it for merging against lp:compiz . The two branches differ slightly (distro is cherry-picking fixes from lp:compiz to lp:compiz/0.9.9). Also - changes to debian/changelog are unnecessary and probably not wanted either. The automerger will automatically update the debian changelog, and updating it manually will probably confuse it.

As for some code review, here are some suggestions I have.

1. Thanks for refactoring some of the common setup code. I think the method could be renamed to assignStrutsWorkAreaAndGeoemtry
2. In TestScreenSizeChangePreviousViewport:

506 + /* Deal with the case where the position is negative, which means
507 + * it's actually wrapped around to the rightmost viewport
508 + */
509 + g.setPos (CompPoint (-300, 200));
510 + ms.setGeometry (g);
511 +
512 + expected = compiz::window::Geometry (-300, 200, 300, 400, 0);

It appears that the relevant assertion here is that the window will have the same geometry if wrapped to the rightmost viewport when the rightmost viewport is unplugged. In that case, it might be better to just set "expected" to "g", as they are both the same.

Everything else seems mostly fine to me.

Doing this review reminded me that both the tests and adjustForSize need some serious refactoring, because even in their simplified versions, there is way too much going on in both of them. I'm not going to block this review on it, however, because:
 a) They were monsters of my own creation, and I should have the responsibility to deal with them.
 b) I was going to suggest doing the refactoring in this review, but decided against it because articulating what needed to be done was so detailed and complicated that it would just cause miscommunication and lots of unrelated change.

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

What you'll need to do from here is:

1. Branch lp:compiz
2. Cherry pick revisions 3643..3646 from your old branch into that one
3. Submit that branch.

review: Resubmit
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

(Or at least 3646 from this branch)

MC Return (mc-return) wrote : Posted in a previous version of this proposal

\o/
Stopping windows from jumping around weirdly should be numbero uno priority for us...
But I do not like this diff either...

review: Needs Fixing
Steve Langasek (vorlon) wrote : Posted in a previous version of this proposal

On Sun, Apr 07, 2013 at 05:59:18AM -0000, Sam Spilsbury wrote:
> Hey, thanks so much for doing this, and also adding test cases.

> A quick note: I'm marking this as "resubmit" as there are merge conflicts.
> It appears that you've taken lp:compiz/raring, made changes there, and
> then submitted it for merging against lp:compiz

Yep, was betrayed by the default behavior of bzr lp-propose-merge here.
I've rebased now and resubmitted:

  https://code.launchpad.net/~vorlon/compiz/lp.763148/+merge/157537

> As for some code review, here are some suggestions I have.

> 1. Thanks for refactoring some of the common setup code. I think the
> method could be renamed to assignStrutsWorkAreaAndGeoemtry

I don't find that a very clear description of what the method does, since
it's also what calls adjustForSize(). But maybe I'm misunderstanding?

> 2. In TestScreenSizeChangePreviousViewport:

> 506 + /* Deal with the case where the position is negative, which means
> 507 + * it's actually wrapped around to the rightmost viewport
> 508 + */
> 509 + g.setPos (CompPoint (-300, 200));
> 510 + ms.setGeometry (g);
> 511 +
> 512 + expected = compiz::window::Geometry (-300, 200, 300, 400, 0);

> It appears that the relevant assertion here is that the window will have
> the same geometry if wrapped to the rightmost viewport when the rightmost
> viewport is unplugged. In that case, it might be better to just set
> "expected" to "g", as they are both the same.

Right, changed - thanks.

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

Sam Spilsbury (smspillaz) wrote :

This looks fine to me. I plan to do some rather large-ish refactoring on this section of the code, so lets not make the perfect the enemy of the good here.

review: Approve
MC Return (mc-return) wrote :

/offtopic on
During recent intensive testing I found another jumping window bug:
bug 1165695

Anyone an idea about the origin (in the code) of that one ?
/offtopic off

Steve, I could not test your branch that fast, but my "Approve" does
not count anyway ;)

review: Abstain
review: Approve (continuous-integration)
Didier Roche (didrocks) wrote :

@Steve: the destination branch is now 0.9.9 for raring (btw, I think we should update Vcs-Bzr). Mind havind a look for making a MP there as well?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/place/src/place.cpp'
2--- plugins/place/src/place.cpp 2013-03-24 05:51:41 +0000
3+++ plugins/place/src/place.cpp 2013-04-07 08:57:20 +0000
4@@ -202,7 +202,9 @@
5 PlaceScreen::handleEvent (XEvent *event)
6 {
7 if (event->type == ConfigureNotify &&
8- event->xconfigure.window == screen->root ())
9+ event->xconfigure.window == screen->root () &&
10+ (event->xconfigure.width != screen->width () ||
11+ event->xconfigure.height != screen->height ()))
12 {
13 mPrevSize.setWidth (screen->width ());
14 mPrevSize.setHeight (screen->height ());
15
16=== modified file 'plugins/place/src/screen-size-change/src/screen-size-change.cpp'
17--- plugins/place/src/screen-size-change/src/screen-size-change.cpp 2012-07-30 10:01:45 +0000
18+++ plugins/place/src/screen-size-change/src/screen-size-change.cpp 2013-04-07 08:57:20 +0000
19@@ -40,8 +40,6 @@
20 int vpX, vpY;
21 compiz::window::Geometry g, vpRelRect;
22 int pivotX, pivotY;
23- int curVpOffsetX = getViewport ().x () * newSize.width ();
24- int curVpOffsetY = getViewport ().y () * newSize.height ();
25
26 g = getGeometry ();
27 compiz::window::Geometry og (g);
28@@ -59,24 +57,14 @@
29 if (pivotY < 0)
30 vpY -= 1;
31
32- /* if window's target vp is to the left of the leftmost viewport on that
33- row, assign its target vp column as 0 (-s->x rel. to current vp) */
34- if (getViewport ().x () + vpX < 0)
35- vpX = -getViewport ().x ();
36-
37- /* if window's target vp is above the topmost viewport on that column,
38- assign its target vp row as 0 (-s->y rel. to current vp) */
39- if (getViewport ().y () + vpY < 0)
40- vpY = -getViewport ().y ();
41-
42 unsigned int mask = mSaver.pop (vpRelRect, CHANGE_X | CHANGE_Y |
43 CHANGE_WIDTH | CHANGE_HEIGHT);
44
45 if (mask)
46 {
47 /* set position/size to saved original rectangle */
48- g.applyChange (compiz::window::Geometry (vpRelRect.x () + vpX * newSize.width (),
49- vpRelRect.y () + vpY * newSize.height (),
50+ g.applyChange (compiz::window::Geometry (vpRelRect.x (),
51+ vpRelRect.y (),
52 vpRelRect.width (),
53 vpRelRect.height (),
54 vpRelRect.border ()), mask);
55@@ -90,42 +78,24 @@
56 vpRelRect.setWidth (g.width ());
57 vpRelRect.setHeight (g.height ());
58
59- g.setPos (g.pos ());
60-
61- int shiftX = vpX * (newSize.width () - oldSize.width ()),
62- shiftY = vpY * (newSize.width () - oldSize.height ());
63+ g = vpRelRect;
64
65 /* if coords. relative to viewport are outside new viewport area,
66 shift window left/up so that it falls inside */
67- if (vpRelRect.x () >= newSize.width ())
68- shiftX -= vpRelRect.x () - (newSize.width () - 1);
69- if (vpRelRect.y () >= newSize.height ())
70- shiftY -= vpRelRect.y () - (newSize.height () - 1);
71-
72- if (shiftX)
73- g.setX (g.x () + shiftX);
74-
75- if (shiftY)
76- g.setY (g.y () + shiftY);
77+ if (vpRelRect.x () + vpRelRect.width() >= newSize.width ())
78+ g.setX (g.x () - (vpRelRect.x () + vpRelRect.width () - newSize.width ()));
79+ if (vpRelRect.y () + vpRelRect.height() >= newSize.height ())
80+ g.setY (g.y () - (vpRelRect.y () + vpRelRect.width () - newSize.height ()));
81
82 g.setWidth (vpRelRect.width ());
83 g.setHeight (vpRelRect.height ());
84 }
85
86- /* Handle non-(0,0) current viewport by shifting by curVpOffsetX,Y,
87- and bring window to (0,0) by shifting by minus its vp offset */
88-
89- g.setX (g.x () + curVpOffsetX - (getViewport ().x () + vpX) * newSize.width ());
90- g.setY (g.y () + curVpOffsetY - (getViewport ().y () + vpY) * newSize.height ());
91-
92 unsigned int flags = 0;
93 const CompRect &workArea = getWorkarea (g);
94
95 compiz::place::clampGeometryToWorkArea (g, workArea, getExtents (), flags, newSize);
96
97- g.setX (g.x () - curVpOffsetX + (getViewport ().x () + vpX) * newSize.width ());
98- g.setY (g.y () - curVpOffsetY + (getViewport ().y () + vpY) * newSize.height ());
99-
100 if (!mask)
101 {
102 /* save window geometry (relative to viewport) so that it
103@@ -151,6 +121,9 @@
104 mSaver.push (vpRelRect, remaining);
105 }
106
107+ g.setX (g.x () + vpX * newSize.width ());
108+ g.setY (g.y () + vpY * newSize.height ());
109+
110 /* for maximized/fullscreen windows, update saved pos/size XXX,
111 * also pull in the old code to handle maximized windows which
112 * currently can't be implemented yet */
113
114=== modified file 'plugins/place/src/screen-size-change/tests/screen-size-change/src/test-place-screen-size-change.cpp'
115--- plugins/place/src/screen-size-change/tests/screen-size-change/src/test-place-screen-size-change.cpp 2012-07-26 09:10:09 +0000
116+++ plugins/place/src/screen-size-change/tests/screen-size-change/src/test-place-screen-size-change.cpp 2013-04-07 08:57:20 +0000
117@@ -57,6 +57,9 @@
118 unsigned int bottom);
119
120 void setGeometry (const compiz::window::Geometry &g);
121+ compiz::window::Geometry sizeAdjustTest (const CompSize &oldSize,
122+ const CompSize &newSize,
123+ CompRect &workArea);
124
125 private:
126
127@@ -162,21 +165,34 @@
128 workArea.setBottom (workArea.bottom () - 24);
129 }
130
131+compiz::window::Geometry
132+MockScreenSizeChangeObject::sizeAdjustTest (const CompSize &oldSize,
133+ const CompSize &newSize,
134+ CompRect &workArea)
135+{
136+ /* Reserve top, bottom and left parts of the screen for
137+ * fake "24px" panels */
138+ reserveStruts (workArea);
139+
140+ setWorkArea (workArea);
141+
142+ compiz::window::Geometry g = adjustForSize (oldSize, newSize);
143+
144+ return g;
145+}
146+
147+
148 TEST_F(CompPlaceScreenSizeChangeTestScreenSizeChange, TestScreenSizeChange)
149 {
150 CompSize current, old;
151 compiz::window::Geometry g (200, 250, 300, 400, 0);
152+ compiz::window::Geometry expected;
153
154 MockScreenSizeChangeObject ms (g);
155
156 current = CompSize (1280, 800);
157
158- /* Reserve top, bottom and left parts of the screen for
159- * fake "24px" panels */
160- CompRect workArea = CompRect (0, 0, current.width (), current.height ());
161- reserveStruts (workArea);
162-
163- ms.setWorkArea (workArea);
164+ CompRect workArea;
165
166 /* First test that changing the screen size
167 * to something smaller here doesn't cause our
168@@ -184,114 +200,100 @@
169
170 old = current;
171 current = CompSize (1024, 768);
172-
173 workArea = CompRect (0, 0, current.width (), current.height ());
174- reserveStruts (workArea);
175-
176- ms.setWorkArea (workArea);
177-
178- g = ms.adjustForSize (old, current);
179-
180- EXPECT_EQ (g, compiz::window::Geometry (200, 250, 300, 400, 0));
181+
182+ expected = compiz::window::Geometry (200, 250, 300, 400, 0);
183+
184+ g = ms.sizeAdjustTest (old, current, workArea);
185+
186+ EXPECT_EQ (expected, g);
187
188 /* Making the screen size bigger with no
189 * saved geometry should cause the window not to move */
190-
191 old = current;
192 current = CompSize (2048, 768);
193-
194 workArea = CompRect (0, 0, current.width (), current.height ());
195- reserveStruts (workArea);
196-
197- ms.setWorkArea (workArea);
198-
199- g = ms.adjustForSize (old, current);
200-
201- EXPECT_EQ (g, compiz::window::Geometry (200, 250, 300, 400, 0));
202+
203+ g = ms.sizeAdjustTest (old, current, workArea);
204+
205+ EXPECT_EQ (expected, g);
206
207 /* Move the window to the other "monitor" */
208-
209 ms.setGeometry (compiz::window::Geometry (1025, 250, 300, 400, 0));
210
211- old = current;
212-
213 /* Unplug a "monitor" */
214+ old = current;
215 current = CompSize (1024, 768);
216-
217 workArea = CompRect (0, 0, current.width (), current.height ());
218- reserveStruts (workArea);
219-
220- ms.setWorkArea (workArea);
221-
222- g = ms.adjustForSize (old, current);
223-
224- EXPECT_EQ (g, compiz::window::Geometry (724, 250, 300, 400, 0));
225-
226- old = current;
227+
228+ expected = compiz::window::Geometry (724, 250, 300, 400, 0);
229+
230+ g = ms.sizeAdjustTest (old, current, workArea);
231+
232+ EXPECT_EQ (expected, g);
233
234 /* Re-plug the monitor - window should go back
235 * to the same position */
236+ old = current;
237 current = CompSize (2048, 768);
238-
239 workArea = CompRect (0, 0, current.width (), current.height ());
240- reserveStruts (workArea);
241-
242- ms.setWorkArea (workArea);
243-
244- g = ms.adjustForSize (old, current);
245-
246- EXPECT_EQ (g, compiz::window::Geometry (1025, 250, 300, 400, 0));
247-
248- old = current;
249+
250+ expected = compiz::window::Geometry (1025, 250, 300, 400, 0);
251+
252+ g = ms.sizeAdjustTest (old, current, workArea);
253+
254+ EXPECT_EQ (expected, g);
255
256 /* Plug 2 monitors downwards, no change */
257+ old = current;
258 current = CompSize (2048, 1536);
259-
260 workArea = CompRect (0, 0, current.width (), current.height ());
261- reserveStruts (workArea);
262-
263- ms.setWorkArea (workArea);
264-
265- g = ms.adjustForSize (old, current);
266-
267- EXPECT_EQ (g, compiz::window::Geometry (1025, 250, 300, 400, 0));
268+
269+ g = ms.sizeAdjustTest (old, current, workArea);
270+
271+ EXPECT_EQ (expected, g);
272
273 /* Move the window to the bottom "monitor" */
274-
275 ms.setGeometry (compiz::window::Geometry (1025, 791, 300, 400, 0));
276
277- old = current;
278-
279 /* Unplug bottom "monitor" */
280+ old = current;
281 current = CompSize (2048, 768);
282-
283 workArea = CompRect (0, 0, current.width (), current.height ());
284- reserveStruts (workArea);
285-
286- ms.setWorkArea (workArea);
287-
288- g = ms.adjustForSize (old, current);
289-
290- EXPECT_EQ (g, compiz::window::Geometry (1025, 344, 300, 400, 0));
291-
292- old = current;
293+
294+ expected = compiz::window::Geometry (1025, 344, 300, 400, 0);
295+
296+ g = ms.sizeAdjustTest (old, current, workArea);
297+
298+ EXPECT_EQ (expected, g);
299
300 /* Re-plug bottom "monitor" */
301+ old = current;
302 current = CompSize (2048, 1356);
303-
304 workArea = CompRect (0, 0, current.width (), current.height ());
305- reserveStruts (workArea);
306-
307- ms.setWorkArea (workArea);
308-
309- g = ms.adjustForSize (old, current);
310-
311- EXPECT_EQ (g, compiz::window::Geometry (1025, 791, 300, 400, 0));
312+
313+ expected = compiz::window::Geometry (1025, 791, 300, 400, 0);
314+
315+ g = ms.sizeAdjustTest (old, current, workArea);
316+
317+ EXPECT_EQ (expected, g);
318+
319+}
320+
321+TEST_F(CompPlaceScreenSizeChangeTestScreenSizeChange, TestScreenChangeWindowsOnSecondViewport)
322+{
323+ CompSize current, old;
324+ compiz::window::Geometry g (1025, 791, 300, 400, 0);
325+ compiz::window::Geometry expected;
326+
327+ MockScreenSizeChangeObject ms (g);
328+
329+ current = CompSize (2048, 1356);
330+
331+ CompRect workArea;
332
333 /* Move the entire window right a viewport */
334-
335 g.setPos (g.pos () + CompPoint (current.width (), 0));
336-
337 ms.setGeometry (g);
338
339 /* Now change the screen resolution again - the window should
340@@ -301,14 +303,115 @@
341 /* Unplug a "monitor" */
342 old = current;
343 current = CompSize (1024, 1356);
344-
345- workArea = CompRect (0, 0, current.width (), current.height ());
346- reserveStruts (workArea);
347-
348- ms.setWorkArea (workArea);
349-
350- g = ms.adjustForSize (old, current);
351-
352- EXPECT_EQ (g, compiz::window::Geometry (current.width () + 724, 791, 300, 400, 0));
353-}
354-
355+ workArea = CompRect (0, 0, current.width (), current.height ());
356+
357+ expected = compiz::window::Geometry (current.width () + 724, 791, 300, 400, 0);
358+
359+ g = ms.sizeAdjustTest (old, current, workArea);
360+
361+ EXPECT_EQ (expected, g);
362+
363+ /* Replug the monitor, make sure that the geometry is restored */
364+ old = current;
365+ current = CompSize (2048, 1356);
366+ workArea = CompRect (0, 0, current.width (), current.height ());
367+
368+ expected = compiz::window::Geometry (current.width () + 1025, 791, 300, 400, 0);
369+
370+ g = ms.sizeAdjustTest (old, current, workArea);
371+
372+ EXPECT_EQ (expected, g);
373+
374+ /* Replug the monitor and move the window to where it fits on the first
375+ * monitor on the second viewport, then make sure it doesn't move */
376+ old = CompSize (2048, 1356);
377+ current = CompSize (1024, 1356);
378+ workArea = CompRect (0, 0, current.width (), current.height ());
379+
380+ g.setPos (CompPoint (old.width () + 25, 791));
381+ ms.setGeometry (g);
382+ /* clear the saved geometry */
383+ ms.unset();
384+
385+ expected = compiz::window::Geometry (current.width () + 25, 791, 300, 400, 0);
386+
387+ g = ms.sizeAdjustTest (old, current, workArea);
388+
389+ EXPECT_EQ (expected, g);
390+
391+ old = current;
392+ current = CompSize (2048, 1356);
393+ workArea = CompRect (0, 0, current.width (), current.height ());
394+
395+ expected = compiz::window::Geometry (current.width () + 25, 791, 300, 400, 0);
396+
397+ g = ms.sizeAdjustTest (old, current, workArea);
398+
399+ EXPECT_EQ (expected, g);
400+}
401+
402+TEST_F(CompPlaceScreenSizeChangeTestScreenSizeChange, TestScreenChangeWindowsOnPreviousViewport)
403+{
404+ CompSize current, old;
405+ compiz::window::Geometry g (0, 0, 300, 400, 0);
406+ compiz::window::Geometry expected;
407+
408+ MockScreenSizeChangeObject ms (g);
409+
410+ current = CompSize (2048, 1356);
411+
412+ CompRect workArea;
413+
414+ /* Deal with the case where the position is negative, which means
415+ * it's actually wrapped around to the rightmost viewport
416+ */
417+ g.setPos (CompPoint (-300, 200));
418+ ms.setGeometry (g);
419+
420+ expected = g;
421+
422+ /* Unplug the right "monitor" */
423+ old = current;
424+ current = CompSize (1024, 1356);
425+ workArea = CompRect (0, 0, current.width (), current.height ());
426+
427+ g = ms.sizeAdjustTest (old, current, workArea);
428+
429+ EXPECT_EQ (expected, g);
430+
431+ /* Re-plug the right "monitor" */
432+ old = current;
433+ current = CompSize (2048, 1356);
434+ workArea = CompRect (0, 0, current.width (), current.height ());
435+
436+ g = ms.sizeAdjustTest (old, current, workArea);
437+
438+ EXPECT_EQ (expected, g);
439+
440+ /* Move the window to the left monitor, verify that it survives an
441+ * unplug/plug cycle
442+ */
443+ g.setPos (CompPoint (-1324, 200));
444+ ms.setGeometry (g);
445+
446+ old = current;
447+ current = CompSize (1024, 1356);
448+ workArea = CompRect (0, 0, current.width (), current.height ());
449+
450+ expected = compiz::window::Geometry (-300, 200, 300, 400, 0);
451+
452+ g = ms.sizeAdjustTest (old, current, workArea);
453+
454+ EXPECT_EQ (expected, g);
455+
456+ old = current;
457+ current = CompSize (2048, 1356);
458+ workArea = CompRect (0, 0, current.width (), current.height ());
459+
460+ expected = compiz::window::Geometry (-1324, 200, 300, 400, 0);
461+
462+ g = ms.sizeAdjustTest (old, current, workArea);
463+
464+ EXPECT_EQ (expected, g);
465+
466+}

Subscribers

People subscribed via source and target branches