Merge lp:~aacid/compiz/do_not_change_viewport_on_resize into lp:compiz/0.9.8

Proposed by Albert Astals Cid on 2012-09-10
Status: Superseded
Proposed branch: lp:~aacid/compiz/do_not_change_viewport_on_resize
Merge into: lp:compiz/0.9.8
Diff against target: 84 lines (+9/-15)
1 file modified
src/window.cpp (+9/-15)
To merge this branch: bzr merge lp:~aacid/compiz/do_not_change_viewport_on_resize
Reviewer Review Type Date Requested Status
Daniel van Vugt 2012-09-10 Resubmit on 2012-09-28
jenkins (community) continuous-integration Approve on 2012-09-10
Compiz Maintainers 2012-09-19 Pending
Review via email: mp+123564@code.launchpad.net

This proposal has been superseded by a proposal from 2012-09-28.

Commit message

Do not move windows between viewports/workspaces when changing its size (e.g. maximizing)

Description of the change

Do not move windows between viewports/workspaces when changing its size (e.g. maximizing)

To be honest i'm a bit "scared" since this touches a function used by lots of other functions, but if we never want a window to change to a different viewports/workspaces i guess it makes sense.

To post a comment you must log in.
jenkins (martin-mrazik+qa) wrote :
review: Approve (continuous-integration)
Sam Spilsbury (smspillaz) wrote :

Hmm, I think the intention of this code is to prevent windows that maximize themselves from going on to other viewports. I think it might make sense to add a check here to check if the window is on the currently active viewport, and if so, to always maximize it on that viewport.

Do you think you might be able to get this code under test?

Daniel van Vugt (vanvugt) wrote :

Actually, I'm not sure about the validity of the bug itself. The bug seems to be that windows get maximized on the workspace which contains their centre-point. That's the way it was designed and the way it works. But that's not what's always intuitive to the user and hence the bug.

If that's the functionality we're aiming to change here then it looks like the wrong code is being modified.

Daniel van Vugt (vanvugt) wrote :

I think you should look at this instead:

screen.cpp: compiz::private_screen::viewports::viewportForGeometry

review: Needs Fixing
Albert Astals Cid (aacid) wrote :

Daniel, right, the code is doign what you say (maximize on the workspace which contains their centre-point) which both I and Sam (that set the bug to Confirmed) think does not make sense.

I disagree the wrong code is being modified, what
   CompScreenImpl::viewportForGeometry (const CompWindow::Geometry& gm, CompPoint& viewport)
does is not return in viewport the viewport that gm should have but the "difference" in viewports from the current one, i.e. it can return {-1, -1} if it thinks the window shall be one viewport left and one top from its current one.

Of course I could make that function return always {0,0} (i.e. don't move me) but that might potentially break other plugins that use it like the group and the wall plugins

Daniel van Vugt (vanvugt) wrote :

OK, I have no opinion until I can review this in detail later.

review: Abstain
Albert Astals Cid (aacid) wrote :

FWIW: I'm going on holiday tomorrow, will be back on 1st October

Daniel van Vugt (vanvugt) wrote :

The branches have changed. Please resubmit targeting lp:compiz

review: Resubmit
3363. By Albert Astals Cid on 2012-10-10

Merge lp:compiz

Unmerged revisions

3363. By Albert Astals Cid on 2012-10-10

Merge lp:compiz

3362. By Albert Astals Cid on 2012-09-10

Do not move windows between viewports/workspaces when changing its size (e.g. maximizing)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/window.cpp'
2--- src/window.cpp 2012-08-30 01:08:48 +0000
3+++ src/window.cpp 2012-09-10 14:43:28 +0000
4@@ -3497,15 +3497,9 @@
5 {
6 CompRect workArea;
7 int mask = 0;
8- int x, y;
9 CompOutput *output;
10 CompPoint viewport;
11
12- screen->viewportForGeometry (old, viewport);
13-
14- x = (viewport.x () - screen->vp ().x ()) * screen->width ();
15- y = (viewport.y () - screen->vp ().y ()) * screen->height ();
16-
17 /* Try to select and output device that the window is on first
18 * and make sure if we are fullscreening or maximizing that the
19 * window is actually able to fit on this output ... otherwise
20@@ -3587,15 +3581,15 @@
21
22 if (fullscreenMonitorsSet)
23 {
24- xwc->x = x + fullscreenMonitorRect.x ();
25- xwc->y = y + fullscreenMonitorRect.y ();
26+ xwc->x = fullscreenMonitorRect.x ();
27+ xwc->y = fullscreenMonitorRect.y ();
28 xwc->width = fullscreenMonitorRect.width ();
29 xwc->height = fullscreenMonitorRect.height ();
30 }
31 else
32 {
33- xwc->x = x + output->x ();
34- xwc->y = y + output->y ();
35+ xwc->x = output->x ();
36+ xwc->y = output->y ();
37 xwc->width = output->width ();
38 xwc->height = output->height ();
39 }
40@@ -3697,7 +3691,7 @@
41 * by the gravity value (so that the corner that the gravity specifies
42 * is 'anchored' to that edge of the workarea) */
43
44- xwc->y = y + workArea.y () + border.top;
45+ xwc->y = workArea.y () + border.top;
46 mask |= CWY;
47
48 switch (priv->sizeHints.win_gravity)
49@@ -3708,7 +3702,7 @@
50 /* Shift the window so that the bottom meets the top of the bottom */
51 height = xwc->height + old.border () * 2;
52
53- max = y + workArea.bottom ();
54+ max = workArea.bottom ();
55 if (xwc->y + xwc->height + border.bottom > max)
56 {
57 xwc->y = max - height - border.bottom;
58@@ -3734,7 +3728,7 @@
59
60 if (state & CompWindowStateMaximizedHorzMask)
61 {
62- xwc->x = x + workArea.x () + border.left;
63+ xwc->x = workArea.x () + border.left;
64 mask |= CWX;
65
66 switch (priv->sizeHints.win_gravity)
67@@ -3744,7 +3738,7 @@
68 case EastGravity:
69 width = xwc->width + old.border () * 2;
70
71- max = x + workArea.right ();
72+ max = workArea.right ();
73
74 if (old.x () + (int) old.width () + border.right > max)
75 {
76@@ -3753,7 +3747,7 @@
77 }
78 else if (old.x () + width + border.right > max)
79 {
80- xwc->x = x + workArea.x () +
81+ xwc->x = workArea.x () +
82 (workArea.width () - border.left - width -
83 border.right) / 2 + border.left;
84 mask |= CWX;

Subscribers

People subscribed via source and target branches