Compiz Core

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

  1. fix-751605
  2. Merge into 0.9.7
Proposed by Charon
Status: Merged
Approved by: Christopher Townsend
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 Approve
Daniel van Vugt Needs Resubmitting
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.
Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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: Needs Resubmitting
Revision history for this message
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?

Revision history for this message
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
Revision history for this message
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!

Revision history for this message
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
Revision history for this message
Unity Merger (unity-merger) wrote :

No commit message specified.

Revision history for this message
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
=== modified file 'src/screen.cpp'
--- src/screen.cpp 2012-06-20 15:32:11 +0000
+++ src/screen.cpp 2013-03-20 19:10:25 +0000
@@ -4095,28 +4095,11 @@
40954095
4096 if (strategy == CoreOptions::OverlappingOutputsSmartMode)4096 if (strategy == CoreOptions::OverlappingOutputsSmartMode)
4097 {4097 {
4098 int centerX, centerY;4098 /* We're only going to use geomRect for overlapping area calculations,
40994099 so the window rectangle is enough. We don't need to consider
4100 /* for smart mode, calculate the overlap of the whole rectangle4100 anything more like the border because it will never be significant
4101 with the output device rectangle */4101 to the result */
4102 geomRect.setWidth (gm.width () + 2 * gm.border ());4102 geomRect = gm;
4103 geomRect.setHeight (gm.height () + 2 * gm.border ());
4104
4105 x = gm.x () % width ();
4106 centerX = (x + (geomRect.width () / 2));
4107 if (centerX < 0)
4108 x += width ();
4109 else if (centerX > width ())
4110 x -= width ();
4111 geomRect.setX (x);
4112
4113 y = gm.y () % height ();
4114 centerY = (y + (geomRect.height () / 2));
4115 if (centerY < 0)
4116 y += height ();
4117 else if (centerY > height ())
4118 y -= height ();
4119 geomRect.setY (y);
4120 }4103 }
4121 else4104 else
4122 {4105 {
41234106
=== modified file 'src/window.cpp'
--- src/window.cpp 2012-04-19 03:51:42 +0000
+++ src/window.cpp 2013-03-20 19:10:25 +0000
@@ -3781,71 +3781,20 @@
3781 * but at least the user will be able to see all of the window */3781 * but at least the user will be able to see all of the window */
3782 output = &screen->outputDevs ().at (screen->outputDeviceForGeometry (old));3782 output = &screen->outputDevs ().at (screen->outputDeviceForGeometry (old));
37833783
3784 if (state & CompWindowStateFullscreenMask ||3784 /*
3785 state & CompWindowStateMaximizedHorzMask)3785 * output is now the correct output for the given geometry.
3786 {3786 * There used to be a lot more logic here to handle the rare special
3787 int width = (mask & CWWidth) ? xwc->width : old.width ();3787 * case of maximizing a window whose hints say it is too large to fit
3788 int height = (mask & CWHeight) ? xwc->height : old.height ();3788 * the output and choose a different one. However that logic was a bad
37893789 * idea because:
3790 window->constrainNewWindowSize (width, height, &width, &height);3790 * (1) It's confusing to the user to auto-magically move a window
37913791 * between monitors when they didn't ask for it. So don't.
3792 if (width > output->width ())3792 * (2) In the worst case where the window can't go small enough to fit
3793 {3793 * the output, they can simply move it with Alt+drag, Alt+F7 or
3794 int distance = std::numeric_limits <int>::max ();3794 * expo.
3795 CompOutput *selected = output;3795 * Not moving the window at all is much less annoying than moving it when
3796 /* That's no good ... try and find the closest output device to this one3796 * the user never asked to.
3797 * which has a large enough size */3797 */
3798 foreach (CompOutput &o, screen->outputDevs ())
3799 {
3800 if (o.workArea ().width () > width)
3801 {
3802 int tDistance = sqrt (pow (abs (o.x () - output->x ()), 2) +
3803 pow (abs (o.y () - output->y ()), 2));
3804
3805 if (tDistance < distance)
3806 {
3807 selected = &o;
3808 tDistance = distance;
3809 }
3810 }
3811 }
3812
3813 output = selected;
3814 }
3815 }
3816
3817 if (state & CompWindowStateFullscreenMask ||
3818 state & CompWindowStateMaximizedVertMask)
3819 {
3820 int width = (mask & CWWidth) ? xwc->width : old.width ();
3821 int height = (mask & CWHeight) ? xwc->height : old.height ();
3822
3823 window->constrainNewWindowSize (width, height, &width, &height);
3824
3825 if (height > output->height ())
3826 {
3827 int distance = std::numeric_limits <int>::max ();
3828 CompOutput *selected = output;
3829 /* That's no good ... try and find the closest output device to this one
3830 * which has a large enough size */
3831 foreach (CompOutput &o, screen->outputDevs ())
3832 {
3833 if (o.workArea ().height () > height)
3834 {
3835 int tDistance = sqrt (pow (abs (o.x () - output->x ()), 2) +
3836 pow (abs (o.y () - output->y ()), 2));
3837
3838 if (tDistance < distance)
3839 {
3840 selected = &o;
3841 tDistance = distance;
3842 }
3843 }
3844 }
3845
3846 output = selected;
3847 }
3848 }
38493798
3850 workArea = output->workArea ();3799 workArea = output->workArea ();
38513800

Subscribers

People subscribed via source and target branches