Merge lp://qastaging/~compiz-team/compiz/compiz.fix_1008020 into lp://qastaging/compiz/0.9.8

Using stackingtest in a loop, I find windows appear over the top of the lock screen even with this fix. But only when using the animation plugin. Is that still what's being fixed here or a different issue?

I see exactly the same stacking behaviour with and without this fix...

Also, there's a trivial conflict in abiversion.h.

Could be a different issue. There is no conflict in abiversion.h according to lp

(Have a look at bug 1008020 for the case that I can absolutely confirm this fixes, I wasn't sure if the screen lock one and this one were related as I can't reproduce it anyways [I hate these race conditions])

> Using stackingtest in a loop, I find windows appear over the top of the lock
> screen even with this fix. But only when using the animation plugin. Is that
> still what's being fixed here or a different issue?
>
> I see exactly the same stacking behaviour with and without this fix...

A small question here: Do the windows appear temporarily on top of the lockscreen, or do they go away quickly? I know that we're supposed to create windows on top of fullscreen windows and it is the job of the fullscreen window to request that it goes on top of the other windows in that case.

As far as I can tell, only the open/close animations are visible over the lock screen.

In that case, there is no stacking bug (but a bug in the animation plugin for sure)

(What happens if you turn down the speed of the animations too)

I updated the description with the case I know this fixes for sure. We can unlinkbug 886605 if appropriate. I'd like to get feedback from cimi and racarr on that since they have macbook airs that can reproduce this.

There is a trivial conflict in abiversion.h

Also, as list::size() can be O(N)

117 + if (serverWindows.size ())

Should be:

117 + if (!serverWindows.empty ())

I think we should be making new functions as useful as possible. That means returning CompWindow* instead of Window (None --> NULL):
  Window cps::WindowManager::getTopServerWindow()

If you just return Window and actually want a CompWindow then you have to waste CPU and more lines of code looking it up again.

There is no CPU wastage since it hits the cached last found window. Im amy case the function cab be changed

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

null

I'm not talking about this proposal. I mean potential wastage for anyone else trying to use the function in future.

To be honest, we should move getTopWindow and getTopServerWindow to a more private place too.

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

I'm not talking about this proposal. I mean potential wastage for anyone else trying to use the function in future.
--
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1008020/+merge/109758
Your team Compiz Maintainers is subscribed to branch lp:compiz.

I don't see any problems running this branch. But I also still can't reproduce any of the bugs we're aiming to fix here. So can't really verify the fixes.

Just some notes on style:
You have inconsistent use of white space in function (calls). Sometimes there's a space, sometimes not. I think we should switch to no white space like most coding conventions do: function(calls).

Compiz coding style has always had a space between the caller and the brackets. i'd prefer to keep it that way

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

Review: Approve

I don't see any problems running this branch. But I also still can't reproduce any of the bugs we're aiming to fix here. So can't really verify the fixes.

Just some notes on style:
You have inconsistent use of white space in function (calls). Sometimes there's a space, sometimes not. I think we should switch to no white space like most coding conventions do: function(calls).

--
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1008020/+merge/110724
Your team Compiz Maintainers is subscribed to branch lp:compiz.

You have functions that may return null pointers, and call sites where you are dereferencing without checking.

Something should be done there.

I suggest something like:

inline Window window_from_compwindow(CompWindow* win)
{
  return win ? win->id () : None;
}

> I suggest something like:
>
> inline Window window_from_compwindow(CompWindow* win)
> {
> return win ? win->id () : None;
> }

Thanks, will do

Line 71 also needs the function treatment.

> PrivateWindow::createCompWindow (i ? children[i - 1] : 0, i ? children[i - 1] : 0, attrib, children[i]);

This is fugly.

Please do something the equivalent of:

CompWindow* top_window_id = i ? children[i - 1] : 0;
PrivateWindow::createCompWindow (top_window_id, top_window_id, attrib, children[i]);

If you had the CompWindowToWindow function defined in a header, you could use it in src/window.cpp instead of implementing the logic again.

> Line 71 also needs the function treatment.
>
> > PrivateWindow::createCompWindow (i ? children[i - 1] : 0, i ? children[i -
> 1] : 0, attrib, children[i]);
>
> This is fugly.
>
> Please do something the equivalent of:
>
> CompWindow* top_window_id = i ? children[i - 1] : 0;
> PrivateWindow::createCompWindow (top_window_id, top_window_id, attrib,
> children[i]);
>

It is not a CompWindow*, but I will still do

Window topWindowId = i ? children[i - 1] : 0;

> If you had the CompWindowToWindow function defined in a header, you could use
> it in src/window.cpp instead of implementing the logic again.

I do not like functions in headers, changing them means rebuilding everything that includes that header.

Remember to use "None" instead of "0". :)

0 == none but I can change it.

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

Remember to use "None" instead of "0". :)
--
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1008020/+merge/110724
Your team Compiz Maintainers is subscribed to branch lp:compiz.