Merge lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic into lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken
- compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic
- Merge into compiz.merge-fix771448-des...
Status: | Superseded |
---|---|
Proposed branch: | lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic |
Merge into: | lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken |
Diff against target: |
332 lines (+111/-62) 5 files modified
plugins/wall/src/wall.cpp (+49/-56) plugins/wall/src/wall.h (+2/-4) plugins/wall/wall.xml.in (+4/-1) src/screen.cpp (+7/-1) tests/manual/plugins/wall.txt (+49/-0) |
To merge this branch: | bzr merge lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
MC Return | Needs Information | ||
Review via email: mp+161279@code.launchpad.net |
This proposal has been superseded by a proposal from 2013-05-13.
Commit message
wall: Remove mouse polling edge flip detection code, instead turn edges on
in the following situations:
1. Edge flip pointer is on (always on)
2. Edge flip move is on, and the screen is grabbed (someone is moving
a window)
3. Edge flip dnd is on, and a dnd type window is mapped (someone is
doing a dnd)
Fix a misconstructed boolean condition in removeAction that could cause
actions to be removed twice and as such cause the edge reference count to
go negative.
Note that dnd window detection is a bit imprecise, but I didn't want to resort
to using some exotic workarounds for the dnd edge cases (such as non-EWMH
compliant dnd windows which are still compliant with ICCCM).
Description of the change
wall: Remove mouse polling edge flip detection code, instead turn edges on
in the following situations:
1. Edge flip pointer is on (always on)
2. Edge flip move is on, and the screen is grabbed (someone is moving
a window)
3. Edge flip dnd is on, and a dnd type window is mapped (someone is
doing a dnd)
Fix a misconstructed boolean condition in removeAction that could cause
actions to be removed twice and as such cause the edge reference count to
go negative.
Note that dnd window detection is a bit imprecise, but I didn't want to resort
to using some exotic workarounds for the dnd edge cases (such as non-EWMH
compliant dnd windows which are still compliant with ICCCM)
MC Return (mc-return) wrote : | # |
MC Return (mc-return) wrote : | # |
Did you test this ?
It does not seem to work here, none of the edge-flipping modes... :(
MC Return (mc-return) wrote : | # |
Or am I missing something ?
Ah -> probably I am missing this part:
260 - if (!(privateScree
261 + if (!privateScreen
262 + !action->active ())
hmm, but this change seems to be unrelated and does not change the logic ?!
(I just exchanged the plugin)
*clueless* at the moment
MC Return (mc-return) wrote : | # |
Or is it this:
292 + and set up screen edges as appropriate
What exactly should I setup here and where ?
MC Return (mc-return) wrote : | # |
But I have some good news as well:
I did not have to change anything regarding the cmake config to compile this MP, just cmake .. && make -j5 :)
This means for the first time ever here all the tests compiled :), which means recent trunk fixes seem to work :)
Sam Spilsbury (smspillaz) wrote : | # |
Yes I did, it works fine here.
Its possible that when I took the patch off my main branch I may have
missed something. I can look into that.
On 27/04/2013 4:42 PM, "MC Return" <email address hidden> wrote:
> Did you test this ?
>
> It does not seem to work here, none of the edge-flipping modes... :(
> --
>
> https:/
> You are the owner of
> lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic.
>
MC Return (mc-return) wrote : | # |
> Yes I did, it works fine here.
>
> Its possible that when I took the patch off my main branch I may have
> missed something. I can look into that.
Thanks :)
I am pretty sure I made no mistake -> and I double-checked, so most probably
something's missing...
MC Return (mc-return) wrote : | # |
Note: I just took the compiled libwall.so to test, so if this needs changes to core...
But this thing below seems unrelated and does not change anything, or does it ?
253 === modified file 'src/screen.cpp'
254 --- src/screen.cpp 2013-02-03 17:58:29 +0000
255 +++ src/screen.cpp 2013-04-27 06:17:25 +0000
256 @@ -3486,7 +3486,8 @@
257 void
258 CompScreenImpl:
259 {
260 - if (!(privateScree
261 + if (!privateScreen
262 + !action->active ())
263 return;
264
265 grabManager.
266 @@ -3902,6 +3903,11 @@
267 SubstructureRed
268 }
269
270 +/* These functions do not guard against negative decrements
271 + * as they are unable to determine the source of the reference
272 + * and as such they should only be called by functions that
273 + * actually determine the source of what was referencing the
274 + * edge and can guard against multiple-
275 void
276 PrivateScreen:
277 {
278
Sam Spilsbury (smspillaz) wrote : | # |
In testing I remember coming across something that would effectively
cause actions to be removed from the active actions list multiple
times, which would in term cause the screen edge reference count to go
below zero. The if condition was the problem. It read:
if NOT
(privateScre
action->active ())
return false;
The sub-condition is true if either privateScreen.
action->active () is true. This means that in order for it to be
false, both of them must be false. Eg,
!(A || B) -> !A && !B
That's incorrect, because privateScreen.
true, so you'd run into situations like this:
A && !B
But that still satisfies A || B because A is still true.
The adjusted condition is correct:
if (!A || !B) -> !(A && B)
What we want is for the function to always return false if
action->active () is false, regardless of what the value of
privateScreen.
On Sat, Apr 27, 2013 at 5:43 PM, MC Return <email address hidden> wrote:
> Note: I just took the compiled libwall.so to test, so if this needs changes to core...
>
> But this thing below seems unrelated and does not change anything, or does it ?
>
> 253 === modified file 'src/screen.cpp'
> 254 --- src/screen.cpp 2013-02-03 17:58:29 +0000
> 255 +++ src/screen.cpp 2013-04-27 06:17:25 +0000
> 256 @@ -3486,7 +3486,8 @@
> 257 void
> 258 CompScreenImpl:
> 259 {
> 260 - if (!(privateScree
> 261 + if (!privateScreen
> 262 + !action->active ())
> 263 return;
> 264
> 265 grabManager.
> 266 @@ -3902,6 +3903,11 @@
> 267 SubstructureRed
> 268 }
> 269
> 270 +/* These functions do not guard against negative decrements
> 271 + * as they are unable to determine the source of the reference
> 272 + * and as such they should only be called by functions that
> 273 + * actually determine the source of what was referencing the
> 274 + * edge and can guard against multiple-
> 275 void
> 276 PrivateScreen:
> 277 {
> 278
>
> --
> https:/
> You are the owner of lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic.
--
Sam Spilsbury
MC Return (mc-return) wrote : | # |
> In testing I remember coming across something that would effectively
> cause actions to be removed from the active actions list multiple
> times, which would in term cause the screen edge reference count to go
> below zero. The if condition was the problem. It read:
>
> if NOT
> (privateScreen.
> action->active ())
> return false;
>
> The sub-condition is true if either privateScreen.
> action->active () is true. This means that in order for it to be
> false, both of them must be false. Eg,
>
> !(A || B) -> !A && !B
>
> That's incorrect, because privateScreen.
> true, so you'd run into situations like this:
>
> A && !B
>
> But that still satisfies A || B because A is still true.
>
> The adjusted condition is correct:
>
> if (!A || !B) -> !(A && B)
>
> What we want is for the function to always return false if
> action->active () is false, regardless of what the value of
> privateScreen.
>
>
Aha -> I thought this might be the problem, but was too lazy to really rethink
the boolean logic (I am still missing my shower & coffee today ;))...
I will approve this blindly this time, because I hope and am sure you will
immediately take care of regressions should those occur...
Sam Spilsbury (smspillaz) wrote : | # |
Can you verify it please?
On Sat, Apr 27, 2013 at 6:06 PM, MC Return <email address hidden> wrote:
> Review: Approve
>
>> In testing I remember coming across something that would effectively
>> cause actions to be removed from the active actions list multiple
>> times, which would in term cause the screen edge reference count to go
>> below zero. The if condition was the problem. It read:
>>
>> if NOT
>> (privateScreen.
>> action->active ())
>> return false;
>>
>> The sub-condition is true if either privateScreen.
>> action->active () is true. This means that in order for it to be
>> false, both of them must be false. Eg,
>>
>> !(A || B) -> !A && !B
>>
>> That's incorrect, because privateScreen.
>> true, so you'd run into situations like this:
>>
>> A && !B
>>
>> But that still satisfies A || B because A is still true.
>>
>> The adjusted condition is correct:
>>
>> if (!A || !B) -> !(A && B)
>>
>> What we want is for the function to always return false if
>> action->active () is false, regardless of what the value of
>> privateScreen.
>>
>>
> Aha -> I thought this might be the problem, but was too lazy to really rethink
> the boolean logic (I am still missing my shower & coffee today ;))...
>
> I will approve this blindly this time, because I hope and am sure you will
> immediately take care of regressions should those occur...
>
>
> --
> https:/
> You are the owner of lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic.
--
Sam Spilsbury
MC Return (mc-return) wrote : | # |
> Can you verify it please?
>
If you ask me to, how can I say no ?
;)
But it'll take a few hours until I can get to it...
MC Return (mc-return) wrote : | # |
I get this error, when trying to install this branch locally (probably not related to changes in this branch):
...
copying build/locale/
copying build/locale/
running install_egg_info
Removing /home/mcr2010/
Writing /home/mcr2010/
gtk-update-
Updating Gtk icon cache.
CMake Error at compizconfig/
file cannot create directory: /compizconfig/
administrative privileges.
Call Stack (most recent call first):
compizconfig/
cmake_
make: *** [install] Error 1
I do not know why I cannot install locally, but I do not want to try to sudo it through...
Installing this way worked not long ago, recently I was not forced to do it, because I was just replacing the compiled plugin binaries manually to test them...
MC Return (mc-return) wrote : | # |
Sam, I got this report (quote by bryonak, who is helping with testing):
"
@MC Return:
Sadly, this one doesn't work well. Window move and pointer flipping work flawlessly, but d&d has some issues. Moreover, after trying to d&d, the other two stop working. Here's a video of it: http://
"
Sam Spilsbury (smspillaz) wrote : | # |
Did the other tester also install the core patch or just the wall plugin.
The core patch is necessary to prevent he bug which the tester has referred
to.
On 28/04/2013 11:07 PM, "MC Return" <email address hidden> wrote:
> Sam, I got this report (quote by bryonak, who is helping with testing):
> "
> @MC Return:
> Sadly, this one doesn't work well. Window move and pointer flipping work
> flawlessly, but d&d has some issues. Moreover, after trying to d&d, the
> other two stop working. Here's a video of it:
> http://
> "
> --
>
> https:/
> You are the owner of
> lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic.
>
MC Return (mc-return) wrote : | # |
Sam, could you merge trunk into this branch ? -> then I could test it myself as IIRC the bug I had above is already fixed in trunk...
MC Return (mc-return) wrote : | # |
Forget it -> (I could do it locally myself)
Sam Spilsbury (smspillaz) wrote : | # |
Ping.
MC Return (mc-return) wrote : | # |
> Ping.
Sorry for the delay -> it is due to the core-changing nature of this MP.
I still had no chance to test this properly... :(
My suggestion would be this, to not hold up this any longer:
First we merge lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken into lp:compiz, then this branch here afterwards.
Currently I have this PPA running here locally: gebner/compiz-cms
It correctly updates and has the latest Compiz core from trunk in it.
I could then run the latest version of trunk here and would spot regressions or non-functionality of wall then. In the worst case we could revert this branch, which probably won't be necessary anyway...
How about this plan ?
Sam Spilsbury (smspillaz) wrote : | # |
On Sat, May 11, 2013 at 5:07 PM, MC Return <email address hidden> wrote:
> Review: Needs Information
>
>> Ping.
>
> Sorry for the delay -> it is due to the core-changing nature of this MP.
> I still had no chance to test this properly... :(
> My suggestion would be this, to not hold up this any longer:
> ...
> Currently I have this PPA running here locally: gebner/compiz-cms
> It correctly updates and has the latest Compiz core from trunk in it.
> I could then run the latest version of trunk here and would spot regressions or non-functionality of wall then. In the worst case we could revert this branch, which probably won't be necessary anyway...
Huh?
What's stopping you from compiling compiz and unity locally?
The reason why I proposed this one is because
lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken
breaks edge-flip-dnd.
--
Sam Spilsbury
MC Return (mc-return) wrote : | # |
>
> Huh?
>
> What's stopping you from compiling compiz and unity locally?
>
My additionally installed lenses/scopes, Compiz plugins and special CCSM settings.
> The reason why I proposed this one is because
> lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken
> breaks edge-flip-dnd.
>
For maximally 3 hours until your branch lands and just in trunk, while fixing all
those other things ;)
Come on, now you must be joking...
Sam Spilsbury (smspillaz) wrote : | # |
> >
> > Huh?
> >
> > What's stopping you from compiling compiz and unity locally?
> >
> My additionally installed lenses/scopes, Compiz plugins and special CCSM
> settings.
>
> > The reason why I proposed this one is because
> > lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-
> broken
> > breaks edge-flip-dnd.
> >
> For maximally 3 hours until your branch lands and just in trunk, while fixing
> all
> those other things ;)
> Come on, now you must be joking...
No, I'm not.
1. I know its broken. It would be unconscionable for me to land something that I know is broken.
2. Landing this doesn't change the fact that you can't yet test the branch that fixes it because it requires a core change.
- 3679. By Sam Spilsbury
-
Merge lp:compiz
- 3680. By Sam Spilsbury
-
Merge from upstream
Unmerged revisions
Preview Diff
1 | === modified file 'plugins/wall/src/wall.cpp' |
2 | --- plugins/wall/src/wall.cpp 2013-04-26 13:15:57 +0000 |
3 | +++ plugins/wall/src/wall.cpp 2013-04-27 06:17:25 +0000 |
4 | @@ -495,23 +495,19 @@ |
5 | |
6 | moveViewport (-dx, -dy, None); |
7 | } |
8 | - if (event->xclient.message_type == Atoms::xdndEnter) |
9 | - { |
10 | - toggleEdges (true); |
11 | - edgeDrag = true; |
12 | - } |
13 | - else if (event->xclient.message_type == Atoms::xdndLeave) |
14 | - edgeDrag = false; |
15 | |
16 | break; |
17 | |
18 | case FocusIn: |
19 | case FocusOut: |
20 | - if (event->xfocus.mode == NotifyGrab) |
21 | - poller.start (); |
22 | - else if (event->xfocus.mode == NotifyUngrab) |
23 | - poller.stop (); |
24 | - break; |
25 | + /* Edges on when grabbed */ |
26 | + if (!optionGetEdgeflipPointer ()) |
27 | + { |
28 | + if (event->xfocus.mode == NotifyGrab) |
29 | + toggleEdges (true); |
30 | + else if (event->xfocus.mode == NotifyUngrab) |
31 | + toggleEdges (false); |
32 | + } |
33 | |
34 | case ConfigureNotify: |
35 | break; |
36 | @@ -523,21 +519,34 @@ |
37 | screen->handleEvent (event); |
38 | } |
39 | |
40 | +/* |
41 | + * When a dnd type window is mapped toggle edge flip dnd windows back on |
42 | + * _NET_WM_WINDOW_TYPE_DND windows. This will not detect all dnd windows |
43 | + * but it will detect the EWMH-compliant ones without resorting to |
44 | + * hacks involving selection stealing. |
45 | + */ |
46 | void |
47 | -WallScreen::positionUpdate (const CompPoint &pos) |
48 | +WallWindow::windowNotify (CompWindowNotify n) |
49 | { |
50 | - if (edgeDrag) |
51 | - return; |
52 | + WallScreen *ws = WallScreen::get (screen); |
53 | + bool toggleOnDnd = ws->optionGetEdgeflipDnd (); |
54 | |
55 | - if (edgeRegion.contains (pos)) |
56 | - toggleEdges (false); |
57 | - else if (noEdgeRegion.contains (pos)) |
58 | + switch (n) |
59 | { |
60 | - if (!screen->grabbed ()) |
61 | - poller.stop (); |
62 | - toggleEdges (true); |
63 | + case CompWindowNotifyMap: |
64 | + if (window->type () & CompWindowTypeDndMask && toggleOnDnd) |
65 | + ws->toggleEdges (true); |
66 | + break; |
67 | + case CompWindowNotifyUnmap: |
68 | + if (window->type () & CompWindowTypeDndMask && toggleOnDnd) |
69 | + ws->toggleEdges (false); |
70 | + break; |
71 | + default: |
72 | + break; |
73 | } |
74 | -} |
75 | + |
76 | + window->windowNotify (n); |
77 | + } |
78 | |
79 | void |
80 | WallWindow::activate () |
81 | @@ -599,19 +608,19 @@ |
82 | void |
83 | WallWindow::grabNotify (int x, |
84 | int y, |
85 | - unsigned int width, |
86 | - unsigned int height) |
87 | + unsigned int state, |
88 | + unsigned int mask) |
89 | { |
90 | - WallScreen::get (screen)->toggleEdges (true); |
91 | - WallScreen::get (screen)->edgeDrag = true; |
92 | + if (mask & (CompWindowGrabMoveMask | CompWindowGrabButtonMask)) |
93 | + WallScreen::get (screen)->windowIsDragMoved = true; |
94 | |
95 | - window->grabNotify (x, y, width, height); |
96 | + window->grabNotify (x, y, state, mask); |
97 | } |
98 | |
99 | void |
100 | WallWindow::ungrabNotify () |
101 | { |
102 | - WallScreen::get (screen)->edgeDrag = false; |
103 | + WallScreen::get (screen)->windowIsDragMoved = false; |
104 | |
105 | window->ungrabNotify (); |
106 | } |
107 | @@ -760,30 +769,16 @@ |
108 | int dx, dy; |
109 | int amountX, amountY; |
110 | |
111 | - if (screen->otherGrabExist ("wall", "move", "group-drag", NULL)) |
112 | - return false; |
113 | + const bool allowFlipDnd = (state & CompAction::StateInitEdgeDnd) && |
114 | + optionGetEdgeflipDnd (); |
115 | + const bool allowFlipMove = (windowIsDragMoved && |
116 | + optionGetEdgeflipMove ()); |
117 | + const bool allowFlipPointer = optionGetEdgeflipPointer (); |
118 | |
119 | - if (state & CompAction::StateInitEdgeDnd) |
120 | - { |
121 | - if (!optionGetEdgeflipDnd ()) |
122 | - return false; |
123 | - } |
124 | - else if (screen->grabExist ("move")) |
125 | - { |
126 | - if (!optionGetEdgeflipMove ()) |
127 | - return false; |
128 | - } |
129 | - else if (screen->grabExist ("group-drag")) |
130 | - { |
131 | - if (!optionGetEdgeflipDnd ()) |
132 | - return false; |
133 | - } |
134 | - else if (!optionGetEdgeflipPointer ()) |
135 | - { |
136 | - toggleEdges (false); |
137 | - poller.start (); |
138 | + if (!allowFlipDnd && |
139 | + !allowFlipMove && |
140 | + !allowFlipPointer) |
141 | return false; |
142 | - } |
143 | |
144 | switch (direction) |
145 | { |
146 | @@ -1573,6 +1568,9 @@ |
147 | ww->isSliding = !optionGetNoSlideMatch ().evaluate (w); |
148 | } |
149 | break; |
150 | + case WallOptions::EdgeflipPointer: |
151 | + toggleEdges (optionGetEdgeflipPointer ()); |
152 | + break; |
153 | |
154 | default: |
155 | break; |
156 | @@ -1633,7 +1631,7 @@ |
157 | moveWindow (None), |
158 | focusDefault (true), |
159 | transform (NoTransformation), |
160 | - edgeDrag (false) |
161 | + windowIsDragMoved (false) |
162 | { |
163 | ScreenInterface::setHandler (screen); |
164 | CompositeScreenInterface::setHandler (cScreen); |
165 | @@ -1700,9 +1698,6 @@ |
166 | setNotify (ArrowShadowColor); |
167 | setNotify (NoSlideMatch); |
168 | setNotify (EdgeflipPointer); |
169 | - |
170 | - poller.setCallback (boost::bind (&WallScreen::positionUpdate, this, |
171 | - _1)); |
172 | } |
173 | |
174 | WallScreen::~WallScreen () |
175 | @@ -1735,8 +1730,6 @@ |
176 | return false; |
177 | if (!CompPlugin::checkPluginABI ("opengl", COMPIZ_OPENGL_ABI)) |
178 | return false; |
179 | - if (!CompPlugin::checkPluginABI ("mousepoll", COMPIZ_MOUSEPOLL_ABI)) |
180 | - return false; |
181 | |
182 | return true; |
183 | } |
184 | |
185 | === modified file 'plugins/wall/src/wall.h' |
186 | --- plugins/wall/src/wall.h 2013-04-26 13:15:57 +0000 |
187 | +++ plugins/wall/src/wall.h 2013-04-27 06:17:25 +0000 |
188 | @@ -167,10 +167,7 @@ |
189 | WallCairoContext highlightContext; |
190 | WallCairoContext arrowContext; |
191 | |
192 | - MousePoller poller; |
193 | - bool edgeDrag; |
194 | - CompRegion edgeRegion; |
195 | - CompRegion noEdgeRegion; |
196 | + bool windowIsDragMoved; |
197 | }; |
198 | |
199 | class WallWindow : |
200 | @@ -184,6 +181,7 @@ |
201 | virtual void activate (); |
202 | void grabNotify (int, int, unsigned int, unsigned int); |
203 | void ungrabNotify (); |
204 | + void windowNotify (CompWindowNotify); |
205 | bool glPaint (const GLWindowPaintAttrib &, const GLMatrix &, |
206 | const CompRegion &, unsigned int); |
207 | |
208 | |
209 | === modified file 'plugins/wall/wall.xml.in' |
210 | --- plugins/wall/wall.xml.in 2012-10-15 10:31:51 +0000 |
211 | +++ plugins/wall/wall.xml.in 2013-04-27 06:17:25 +0000 |
212 | @@ -17,7 +17,6 @@ |
213 | </relation> |
214 | <requirement> |
215 | <plugin>opengl</plugin> |
216 | - <plugin>mousepoll</plugin> |
217 | </requirement> |
218 | </deps> |
219 | <options> |
220 | @@ -290,6 +289,7 @@ |
221 | <edge name="Left"/> |
222 | </default> |
223 | <allowed edgednd="true"/> |
224 | + <passive_grab>false</passive_grab> |
225 | </option> |
226 | <option name="flip_right_edge" type="edge"> |
227 | <_short>Flip Right</_short> |
228 | @@ -299,6 +299,7 @@ |
229 | <edge name="Right"/> |
230 | </default> |
231 | <allowed edgednd="true"/> |
232 | + <passive_grab>false</passive_grab> |
233 | </option> |
234 | <option name="flip_up_edge" type="edge"> |
235 | <_short>Flip Up</_short> |
236 | @@ -308,6 +309,7 @@ |
237 | <edge name="Top"/> |
238 | </default> |
239 | <allowed edgednd="true"/> |
240 | + <passive_grab>false</passive_grab> |
241 | </option> |
242 | <option name="flip_down_edge" type="edge"> |
243 | <_short>Flip Down</_short> |
244 | @@ -317,6 +319,7 @@ |
245 | <edge name="Bottom"/> |
246 | </default> |
247 | <allowed edgednd="true"/> |
248 | + <passive_grab>false</passive_grab> |
249 | </option> |
250 | </subgroup> |
251 | </group> |
252 | |
253 | === modified file 'src/screen.cpp' |
254 | --- src/screen.cpp 2013-02-03 17:58:29 +0000 |
255 | +++ src/screen.cpp 2013-04-27 06:17:25 +0000 |
256 | @@ -3486,7 +3486,8 @@ |
257 | void |
258 | CompScreenImpl::removeAction (CompAction *action) |
259 | { |
260 | - if (!(privateScreen.initialized || action->active ())) |
261 | + if (!privateScreen.initialized || |
262 | + !action->active ()) |
263 | return; |
264 | |
265 | grabManager.setCurrentState(action->state()); |
266 | @@ -3902,6 +3903,11 @@ |
267 | SubstructureRedirectMask | SubstructureNotifyMask, &xev); |
268 | } |
269 | |
270 | +/* These functions do not guard against negative decrements |
271 | + * as they are unable to determine the source of the reference |
272 | + * and as such they should only be called by functions that |
273 | + * actually determine the source of what was referencing the |
274 | + * edge and can guard against multiple-references-per-owner */ |
275 | void |
276 | PrivateScreen::enableEdge (int edge) |
277 | { |
278 | |
279 | === added directory 'tests/manual/plugins' |
280 | === added file 'tests/manual/plugins/wall.txt' |
281 | --- tests/manual/plugins/wall.txt 1970-01-01 00:00:00 +0000 |
282 | +++ tests/manual/plugins/wall.txt 2013-04-27 06:17:25 +0000 |
283 | @@ -0,0 +1,49 @@ |
284 | +COMPIZ WALL PLUGIN MANUAL TESTS |
285 | +=============================== |
286 | +Sam Spilsbury <smspillaz@gmail.com> |
287 | + |
288 | +Edge Flip Pointer |
289 | +----------------- |
290 | +Setup: |
291 | +#. Enable "Edge Flip Pointer" in the wall plugin |
292 | + and set up screen edges as appropriate |
293 | + |
294 | +Actions: |
295 | +#. Move cursor to relevant screen edge |
296 | +#. Viewport will change to the next one |
297 | + over |
298 | +#. Viewport will not change if that |
299 | + viewport is the last one in the row |
300 | + or column |
301 | + |
302 | +Edge Flip Move |
303 | +-------------- |
304 | +Setup: |
305 | +#. Disable "Edge Flip Pointer" in the wall plugin, |
306 | + enable "Edge Flip Move" in the wall plugin, |
307 | + and set up screen edges as appropriate |
308 | + |
309 | +Actions: |
310 | +#. Grab window by titlebar |
311 | +#. Move cursor to relevant screen edge |
312 | +#. Viewport will change to the next one |
313 | + over while window is grabbed |
314 | +#. Viewport will not change if that |
315 | + viewport is the last one in the row |
316 | + or colum |
317 | + |
318 | +Edge Flip Dnd |
319 | +------------- |
320 | +Setup: |
321 | +#. Disable "Edge Flip Pointer" in the wall plugin, |
322 | + enable "Edge Flip Dnd" in the wall plugin, |
323 | + and set up screen edges as appropriate |
324 | + |
325 | +Actions: |
326 | +#. Grab a desktop icon |
327 | +#. Move cursor to relevant screen edge |
328 | +#. Viewport will change to the next one |
329 | + over while icon is grabbed |
330 | +#. Viewport will not change if that |
331 | + viewport is the last one in the row |
332 | + or colum |
You did it :)
From a first look:
Typos:
332 + or colum
316 + or colum
I do not understand this step:
292 + and set up screen edges as appropriate
307 + and set up screen edges as appropriate
323 + and set up screen edges as appropriate
I think this ^^ could be removed.
Please add newlines after:
66 + break;
70 + break;
The rest LGTM. I will test this ASAP.