Merge lp://qastaging/~mc-return/compiz/compiz.merge-fix1162484-original-cube-top-and-bottom-caps-not-deformed into lp://qastaging/compiz/0.9.10

Please re-target this for lp:compiz/0.9.10

I'm a bit confused about this:

5 void
6 CubeaddonScreen::CubeCap::load (bool scale, bool aspect, bool clamp)
7 {
8 - if (mFiles.empty ())
9 - return;
10 -

It seems like the point is to remove the early return if no images are available. However cubeaddon only loads images (to the best of my understanding). I'm pretty sure there's a risk of a segfault or std::out_of_range exeception just a few lines underneath it:

    mCurrent = mCurrent % mFiles.size ();

    CompString imgName = mFiles[mCurrent].s ();
    CompString pname = "cubeaddon";
    mTexture = GLTexture::readImageToTexture (imgName, pname, tSize);

Was this change really needed?

34 - cScreen->damageScreen ();
35 - }

Was there a reason for this one? The function documentation says that we should be posting a full-screen damage at that point.

/*
 * Switch cap, load it and damage screen if possible
 */
bool
CubeaddonScreen::changeCap (bool top, int change)

I'd imagine that's because if this function happened on a timer and there was no other damage or motion, we'd want the cap change to reflected as an immediate repaint.

227 -
228 CubeaddonScreen::CubeaddonScreen (CompScreen *s) :
229 PluginClassHandler<CubeaddonScreen, CompScreen> (s),
230 CubeaddonOptions (),
231 @@ -1608,4 +1605,3 @@
232
233 return true;
234 }
235 -

This is just a change in whitespace. Its not really a big problem, see if you can avoid them though because too many can clutter up diffs.

Hi, and thanks for the review :)
A lot of surprising Compiz action happening...

> I'm a bit confused about this:
>
> 5 void
> 6 CubeaddonScreen::CubeCap::load (bool scale, bool aspect, bool clamp)
> 7 {
> 8 - if (mFiles.empty ())
> 9 - return;
> 10 -
>
> It seems like the point is to remove the early return if no images are
> available. However cubeaddon only loads images (to the best of my
> understanding). I'm pretty sure there's a risk of a segfault or
> std::out_of_range exeception just a few lines underneath it:
>
> mCurrent = mCurrent % mFiles.size ();
>
> CompString imgName = mFiles[mCurrent].s ();
> CompString pname = "cubeaddon";
> mTexture = GLTexture::readImageToTexture (imgName, pname, tSize);
>
> Was this change really needed?
>
Well, currently this cannot happen, because CubeaddonScreen::CubeCap::load
is just called by CubeaddonScreen::changeCap, where cap->mFiles.size ()
is already checked - no need to check it again, because this check already
happened a moment ago.

> 34 - cScreen->damageScreen ();
> 35 - }
>
> Was there a reason for this one? The function documentation says that we
> should be posting a full-screen damage at that point.
>
I was not sure about that one, but my logic tells me that we are already
rendering fullscreen (with fullscreen damage), when we show the cube -
but I can bring that back in.

> /*
> * Switch cap, load it and damage screen if possible
> */
> bool
> CubeaddonScreen::changeCap (bool top, int change)
>
> I'd imagine that's because if this function happened on a timer and there was
> no other damage or motion, we'd want the cap change to reflected as an
> immediate repaint.
>
Probably you are right, I'll bring it back in.
Unfortunately I could not test to see if any flickering occurs, because switching
top and bottom cap textures via keyboard shortcut is currently broken as well:
see bug #1162740

> 227 -
> 228 CubeaddonScreen::CubeaddonScreen (CompScreen *s) :
> 229 PluginClassHandler<CubeaddonScreen, CompScreen> (s),
> 230 CubeaddonOptions (),
> 231 @@ -1608,4 +1605,3 @@
> 232
> 233 return true;
> 234 }
> 235 -
>
> This is just a change in whitespace. Its not really a big problem, see if you
> can avoid them though because too many can clutter up diffs.

Unfortunately I found a lot of whitespaces, newlines and indentation problems
throughout the code...
IMHO we should fix those in the long run and beautify our code.
Existing code should also be an example how we want future code to be.
If indentation and style are a mess, code also is harder to read and bugs can
easier slip through undetected...

On Tue, Apr 2, 2013 at 4:13 PM, MC Return <email address hidden> wrote:
> Hi, and thanks for the review :)
> A lot of surprising Compiz action happening...
>
>> I'm a bit confused about this:
>>
>> 5 void
>> 6 CubeaddonScreen::CubeCap::load (bool scale, bool aspect, bool clamp)
>> 7 {
>> 8 - if (mFiles.empty ())
>> 9 - return;
>> 10 -
>>
>> It seems like the point is to remove the early return if no images are
>> available. However cubeaddon only loads images (to the best of my
>> understanding). I'm pretty sure there's a risk of a segfault or
>> std::out_of_range exeception just a few lines underneath it:
>>
>> mCurrent = mCurrent % mFiles.size ();
>>
>> CompString imgName = mFiles[mCurrent].s ();
>> CompString pname = "cubeaddon";
>> mTexture = GLTexture::readImageToTexture (imgName, pname, tSize);
>>
>> Was this change really needed?
>>
> Well, currently this cannot happen, because CubeaddonScreen::CubeCap::load
> is just called by CubeaddonScreen::changeCap, where cap->mFiles.size ()
> is already checked - no need to check it again, because this check already
> happened a moment ago.

Okay. What I would suggest doing is dropping the former check instead
of the one inside CubeCap::load. That means that we don't need to have
one of CubeCap::load's preconditions be that cap->mFiles.size () is >
0. Instead, the check only happens once, and it happens inside of
CubeCap::load (which in fact, is a better design, because now the
caller doesn't need to care about how many files the cap has).

>
>> 34 - cScreen->damageScreen ();
>> 35 - }
>>
>> Was there a reason for this one? The function documentation says that we
>> should be posting a full-screen damage at that point.
>>
> I was not sure about that one, but my logic tells me that we are already
> rendering fullscreen (with fullscreen damage), when we show the cube -
> but I can bring that back in.
>
>> /*
>> * Switch cap, load it and damage screen if possible
>> */
>> bool
>> CubeaddonScreen::changeCap (bool top, int change)
>>
>> I'd imagine that's because if this function happened on a timer and there was
>> no other damage or motion, we'd want the cap change to reflected as an
>> immediate repaint.
>>
> Probably you are right, I'll bring it back in.
> Unfortunately I could not test to see if any flickering occurs, because switching
> top and bottom cap textures via keyboard shortcut is currently broken as well:
> see bug #1162740
>

Yes, we do render full-screen when the cube is active. However, we
don't post a fullscreen damage on every frame, we only post one if
something changes that requires us to update the scene. This would be
one of those things.

>> 227 -
>> 228 CubeaddonScreen::CubeaddonScreen (CompScreen *s) :
>> 229 PluginClassHandler<CubeaddonScreen, CompScreen> (s),
>> 230 CubeaddonOptions (),
>> 231 @@ -1608,4 +1605,3 @@
>> 232
>> 233 return true;
>> 234 }
>> 235 -
>>
>> This is just a change in whitespace. Its not really a big problem, see if you
>> can avoid them though because too many can clutter up diffs.
>
> Unfortunately I found a lot of whitespaces, newlines and indentation probl...

> Okay. What I would suggest doing is dropping the former check instead
> of the one inside CubeCap::load. That means that we don't need to have
> one of CubeCap::load's preconditions be that cap->mFiles.size () is >
> 0. Instead, the check only happens once, and it happens inside of
> CubeCap::load (which in fact, is a better design, because now the
> caller doesn't need to care about how many files the cap has).
>
You're right. I'll look into that.

>
> Yes, we do render full-screen when the cube is active. However, we
> don't post a fullscreen damage on every frame, we only post one if
> something changes that requires us to update the scene. This would be
> one of those things.
>
I've corrected that now - sorry for the mistake.

>
> Okay. What I would suggest doing is dropping the former check instead
> of the one inside CubeCap::load. That means that we don't need to have
> one of CubeCap::load's preconditions be that cap->mFiles.size () is >
> 0. Instead, the check only happens once, and it happens inside of
> CubeCap::load (which in fact, is a better design, because now the
> caller doesn't need to care about how many files the cap has).
>
Done in r3647.

Also:
Removed redundant mCurrent = mCurrent % mFiles.size (); calculation,
this has already been done in CubeaddonScreen::changeCap.

cap->mCurrent = (cap->mCurrent + change + count) % count;
could imho also be reduced to:
cap->mCurrent = (cap->mCurrent + change) % count;
because modulo will never return negative values AFAIK, but I kept it
for now...
Also we just need to do this calculation if count and change both are != 0

Bug #1162740 is still open, I suspect the problem is hidden here:

#define BIND_ACTION(opt, t, d) \
    optionSet##opt##Initiate (boost::bind (&CubeaddonScreen::changeCap, this, t, d))

    BIND_ACTION (TopNextKey, true, 1);
    BIND_ACTION (TopPrevKey, true, -1);
    BIND_ACTION (BottomNextKey, false, 1);
    BIND_ACTION (BottomNextKey, false, -1);

    BIND_ACTION (TopNextButton, true, 1);
    BIND_ACTION (TopPrevButton, true, -1);
    BIND_ACTION (BottomNextButton, false, 1);
    BIND_ACTION (BottomNextButton, false, -1);

#undef BIND_ACTION

I could not get the plugin to send those values to changeCap :(
Do you have an idea what could be wrong with that, Sam ?

> >
> > Okay. What I would suggest doing is dropping the former check instead
> > of the one inside CubeCap::load. That means that we don't need to have
> > one of CubeCap::load's preconditions be that cap->mFiles.size () is >
> > 0. Instead, the check only happens once, and it happens inside of
> > CubeCap::load (which in fact, is a better design, because now the
> > caller doesn't need to care about how many files the cap has).
> >
> Done in r3647.
>

Thanks, +1
>
> Bug #1162740 is still open, I suspect the problem is hidden here:
>
> #define BIND_ACTION(opt, t, d) \
> optionSet##opt##Initiate (boost::bind (&CubeaddonScreen::changeCap, this,
> t, d))
>
> BIND_ACTION (TopNextKey, true, 1);
> BIND_ACTION (TopPrevKey, true, -1);
> BIND_ACTION (BottomNextKey, false, 1);
> BIND_ACTION (BottomNextKey, false, -1);
>
> BIND_ACTION (TopNextButton, true, 1);
> BIND_ACTION (TopPrevButton, true, -1);
> BIND_ACTION (BottomNextButton, false, 1);
> BIND_ACTION (BottomNextButton, false, -1);
>
> #undef BIND_ACTION
>
> I could not get the plugin to send those values to changeCap :(
> Do you have an idea what could be wrong with that, Sam ?

Is it that changeCap is never called or is it that the values for "top" and "change" in CubeaddonScreen::changeCap are never different for each keybinding.

If its the former, try binding the action to another keybinding, eg Control-Super-a. It might be that we don't allow actions to be bound on non-modified keybindings.

>
> Is it that changeCap is never called or is it that the values for "top" and
> "change" in CubeaddonScreen::changeCap are never different for each
> keybinding.
>
> If its the former, try binding the action to another keybinding, eg Control-
> Super-a. It might be that we don't allow actions to be bound on non-modified
> keybindings.

It is the former, but I tried all kinds of mouse and keyboard combos.
I can confirm that keys get recognized during the cube -> for example I can
start the rotating cube screensaver and then do all kinds of things like
moving windows from viewport to viewport, minimizing/maximizing windows,
revealing/hiding Guake or type into a console...

Just those cubeaddon ones are striking :(
I really suspect the code I posted above to be the culprit...

Unfortunately I found one other bug in cube-addon:
The deformation stops during the time windows are animated :(

On Tue, Apr 2, 2013 at 11:52 PM, MC Return <email address hidden> wrote:
>>
> It is the former, but I tried all kinds of mouse and keyboard combos.
> I can confirm that keys get recognized during the cube -> for example I can
> start the rotating cube screensaver and then do all kinds of things like
> moving windows from viewport to viewport, minimizing/maximizing windows,
> revealing/hiding Guake or type into a console...

Is it just during the screensaver where the keybindings don't work?

>
> Just those cubeaddon ones are striking :(
> I really suspect the code I posted above to be the culprit...
> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix1162484-original-cube-top-and-bottom-caps-not-deformed/+merge/156469
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

>
> Is it just during the screensaver where the keybindings don't work?
>
No. Do they work for you ?

FAILED: Continuous integration, rev:3647
http://jenkins.qa.ubuntu.com/job/compiz-ci/115/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/152/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/504/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/115/rebuild

The Jenkins failures seem to be unrelated to this MP...

FAILED: Continuous integration, rev:3648
http://jenkins.qa.ubuntu.com/job/compiz-ci/119/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/156/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/508/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/119/rebuild

> >
> > Is it just during the screensaver where the keybindings don't work?
> >
> No. Do they work for you ?

I haven't tried. But I suspect that the screensaver plugin doesn't take a grab when it activates, which would mean that single-key-only bindings are unlikely to work the way you might expect them to.

I don't think there's a "bug" here if the keybindings work whilst manually rotating the cube

>
> I haven't tried. But I suspect that the screensaver plugin doesn't take a grab
> when it activates, which would mean that single-key-only bindings are unlikely
> to work the way you might expect them to.
>
> I don't think there's a "bug" here if the keybindings work whilst manually
> rotating the cube

Those keyboard/mouse shortcuts *never* work. I shouldn't have mentioned screensaver...