Merge lp:~charon030/compiz-core/fix-751605 into lp:compiz-core

Proposed by Charon on 2012-11-18
Status: Merged
Approved by: Christopher Townsend on 2013-04-15
Approved revision: 3127
Merged at revision: 3135
Proposed branch: lp:~charon030/compiz-core/fix-751605
Merge into: lp:compiz-core
Diff against target: 126 lines (+19/-87)
2 files modified
src/screen.cpp (+5/-22)
src/window.cpp (+14/-65)
To merge this branch: bzr merge lp:~charon030/compiz-core/fix-751605
Reviewer Review Type Date Requested Status
Christopher Townsend 2013-03-22 Approve on 2013-04-15
Daniel van Vugt 2012-11-18 Resubmit on 2012-12-05
Review via email: mp+134814@code.launchpad.net

Commit message

Prevent windows from maximizing on the wrong monitor (LP: #751605)

  This fixes two causes I have found:
    1. outputDeviceForGeometry was wrapping to the wrong monitor (back to the
       top or left) if a window was mostly off the bottom/right of the screen.
    2. Even when outputDeviceForGeometry returned the correct output,
       PrivateWindow::addWindowSizeChanges was sometimes changing it to an
       incorrect output in the case where the old size of a window exceeds
       the dimensions of the smaller monitor you're trying to maximize it on.

Description of the change

I ported the same code changes Daniel van Vugt did in

https://code.launchpad.net/~vanvugt/compiz/fix-751605

to compiz-core (Compiz 0.9.7.x). This will allow users of Ubuntu 12.04 LTS to benefit from Daniel's bugfix.

To post a comment you must log in.
Daniel van Vugt (vanvugt) wrote :

Looks almost perfect, however I notice some whitespace changes on lines 117 and 119 compared to the original fix:
https://code.launchpad.net/~vanvugt/compiz/fix-751605/+merge/134404

Please ensure the whitespace etc are identical to the original fix so we don't encounter conflicts in future.

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote :

Also, I don't want to rush a fix into precise until the fix has been tested for a little while by raring/quantal users. So we should backport to 0.9.8 for quantal first.

It's becoming a common mistake to backport fixes to LTS before they have had real-world testing in the latest release(s) first. As annoying as it is to delay a fix, we should always ensure fixes have had user testing in newer releases first.

Charon (charon030) wrote :

Daniel, thanks for your comments. I wasn't aware of that policy. I will fix the patch with respect to the spaces.

Daniel van Vugt (vanvugt) wrote :

Please re-propose for lp:compiz/0.9.8 instead.

I would rather risk testing it on quantal before it's accepted for precise.

review: Resubmit
Charon (charon030) wrote :

Any chance that this patch could be accepted now? I backported the patch also to 0.9.8 but since then no new release was published. Anyway the patch has been for quite a while in raring.
I know priority is on Mir now. Will there be there still be bugfix releases of compiz in the future?

Christopher Townsend (townsend) wrote :

Hi Charon,

I'm going to see about getting this accepted into lp:compiz-core. That said, I noticed that the original merge had some tests. Can those tests also be backported to this MP?

Seeing as this has been in Raring since late November of last year and the code is still intact, I say otherwise this is good.

Thanks!

review: Needs Information
Charon (charon030) wrote :

Hi Christopher,

thanks for having a look at this. The tests aren't here more or less by intention. See also the comments from Daniel van Vugt when backporting the patch to compiz 0.9.8:

https://code.launchpad.net/~charon030/compiz/fix-751605/+merge/138867

Thanks!

Christopher Townsend (townsend) wrote :

Hi Charon,

Ah, ok, got it! I'm going to go ahead and approve this MP into lp:compiz-core and see about getting it SRU'd for 12.04.

Thanks!

review: Approve
Unity Merger (unity-merger) wrote :

No commit message specified.

Christopher Townsend (townsend) wrote :

I set the commit message and re-approved.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/screen.cpp'
2--- src/screen.cpp 2012-06-20 15:32:11 +0000
3+++ src/screen.cpp 2013-03-20 19:10:25 +0000
4@@ -4095,28 +4095,11 @@
5
6 if (strategy == CoreOptions::OverlappingOutputsSmartMode)
7 {
8- int centerX, centerY;
9-
10- /* for smart mode, calculate the overlap of the whole rectangle
11- with the output device rectangle */
12- geomRect.setWidth (gm.width () + 2 * gm.border ());
13- geomRect.setHeight (gm.height () + 2 * gm.border ());
14-
15- x = gm.x () % width ();
16- centerX = (x + (geomRect.width () / 2));
17- if (centerX < 0)
18- x += width ();
19- else if (centerX > width ())
20- x -= width ();
21- geomRect.setX (x);
22-
23- y = gm.y () % height ();
24- centerY = (y + (geomRect.height () / 2));
25- if (centerY < 0)
26- y += height ();
27- else if (centerY > height ())
28- y -= height ();
29- geomRect.setY (y);
30+ /* We're only going to use geomRect for overlapping area calculations,
31+ so the window rectangle is enough. We don't need to consider
32+ anything more like the border because it will never be significant
33+ to the result */
34+ geomRect = gm;
35 }
36 else
37 {
38
39=== modified file 'src/window.cpp'
40--- src/window.cpp 2012-04-19 03:51:42 +0000
41+++ src/window.cpp 2013-03-20 19:10:25 +0000
42@@ -3781,71 +3781,20 @@
43 * but at least the user will be able to see all of the window */
44 output = &screen->outputDevs ().at (screen->outputDeviceForGeometry (old));
45
46- if (state & CompWindowStateFullscreenMask ||
47- state & CompWindowStateMaximizedHorzMask)
48- {
49- int width = (mask & CWWidth) ? xwc->width : old.width ();
50- int height = (mask & CWHeight) ? xwc->height : old.height ();
51-
52- window->constrainNewWindowSize (width, height, &width, &height);
53-
54- if (width > output->width ())
55- {
56- int distance = std::numeric_limits <int>::max ();
57- CompOutput *selected = output;
58- /* That's no good ... try and find the closest output device to this one
59- * which has a large enough size */
60- foreach (CompOutput &o, screen->outputDevs ())
61- {
62- if (o.workArea ().width () > width)
63- {
64- int tDistance = sqrt (pow (abs (o.x () - output->x ()), 2) +
65- pow (abs (o.y () - output->y ()), 2));
66-
67- if (tDistance < distance)
68- {
69- selected = &o;
70- tDistance = distance;
71- }
72- }
73- }
74-
75- output = selected;
76- }
77- }
78-
79- if (state & CompWindowStateFullscreenMask ||
80- state & CompWindowStateMaximizedVertMask)
81- {
82- int width = (mask & CWWidth) ? xwc->width : old.width ();
83- int height = (mask & CWHeight) ? xwc->height : old.height ();
84-
85- window->constrainNewWindowSize (width, height, &width, &height);
86-
87- if (height > output->height ())
88- {
89- int distance = std::numeric_limits <int>::max ();
90- CompOutput *selected = output;
91- /* That's no good ... try and find the closest output device to this one
92- * which has a large enough size */
93- foreach (CompOutput &o, screen->outputDevs ())
94- {
95- if (o.workArea ().height () > height)
96- {
97- int tDistance = sqrt (pow (abs (o.x () - output->x ()), 2) +
98- pow (abs (o.y () - output->y ()), 2));
99-
100- if (tDistance < distance)
101- {
102- selected = &o;
103- tDistance = distance;
104- }
105- }
106- }
107-
108- output = selected;
109- }
110- }
111+ /*
112+ * output is now the correct output for the given geometry.
113+ * There used to be a lot more logic here to handle the rare special
114+ * case of maximizing a window whose hints say it is too large to fit
115+ * the output and choose a different one. However that logic was a bad
116+ * idea because:
117+ * (1) It's confusing to the user to auto-magically move a window
118+ * between monitors when they didn't ask for it. So don't.
119+ * (2) In the worst case where the window can't go small enough to fit
120+ * the output, they can simply move it with Alt+drag, Alt+F7 or
121+ * expo.
122+ * Not moving the window at all is much less annoying than moving it when
123+ * the user never asked to.
124+ */
125
126 workArea = output->workArea ();
127

Subscribers

People subscribed via source and target branches