Merge lp:~smspillaz/compiz-core/compiz-core.fix_939228 into lp:compiz-core/0.9.8

Proposed by Sam Spilsbury on 2012-05-17
Status: Superseded
Proposed branch: lp:~smspillaz/compiz-core/compiz-core.fix_939228
Merge into: lp:compiz-core/0.9.8
Diff against target: 294 lines (+221/-14)
3 files modified
src/privatescreen.h (+30/-4)
src/privatescreen/tests/test-privatescreen.cpp (+166/-0)
src/screen.cpp (+25/-10)
To merge this branch: bzr merge lp:~smspillaz/compiz-core/compiz-core.fix_939228
Reviewer Review Type Date Requested Status
Daniel van Vugt 2012-05-17 Resubmit on 2012-05-17
Alan Griffiths 2012-05-17 Pending
Review via email: mp+106116@code.launchpad.net

This proposal supersedes a proposal from 2012-05-15.

This proposal has been superseded by a proposal from 2012-05-18.

Description of the change

== Problem ==

See LP#939288 - windows would jump if you had them on screen edges. Add tests for viewport picking behaviour.

== Solution ==

Don't return negative values for CompScreen::viewportForGeometry

== Test ==

Included.

To post a comment you must log in.
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

why change "class" to "struct" when it behaves like a struct?

Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

LGTM

review: Approve
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

The fix works if I follow the test case that I just documented in bug 939228. However the opposite way around is still broken;

1. Move a window to the far left workspace so that it overlaps the left side of the screen a little.
2. Switch to the far right workspace where you can see a little of the overlapping window.
3. Alt-Tab to the window.

Observed: Window jumps to being entirely on the far left workspace.
Expected: Window stays where it was.

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

Hi,

I am unable to reproduce the issue you mentioned. Are you running with lp:~smspillaz/compiz-wall-plugin/compiz-wall-plugin.fix_939228 ?

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

conflicts fixed.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Deduplicated bugs. This is now bug 755842.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

If the other branch is a prereq, can we mark it as such? (even though it's a different project)

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

> conflicts fixed.

But not pushed to LP yet. The conflict is still there :)
Though it's simple enough for anyone to fix that I won't say Needs Fixing.

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

Hum, I'll try again

Alan Griffiths (alan-griffiths) : Posted in a previous version of this proposal
review: Approve
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

I'm still trying to test the prerequisite wall fix, but am delayed by new cmake/precise bugs.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Contains a few nontrivial conflicts. Please fix.

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Needs Information:
As far as I can tell, the bug is already fixed by the prerequisite wall fix, which is now committed. I can't reproduce the bug any more so why do we need to complicate viewportForGeometry or change it at all?

Needs Fixing:
This branch introduces odd viewport bugs where windows suddenly change the viewport they're on when you select (zoom into) a new viewport using expo. lp:compiz-core has no such bugs.

So the bug doesn't exist any more, and this branch introduces a worse one.

If you would still like to submit this to keep the test cases, please:
1. Fix the regression; and
2. Disassociate the branch from the bug numbers.

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

If the wall bug is fixed, then I guess it makes sense to keep the old behaviour and keep the tests. I'll resubmit to that end.

Daniel van Vugt (vanvugt) wrote :

The target branch is now retired. Please resubmit for: lp:compiz/0.9.8

(and ideally please remove references to bug numbers in the new proposal)

review: Resubmit

Unmerged revisions

3116. By Sam Spilsbury on 2012-05-17

Revert to the old behaviour with a note about possible bugs

3115. By Sam Spilsbury on 2012-05-15

Merged lp:compiz-core

3114. By Sam Spilsbury on 2012-05-15

Merge lp:compiz-core

3113. By Sam Spilsbury on 2012-04-27

Merge lp:compiz-core

3112. By Sam Spilsbury on 2012-04-24

s/class/struct/

3111. By Sam Spilsbury on 2012-04-24

Don't return a negative value for CompScreen::viewportForGeometry

Fix LP#939288 - If a window was partially on a viewport which was the last
one in its row or column, and we were on the viewport on the opposite side
of that row or column, compiz would work backwards to find the position
of the window (which would be negative) and return a negative viewport
position. This caused plugins like wall to behave strangely

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/privatescreen.h'
2--- src/privatescreen.h 2012-05-10 12:58:01 +0000
3+++ src/privatescreen.h 2012-05-17 05:48:18 +0000
4@@ -440,15 +440,41 @@
5 unsigned int activeNum;
6 };
7
8+class ViewportRetrievalInterface
9+{
10+ public:
11+
12+ virtual ~ViewportRetrievalInterface () {}
13+
14+ virtual const CompPoint & getCurrentViewport () const = 0;
15+ virtual const CompSize & viewportDimentions () const = 0;
16+};
17+
18 // Apart from a use by StartupSequence::addSequence this data
19 // is only used by CompScreenImpl - like the OrphanData struct
20-struct ViewPort
21+struct ViewPort :
22+ public ViewportRetrievalInterface
23 {
24- ViewPort();
25- CompPoint vp;
26- CompSize vpSize;
27+ public:
28+
29+ ViewPort();
30+ CompPoint vp;
31+ CompSize vpSize;
32+
33+ private:
34+
35+ const CompPoint & getCurrentViewport () const { return vp; }
36+ const CompSize & viewportDimentions () const { return vpSize; }
37 };
38
39+namespace viewports
40+{
41+ void viewportForGeometry (const CompWindow::Geometry &gm,
42+ CompPoint &viewport,
43+ ViewportRetrievalInterface *viewports,
44+ const CompSize &screenSize);
45+}
46+
47 class StartupSequence : boost::noncopyable
48 {
49 public:
50
51=== modified file 'src/privatescreen/tests/test-privatescreen.cpp'
52--- src/privatescreen/tests/test-privatescreen.cpp 2012-05-10 11:30:52 +0000
53+++ src/privatescreen/tests/test-privatescreen.cpp 2012-05-17 05:48:18 +0000
54@@ -10,6 +10,10 @@
55
56 #include <stdlib.h>
57
58+using ::testing::Return;
59+using ::testing::ReturnRef;
60+using ::testing::_;
61+
62 namespace {
63
64 class MockCompScreen : public CompScreen
65@@ -202,6 +206,15 @@
66 MOCK_METHOD1(getWindowState, unsigned int (Window id));
67 };
68
69+class MockViewportRetreival :
70+ public compiz::private_screen::ViewportRetrievalInterface
71+{
72+ public:
73+
74+ MOCK_CONST_METHOD0(getCurrentViewport, const CompPoint & ());
75+ MOCK_CONST_METHOD0(viewportDimentions, const CompSize & ());
76+};
77+
78 class StubActivePluginsOption : public CoreOptions
79 {
80 public:
81@@ -751,3 +764,156 @@
82 CoreOptions coreOptions(false);
83 em.init();
84 }
85+
86+TEST(privatescreen_ViewportGeometryTest, PickCurrent)
87+{
88+ CompPoint vp;
89+ compiz::window::Geometry g (250, 250, 500, 500, 0);
90+ MockViewportRetreival mvp;
91+
92+ CompPoint current (0, 0);
93+ CompSize dimentions (1, 1);
94+
95+ EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
96+ EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
97+
98+ compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
99+
100+ EXPECT_EQ (vp, CompPoint (0, 0));
101+}
102+
103+TEST(privatescreen_ViewportGeometryTest, PickRight)
104+{
105+ CompPoint vp;
106+ compiz::window::Geometry g (1250, 0, 500, 500, 0);
107+ MockViewportRetreival mvp;
108+
109+ CompPoint current (0, 0);
110+ CompSize dimentions (2, 1);
111+
112+ EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
113+ EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
114+
115+ compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
116+
117+ EXPECT_EQ (vp, CompPoint (1, 0));
118+}
119+
120+TEST(privatescreen_ViewportGeometryTest, PickLeft)
121+{
122+ CompPoint vp;
123+ compiz::window::Geometry g (-750, 0, 500, 500, 0);
124+ MockViewportRetreival mvp;
125+
126+ CompPoint current (1, 0);
127+ CompSize dimentions (2, 1);
128+
129+ EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
130+ EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
131+
132+ compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
133+
134+ EXPECT_EQ (vp, CompPoint (0, 0));
135+}
136+
137+TEST(privatescreen_ViewportGeometryTest, PickBottom)
138+{
139+ CompPoint vp;
140+ compiz::window::Geometry g (0, 1250, 500, 500, 0);
141+ MockViewportRetreival mvp;
142+
143+ CompPoint current (0, 0);
144+ CompSize dimentions (1, 2);
145+
146+ EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
147+ EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
148+
149+ compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
150+
151+ EXPECT_EQ (vp, CompPoint (0, 1));
152+}
153+
154+TEST(privatescreen_ViewportGeometryTest, PickTop)
155+{
156+ CompPoint vp;
157+ compiz::window::Geometry g (0, -750, 500, 500, 0);
158+ MockViewportRetreival mvp;
159+
160+ CompPoint current (0, 1);
161+ CompSize dimentions (1, 2);
162+
163+ EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
164+ EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
165+
166+ compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
167+
168+ EXPECT_EQ (vp, CompPoint (0, 0));
169+}
170+
171+TEST(privatescreen_ViewportGeometryTest, PickTopWhenJustAbove)
172+{
173+ CompPoint vp;
174+ compiz::window::Geometry g (0, -251, 500, 500, 0);
175+ MockViewportRetreival mvp;
176+
177+ CompPoint current (0, 1);
178+ CompSize dimentions (1, 2);
179+
180+ EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
181+ EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
182+
183+ compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
184+
185+ EXPECT_EQ (vp, CompPoint (0, 0));
186+}
187+
188+TEST(privatescreen_ViewportGeometryTest, PickRightWhenJustRight)
189+{
190+ CompPoint vp;
191+ compiz::window::Geometry g (751, 0, 500, 500, 0);
192+ MockViewportRetreival mvp;
193+
194+ CompPoint current (0, 0);
195+ CompSize dimentions (2, 1);
196+
197+ EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
198+ EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
199+
200+ compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
201+
202+ EXPECT_EQ (vp, CompPoint (1, 0));
203+}
204+
205+TEST(privatescreen_ViewportGeometryTest, PickLeftWhenJustLeft)
206+{
207+ CompPoint vp;
208+ compiz::window::Geometry g (-251, 0, 500, 500, 0);
209+ MockViewportRetreival mvp;
210+
211+ CompPoint current (1, 0);
212+ CompSize dimentions (2, 1);
213+
214+ EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
215+ EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
216+
217+ compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
218+
219+ EXPECT_EQ (vp, CompPoint (0, 0));
220+}
221+
222+TEST(privatescreen_ViewportGeometryTest, PickBottomWhenJustBelow)
223+{
224+ CompPoint vp;
225+ compiz::window::Geometry g (0, 751, 500, 500, 0);
226+ MockViewportRetreival mvp;
227+
228+ CompPoint current (0, 0);
229+ CompSize dimentions (1, 2);
230+
231+ EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
232+ EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
233+
234+ compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
235+
236+ EXPECT_EQ (vp, CompPoint (0, 1));
237+}
238
239=== modified file 'src/screen.cpp'
240--- src/screen.cpp 2012-05-10 11:37:42 +0000
241+++ src/screen.cpp 2012-05-17 05:48:18 +0000
242@@ -4135,27 +4135,42 @@
243 }
244
245 /* Returns default viewport for some window geometry. If the window spans
246- more than one viewport the most appropriate viewport is returned. How the
247- most appropriate viewport is computed can be made optional if necessary. It
248- is currently computed as the viewport where the center of the window is
249- located. */
250+ * more than one viewport the most appropriate viewport is returned. How the
251+ * most appropriate viewport is computed can be made optional if necessary. It
252+ * is currently computed as the viewport where the center of the window is
253+ * located.
254+ *
255+ * XXX: It is possible for this function to return a negative viewport, which
256+ * definitely feels wrong, however it seems that some plugins depend on this behaviour
257+ * so they need to be fixed first
258+ */
259 void
260-CompScreenImpl::viewportForGeometry (const CompWindow::Geometry& gm,
261- CompPoint& viewport)
262+compiz::private_screen::viewports::viewportForGeometry (const CompWindow::Geometry &gm,
263+ CompPoint &viewport,
264+ ViewportRetrievalInterface *viewports,
265+ const CompSize & screenSize)
266 {
267 CompRect rect (gm);
268 int offset;
269
270+ const CompPoint &vp = viewports->getCurrentViewport ();
271+ const CompSize &vpSize = viewports->viewportDimentions ();
272+
273 rect.setWidth (gm.widthIncBorders ());
274 rect.setHeight (gm.heightIncBorders ());
275
276 offset = rect.centerX () < 0 ? -1 : 0;
277- viewport.setX (privateScreen.viewPort.vp.x () + ((rect.centerX () / width ()) + offset) %
278- privateScreen.viewPort.vpSize.width ());
279+ viewport.setX (vp.x () + ((rect.centerX () / screenSize.width ()) + offset) % vpSize.width ());
280
281 offset = rect.centerY () < 0 ? -1 : 0;
282- viewport.setY (privateScreen.viewPort.vp.y () + ((rect.centerY () / height ()) + offset ) %
283- privateScreen.viewPort.vpSize.height ());
284+ viewport.setY (vp.y () + ((rect.centerY () / screenSize.height ()) + offset ) % vpSize.height ());
285+}
286+
287+void
288+CompScreenImpl::viewportForGeometry (const CompWindow::Geometry& gm,
289+ CompPoint& viewport)
290+{
291+ compiz::private_screen::viewports::viewportForGeometry (gm, privateScreen.viewPort.vp, &privateScreen.viewPort, *this);
292 }
293
294 int

Subscribers

People subscribed via source and target branches