Merge lp:~mc-return/compiz/compiz.merge-ezoom-cleanup into lp:compiz/0.9.10

[MC Return]

*EZoom (Enhanced Zoom Desktop) code cleanup:

Optimized ABI check.
Use prefix- instead of postfix-increment.
Assignment and declaration of local variables in one line.
Merged if statements.
Removed redundant brackets.
Fixed indentation.
Added and removed newlines to improve readability.

*EZoom (Enhanced Zoom Desktop) code changes:

Replaced #define WIDTH (x2 - x1) with int width = x2 - x1;
and #define HEIGHT (y2 - y1) with int height = y2 - y1;

Replaced WIDTHOK and HEIGHTOK macros with local variables:
WIDTHOK and HEIGHTOK just need to be calculated once and can then
be used for all cases in void EZoomScreen::ensureVisibilityArea (...).

static_cast <float> for int denominators in divisions.

Removed these redundant gl calls:
glEnable (GL_TEXTURE_2D);
glDisable (GL_TEXTURE_2D);

Zoom Box fixes:
Enabled GL_BLEND for the zoom box to fix transparency issues.
Made outline and fill color and transparency of the zoom box configurable.
Outline width is 2.0 for the zoom box as well now.
(same like for grid preview and screenshot selector outlines)
Fixed zoom box damage handling.
Minor restructuring in zoom box rendering code.

Minor ezoom.xml.in cleanup and enhancements.
Load after mousepoll.

(fixes LP: #1171665, partially fixes LP: #1051290 and LP: #1051295)

CCSM look of Screenshot plugin:
https://bugs.launchpad.net/compiz-core/+bug/771875/+attachment/3652256/+files/Screenshot%20from%202013-04-24%2014%3A17%3A23.png

[Sam Spilsbury (smspillaz)]

78 + zooms.at (out).xVelocity ||
79 + zooms.at (out).yVelocity ||
80 + zooms.at (out).zVelocity ||

It might be better to express these as != 0.0f

Though they are floats - so they really should be compared in terms of rounding to avoid floating point comparison errors. But I wouldn't worry about that for now as we don't really have a uniform method to compare floats. See http://stackoverflow.com/questions/17333/most-effective-way-for-float-and-double-comparison . We'll need to find a way to do this properly and uniformly.

110 - {
111 zooms.at (out).currentZoom += (zooms.at (out).zVelocity * chunk) /
112 cScreen->redrawTime ();
113 - }

While not strictly necessary, these brackets aid readability for me because the code spans two lines. There's no hard and fast rule here though.

217 - for (out = 0; out < zooms.size (); out++)
218 - {
219 + for (unsigned int out = 0; out < zooms.size (); out++)
220 if (isInMovement (out) && isActive (out))
221 {
222 cScreen->damageScreen ();
223 break;
224 }
225 - }

As I've mentioned a few times before, please don't remove brackets in control-flow statements which have nested control flow statements with brackets. The reason is that its confusing to read at a glance.

251 + GLMatrix zTransform (transform);
252 + int inx1, inx2, iny1, iny2;
253 + int out = output->id ();
254 + GLushort colorData[4];

These should probably line up - remove the tab usage in between the "int" and the name declaration and just use spaces to line it up.

295 + const GLMatrix &transform,
296 + const CompRegion &region,
297 + CompOutput *output,
298 + unsigned int mask)

Ditto.

#define WIDTHOK (float)(x2-x1) / (float)o->width () < zooms.at (out).newZoom
#define HEIGHTOK (float)(y2-y1) / (float)o->height () < zooms.at (out).newZoom

...

541 if (WIDTHOK &&
542 - HEIGHTOK) {
543 + HEIGHTOK)
544 + {

Its an extraneous point, but remove these macros and replace them with inline functions if you can.

656 screen->warpPointer ((int) (mouse.x () - pointerX) -
657 - (int) ((float)diffX * z),
658 - (int) (mouse.y () - pointerY) -
659 + (int) ((float)diffX * z),
660 + (int) (mouse.y () - pointerY) -
661 (int) ((float)diffY * z));

If we're editing whitespace here, the rule in compiz is that the following line in a wrapped statement should be four spaces from where the first started, eg:

int foo = bar -
              baz -
                  var;

769 +// glDisable (GL_BLEND);
...
826 +// glDisable (GL_TEXTURE_2D);

Take note of what I said in the other review about BLEND enable / disable. It should be turned on and off whenever blending is actually needed and not left on at the end of the function. As for removing code - just remove it, don't comment it out.

897 #define WIDTH (x2 - x1)
898 #define HEIGHT (y2 - y1)

Replace those with variables.

1061 + setScaleBigger (out, (float) width / o->width (), (float)
1062 + height / o->height ());

The denominator here should be the one that is cast - not the numerator. Eg

1061 + setScaleBigger ...

[MC Return (mc-return)]

> 78 + zooms.at (out).xVelocity ||
> 79 + zooms.at (out).yVelocity ||
> 80 + zooms.at (out).zVelocity ||
>
> It might be better to express these as != 0.0f
>
> Though they are floats - so they really should be compared in terms of
> rounding to avoid floating point comparison errors. But I wouldn't worry about
> that for now as we don't really have a uniform method to compare floats. See
> http://stackoverflow.com/questions/17333/most-effective-way-for-float-and-
> double-comparison . We'll need to find a way to do this properly and
> uniformly.
>
Maybe we should do this in a follow-up MP ?

> 110 - {
> 111 zooms.at (out).currentZoom += (zooms.at (out).zVelocity *
> chunk) /
> 112 cScreen->redrawTime ();
> 113 - }
>
> While not strictly necessary, these brackets aid readability for me because
> the code spans two lines. There's no hard and fast rule here though.
>
The indentation was not ideal in this case - I've fixed this now.

> 217 - for (out = 0; out < zooms.size (); out++)
> 218 - {
> 219 + for (unsigned int out = 0; out < zooms.size (); out++)
> 220 if (isInMovement (out) && isActive (out))
> 221 {
> 222 cScreen->damageScreen ();
> 223 break;
> 224 }
> 225 - }
>
> As I've mentioned a few times before, please don't remove brackets in control-
> flow statements which have nested control flow statements with brackets. The
> reason is that its confusing to read at a glance.
>
I am sorry, it was from a cleanup that I did, before knowing the style we agreed
on, I just missed it. I have readded the brackets now.

> 251 + GLMatrix zTransform (transform);
> 252 + int inx1, inx2, iny1, iny2;
> 253 + int out = output->id ();
> 254 + GLushort colorData[4];
>
>
> These should probably line up - remove the tab usage in between the "int" and
> the name declaration and just use spaces to line it up.
>
Done. I will use spaces in these cases from now on.

> 295 + const GLMatrix &transform,
> 296 + const CompRegion &region,
> 297 + CompOutput *output,
> 298 + unsigned int mask)
>
> Ditto.
>
Done as well.

> #define WIDTHOK (float)(x2-x1) / (float)o->width () < zooms.at (out).newZoom
> #define HEIGHTOK (float)(y2-y1) / (float)o->height () < zooms.at (out).newZoom
>
> ...
>
> 541 if (WIDTHOK &&
> 542 - HEIGHTOK) {
> 543 + HEIGHTOK)
> 544 + {
>
> Its an extraneous point, but remove these macros and replace them with inline
> functions if you can.
>
I'll see what I can do.

> 656 screen->warpPointer ((int) (mouse.x () - pointerX) -
> 657 - (int)
> ((float)diffX * z),
> 658 - ...

[MC Return (mc-return)]

>
> #define WIDTHOK (float)(x2-x1) / (float)o->width () < zooms.at (out).newZoom
> #define HEIGHTOK (float)(y2-y1) / (float)o->height () < zooms.at (out).newZoom
>
> ...
>
> 541 if (WIDTHOK &&
> 542 - HEIGHTOK) {
> 543 + HEIGHTOK)
> 544 + {
>
> Its an extraneous point, but remove these macros and replace them with inline
> functions if you can.
>
Looking at it in detail, no inline function is even needed. Replaced those macros
with local bools.

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

>
> > 1061 + setScaleBigger (out, (float) width / o->width (), (float)
> > 1062 + height / o->height ());
> >
> > The denominator here should be the one that is cast - not the numerator. Eg
> >
> > 1061 + setScaleBigger (out, width / static_cast <float> (o->width
> > ()),
> > 1062 + height / static_cast <float> (o->height
> ()));
> >
> > 1345 + float scale = MAX ((float) width / screen->outputDevs
> > ().at(out).width (),
> > 1346 + (float) height / screen->outputDevs ().at
> > (out).height ());
> >
> > Ditto.
> >
> I think this was done, because width and height are type int...
> > Other than that looks good.

CompRect::width () and CompRect::height () are also integers.

It should be the denominator that's being divided and not the numerator.

[MC Return (mc-return)]

>
> It should be the denominator that's being divided and not the numerator.

Hopefully fixed now as well in r3666.

During testing I found bug #1171665 and fixed it as well :)

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

Hey, thanks for fixing that

As I mentioned before, try to keep separate fixes in separate proposals where possible. There are some comments I have on the fix for LP #1171665 :

 432 /* draw outline */
 433 alpha = ((float) optionGetOutlineColorAlpha () / 65535.0f);
 434 color = optionGetOutlineColor ();
 435
 436 colorData[0] = alpha * color[0];
 437 colorData[1] = alpha * color[1];
 438 colorData[2] = alpha * color[2];
 439 colorData[3] = alpha * 65535.0f;
434440

1. The numerator is being cast in floating point division at 433. This is not necessary.
2. Replace "65535.0f" with static_cast <float> (std::numeric_limits <unsigned short>::max ()). That is a bit verbose, so you can assign it to a const if necessary, eg:

const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();

[MC Return (mc-return)]

> Hey, thanks for fixing that
>
No problem.

> As I mentioned before, try to keep separate fixes in separate proposals where
> possible. There are some comments I have on the fix for LP #1171665 :
>
Urgh - sorry -> sometimes I can't help myself -> I stumbled upon this bug during
testing and I knew how to fix it -> so I did. Sorry again.

> 432 /* draw outline */
> 433 alpha = ((float) optionGetOutlineColorAlpha () / 65535.0f);
> 434 color = optionGetOutlineColor ();
> 435
> 436 colorData[0] = alpha * color[0];
> 437 colorData[1] = alpha * color[1];
> 438 colorData[2] = alpha * color[2];
> 439 colorData[3] = alpha * 65535.0f;
> 434440
>
> 1. The numerator is being cast in floating point division at 433. This is not
> necessary.
> 2. Replace "65535.0f" with static_cast <float> (std::numeric_limits <unsigned
> short>::max ()). That is a bit verbose, so you can assign it to a const if
> necessary, eg:
>
> const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();

If I am not totally mistaken we already have constants for this, I think they are
named COLOR ? I have to check...
If we do not have global constants, we should create those, because this are values
constantly used throughout the code...

[MC Return (mc-return)]

> 1. The numerator is being cast in floating point division at 433. This is not
> necessary.

Very good catch. Fixed in r3667.

> 2. Replace "65535.0f" with static_cast <float> (std::numeric_limits <unsigned
> short>::max ()). That is a bit verbose, so you can assign it to a const if
> necessary, eg:
>
> const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();

Don't we have global constants for things like that ?
I also noticed PI getting defined by several plugins - we should have global
constants for values like these, which get used several times throughout our
code...

In composite.h I found:

#define OPAQUE 0xffff
#define COLOR 0xffff
#define BRIGHT 0xffff

for example...

Maybe use COLOR in our case ?

review: Needs Information

[Sam Spilsbury (smspillaz)]

>
> > 1. The numerator is being cast in floating point division at 433. This is
> not
> > necessary.
>
> Very good catch. Fixed in r3667.
>
> > 2. Replace "65535.0f" with static_cast <float> (std::numeric_limits
> <unsigned
> > short>::max ()). That is a bit verbose, so you can assign it to a const if
> > necessary, eg:
> >
> > const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();
>
> Don't we have global constants for things like that ?
> I also noticed PI getting defined by several plugins - we should have global
> constants for values like these, which get used several times throughout our
> code...
>
> In composite.h I found:
>
> #define OPAQUE 0xffff
> #define COLOR 0xffff
> #define BRIGHT 0xffff
>
> for example...
>
> Maybe use COLOR in our case ?

So 0xffff is basically MAX_USHRT. It would be better to switch those over to be actual constants one day, but not here. The better place to use those #defines is where you're modifying a CompWindowPaintAttrib as opposed to expressing an alpha value as a fraction (eg "what does 34362 divided by COLOR mean?")

[MC Return (mc-return)]

Okay, all done :)

Also replaced postfix increments with prefix ones, because those are more efficient and faster...

review: Needs Resubmitting

[MC Return (mc-return)]

I just want to note here, that EZoom is still far from being perfect, the overdrawing of the zoom box is also an issue
here (the same problem like screenshot), also EZoom still misses damage rectangle calculation and handling for the
zoom box - currently there is no damage during zoom box drawing applied at all...

So a follow-up MP or (looking at the options list of EZoom) multiple ones will be necessary to make EZoom shine perfectly...

[MC Return (mc-return)]

Hmm, I have a fix for the missing zoom box damage handling, this would be the diff:

=== modified file 'plugins/ezoom/src/ezoom.cpp'
@@ -464,6 +466,12 @@
     streamingBuffer->render (zTransform);

     glDisable (GL_BLEND);
+
+ /* Damage the zoom selection box region during draw */
+ cScreen->damageRegion (CompRegion (x1 - 1,
+ y1 - 1,
+ x2 - x1 + 1,
+ y2 - y1 + 1));
 }

Do we want this separated ?

review: Needs Information

[Sam Spilsbury (smspillaz)]

The correct fix for the zoom-box handling would be to call damageRegion for the last and current zoom box rectangle. So at least you're on the right track there, but yes, please do it separately. For now I'm fine with this as-is.

review: Approve

[MC Return (mc-return)]

> The correct fix for the zoom-box handling would be to call damageRegion for
> the last and current zoom box rectangle. So at least you're on the right track
> there, but yes, please do it separately. For now I'm fine with this as-is.

Cool, thanks. I am working on the follow-up...

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-autolanding/10/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-amd64-autolanding/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-armhf-autolanding/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-i386-autolanding/10/console

review: Needs Fixing (continuous-integration)

[MC Return (mc-return)]

*Urgh*
The "Applying patch ubuntu-config.patch" madness hit me here...

Let's quilt-fix this... Sooner or later quilt patching has to be done anyway...

[MC Return (mc-return)]

Grmpf, I already got the follow-up ready :(

[MC Return (mc-return)]

Hmm, investigation shows that the ubuntu quilt patch just removes all default EZoom shortcuts, namely
the predefined shortcuts to zoom in/out, invoke the zoom box and to zoom to a specific window.

I guess this was done, because all "Super+holdanykey" combinations conflict with Unity's default behaviour
(display help overlay/show-launcher-shortcut-numbers), which is a design problem in itself (see bug #1069158
if you are interested in details).

The removal of these standard-shortcuts was completely unnecessary for Ubuntu anyway, as EZoom is disabled
per default in Ubuntu anyway AFAIK.

I vote for removing this ugly patch from EZoom completely as it is not needed anyway and massively complicates
development.

review: Needs Information

[Sam Spilsbury (smspillaz)]

> Hmm, investigation shows that the ubuntu quilt patch just removes all default
> EZoom shortcuts, namely
> the predefined shortcuts to zoom in/out, invoke the zoom box and to zoom to a
> specific window.
>
> I guess this was done, because all "Super+holdanykey" combinations conflict
> with Unity's default behaviour
> (display help overlay/show-launcher-shortcut-numbers), which is a design
> problem in itself (see bug #1069158
> if you are interested in details).
>
> The removal of these standard-shortcuts was completely unnecessary for Ubuntu
> anyway, as EZoom is disabled
> per default in Ubuntu anyway AFAIK.
>
> I vote for removing this ugly patch from EZoom completely as it is not needed
> anyway and massively complicates
> development.

Ask didrocks about it on IRC then.

review: Needs Information

[Sam Spilsbury (smspillaz)]

I saw there was some discussion yesterday - was there an outcome from this?

review: Needs Information

[MC Return (mc-return)]

> I saw there was some discussion yesterday - was there an outcome from this?

Reconstruction shows:

Apr 25 10:08:23 <MCR> didrocks, I would like to remove the quilt patch from EZoom xml.
Apr 25 10:08:51 <didrocks> MCR: is it upstream?
Apr 25 10:09:13 <MCR> The ubuntu quilt patch just removes 4 shortcuts from EZoom.
Apr 25 10:09:18 <didrocks> MCR: right
Apr 25 10:09:24 <MCR> No, it will affect Ubuntu
Apr 25 10:09:27 <MCR> but not really
Apr 25 10:09:36 <didrocks> MCR: so no, this patch is there for a usability reason
Apr 25 10:09:37 <MCR> as EZoom is turned off by default
Apr 25 10:09:49 <didrocks> MCR: usability testing showed that people hit that by mistake
Apr 25 10:09:55 <didrocks> then, they feel "stuck"
Apr 25 10:10:00 <didrocks> and reboot their machine
Apr 25 10:10:05 <didrocks> not a nice experience :/
Apr 25 10:10:10 <didrocks> hence the patch
Apr 25 10:10:13 <MCR> how can someone hit this by mistake ?
Apr 25 10:10:23 <didrocks> MCR: it seems it happens a lot
Apr 25 10:10:29 <MCR> you have to enable EZoom manually first
Apr 25 10:10:30 <didrocks> especially super + scrollwheel
Apr 25 10:10:36 <didrocks> MCR: it's enabled by default
Apr 25 10:10:39 <MCR> ah
Apr 25 10:10:45 <MCR> my fault then
Apr 25 10:10:53 <didrocks> no worry :)
Apr 25 10:11:08 <MCR> How about changing the upstream shortcuts to something else then ?
Apr 25 10:11:42 <didrocks> MCR: I think the shortcut makes default
Apr 25 10:12:06 <MCR> didrocks, it is completely useless to have an enabled EZoom without any shortcuts...
Apr 25 10:12:19 <MCR> why is it enabled in the first place ?
Apr 25 10:12:21 <didrocks> MCR: the idea is to avoid people enabling/disabling plugin (which is risky)
Apr 25 10:12:30 <didrocks> then, having an option to enable the shortcuts
Apr 25 10:12:35 <didrocks> MCR: want to tackle that? :)
Apr 25 10:12:40 <MCR> didrocks, soon it won't be risky anymore
Apr 25 10:16:34 <MCR> didrocks, I want to reduce Ubuntu-distro-patches against Compiz to a minimum... it makes changes to .xml.in files and code to a small horror-trip ;)

We now know:

1. EZoom is enabled by default, because enabling it manually in CCSM once had a high risk of crashing Compiz.
2. Although EZoom is enabled by default all 4 standard shortcuts are removed from Ubuntu, because they conflict
   with "Super" (see bug #1069158 if you are interested in details).

So IMHO this means that one solution would be to remove the EZoom Ubuntu patch that is removing the shortcuts and
to disable EZoom by default, while making sure to reduce the chance of a crashing Compiz, if users enable or disable
plugins there.
IMHO we are 1000x more stable in CCSM now, then we were when this patching was designed...

What is your opinion on this matter ?

review: Needs Information

[Sam Spilsbury (smspillaz)]

On Fri, Apr 26, 2013 at 2:55 PM, MC Return <email address hidden> wrote:
> Review: Needs Information
>
>> I saw there was some discussion yesterday - was there an outcome from this?
...
>
> We now know:
>
> 1. EZoom is enabled by default, because enabling it manually in CCSM once had a high risk of crashing Compiz.
> 2. Although EZoom is enabled by default all 4 standard shortcuts are removed from Ubuntu, because they conflict
> with "Super" (see bug #1069158 if you are interested in details).
>
> So IMHO this means that one solution would be to remove the EZoom Ubuntu patch that is removing the shortcuts and
> to disable EZoom by default, while making sure to reduce the chance of a crashing Compiz, if users enable or disable
> plugins there.
> IMHO we are 1000x more stable in CCSM now, then we were when this patching was designed...
>
> What is your opinion on this matter ?

I think we should add an autopilot test to enable and disable the unity plugin.

That could get tricky though - I'll have to check Lukasz about whether
or not that's possible.

In any case, are you able to refresh the distro patches and unblock
this merge review?

> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-ezoom-cleanup/+merge/159991
> You are reviewing the proposed merge of lp:~mc-return/compiz/compiz.merge-ezoom-cleanup into lp:compiz.

--
Sam Spilsbury

[MC Return (mc-return)]

>
> In any case, are you able to refresh the distro patches and unblock
> this merge review?
>
Probably yes, but I hate horror-trips.

You really could do me a *big* favor if you could do it. Sooner or later I'll have to quilt again, but I really would prefer later... ;)

The follow-up of this MP with the corrected damage handling is also ready here locally, so it would be appreciated to get this in.

(the float comparisons also still need fixing)

[Sam Spilsbury (smspillaz)]

>
> >
> > In any case, are you able to refresh the distro patches and unblock
> > this merge review?
> >
> Probably yes, but I hate horror-trips.
>
> You really could do me a *big* favor if you could do it. Sooner or later I'll
> have to quilt again, but I really would prefer later... ;)

My time is very limited these days with study commitments. I can't be writing code for other people. Can you please try to do it yourself?

Alternatively, separate the xml changes out into another merge proposal so that we can unblock this one.

>
> The follow-up of this MP with the corrected damage handling is also ready here
> locally, so it would be appreciated to get this in.
>
> (the float comparisons also still need fixing)

[MC Return (mc-return)]

>
> My time is very limited these days with study commitments. I can't be writing
> code for other people. Can you please try to do it yourself?
>
> Alternatively, separate the xml changes out into another merge proposal so
> that we can unblock this one.
>
Not that easy to separate those as changes to both files are needed.
I will get to it one day. Set to WIP for now...

review: Needs Fixing

[MC Return (mc-return)]

This plugin is a good example for unnecessary patching. The quilt patch here just removes the standard shortcuts completely, while still having the plugin enabled by default in Ubuntu. That makes *no* *sense* *at* *all*.

The correct solution would be:
1. Remove the quilt patch from ezoom completely
2. Disable EZoom for Ubuntu per default

Why is this the better solution you might ask:

1. The plugin does not get loaded at all, which improves memory usage, boot speed and Compiz start speed, while not changing any functionality for the Ubuntu user.
2. The stupid quilt patch is removed from lp:compiz, making hacking and testing the code much easier.
3. It is easier for the user to re-enable EZoom, because they do not have to manually add all the shortcuts to make the plugin usable again, instead they just have to enable the plugin.

review: Needs Information

[MC Return (mc-return)]

r3670 fully fixes damage handling of the zoom box and also improves the ABI logic for this plugin.

[MC Return (mc-return)]

Info (for easier review): This was all that was needed to be added to fix damage handling here:

 470 /* Damage the zoom selection box region during draw */
 471 cScreen->damageRegion (CompRegion (x1 - 1,
 472 y1 - 1,
 473 x2 - x1 + 1,
 474 y2 - y1 + 1));

The other changes in r3670 are added comments and a improved ABI check as discussed in the other MP already.

[MC Return (mc-return)]

Now IMHO all that is needed to be done for this to land is to remove EZoom from the list of plugins, which are enabled by default and to remove the quilt patching of ezoom...

review: Needs Information

[Sam Spilsbury (smspillaz)]

Please revert this change as I mentioned before:

1673 - if (!CompPlugin::checkPluginABI ("core", CORE_ABIVERSION) ||
1674 - !CompPlugin::checkPluginABI ("composite", COMPIZ_COMPOSITE_ABI) ||
1675 - !CompPlugin::checkPluginABI ("opengl", COMPIZ_OPENGL_ABI) ||
1676 - !CompPlugin::checkPluginABI ("mousepoll", COMPIZ_MOUSEPOLL_ABI))
1677 - return false;
1678 + if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION) &&
1679 + CompPlugin::checkPluginABI ("composite", COMPIZ_COMPOSITE_ABI) &&
1680 + CompPlugin::checkPluginABI ("opengl", COMPIZ_OPENGL_ABI) &&
1681 + CompPlugin::checkPluginABI ("mousepoll", COMPIZ_MOUSEPOLL_ABI))
1682 + return true;
1683
1684 - return true;
1685 + return false;

I'd also really prefer it if changes to plugins were done in separate branches for each semantic change - it makes review a lot easier and means that we don't have to hold up whole branches because of some unrelated change which doesn't fit with the broader project goals. I won't require it here, but please keep this in mind in future.

Other than the ABI change, the other changes seem fine to me, keep up the good work.

[MC Return (mc-return)]

> Please revert this change as I mentioned before:
>
> 1673 - if (!CompPlugin::checkPluginABI ("core", CORE_ABIVERSION) ||
> 1674 - !CompPlugin::checkPluginABI ("composite", COMPIZ_COMPOSITE_ABI) ||
> 1675 - !CompPlugin::checkPluginABI ("opengl", COMPIZ_OPENGL_ABI) ||
> 1676 - !CompPlugin::checkPluginABI ("mousepoll", COMPIZ_MOUSEPOLL_ABI))
> 1677 - return false;
> 1678 + if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION) &&
> 1679 + CompPlugin::checkPluginABI ("composite", COMPIZ_COMPOSITE_ABI) &&
> 1680 + CompPlugin::checkPluginABI ("opengl", COMPIZ_OPENGL_ABI) &&
> 1681 + CompPlugin::checkPluginABI ("mousepoll", COMPIZ_MOUSEPOLL_ABI))
> 1682 + return true;
> 1683
> 1684 - return true;
> 1685 + return false;
>
Ack, will propose those separately as stated in the other MP already.

> I'd also really prefer it if changes to plugins were done in separate branches
> for each semantic change - it makes review a lot easier and means that we
> don't have to hold up whole branches because of some unrelated change which
> doesn't fit with the broader project goals. I won't require it here, but
> please keep this in mind in future.
>
The real problem here is that this will fail to autoland, because of the quilt patching.
So this MP "Needs Information" on how to continue with this:

The correct solution would be:
1. Remove the quilt patch from ezoom completely
2. Disable EZoom for Ubuntu per default

Why is this the better solution you might ask:

1. The plugin does not get loaded at all, which improves memory usage, boot speed and Compiz start speed, while not changing any functionality for the Ubuntu user.
2. The stupid quilt patch is removed from lp:compiz, making hacking and testing the code much easier.
3. It is easier for the user to re-enable EZoom, because they do not have to manually add all the shortcuts to make the plugin usable again, instead they just have to enable the plugin.

> Other than the ABI change, the other changes seem fine to me, keep up the good
> work.

I'll revert the ABI logic change here as well.

[Sam Spilsbury (smspillaz)]

> The real problem here is that this will fail to autoland, because of the quilt
> patching.
> So this MP "Needs Information" on how to continue with this:
>
> The correct solution would be:
> 1. Remove the quilt patch from ezoom completely
> 2. Disable EZoom for Ubuntu per default
>
> Why is this the better solution you might ask:
>
> 1. The plugin does not get loaded at all, which improves memory usage, boot
> speed and Compiz start speed, while not changing any functionality for the
> Ubuntu user.
> 2. The stupid quilt patch is removed from lp:compiz, making hacking and
> testing the code much easier.
> 3. It is easier for the user to re-enable EZoom, because they do not have to
> manually add all the shortcuts to make the plugin usable again, instead they
> just have to enable the plugin.
>

It should be possible to just refresh the patches. Was there anything preventing you from doing that?

[MC Return (mc-return)]

> > The real problem here is that this will fail to autoland, because of the
> quilt
> > patching.
> > So this MP "Needs Information" on how to continue with this:
> >
> > The correct solution would be:
> > 1. Remove the quilt patch from ezoom completely
> > 2. Disable EZoom for Ubuntu per default
> >
> > Why is this the better solution you might ask:
> >

> >
>
>
> It should be possible to just refresh the patches. Was there anything
> preventing you from doing that?

Yes, 4 reasons:

1. Because the correct solution would be:
* Remove the quilt patch from ezoom completely
* Disable EZoom for Ubuntu per default

Why ?

Reason 2: The plugin does not get loaded at all, which improves memory usage, boot
speed and Compiz start speed, while not changing any functionality for the
Ubuntu user.

Reason 3: The stupid quilt patch is removed from lp:compiz, making hacking and
testing the code much easier, without having to think about quilt.

Reason 4: It is easier for the user to re-enable EZoom, because they do not have to
manually add all the shortcuts to make the plugin usable again, instead they
just have to enable the plugin.

Reason 5: For users who already have their own EZoom configuration set up the removed
quilt patch also makes no difference.

Are there *any* reasons to keep the quilt patch for EZoom ? I do not see any...

review: Needs Information

[MC Return (mc-return)]

*5 reasons ^^ -> typo ;)

[Sam Spilsbury (smspillaz)]

> Yes, 4 reasons:
>
> 1. Because the correct solution would be:
> * Remove the quilt patch from ezoom completely
> * Disable EZoom for Ubuntu per default
>
> Why ?
>
> Reason 2: The plugin does not get loaded at all, which improves memory usage, boot
> speed and Compiz start speed, while not changing any functionality for the
> Ubuntu user.
>
> Reason 3: The stupid quilt patch is removed from lp:compiz, making hacking and
> testing the code much easier, without having to think about quilt.
>
> Reason 4: It is easier for the user to re-enable EZoom, because they do not have to
> manually add all the shortcuts to make the plugin usable again, instead they
> just have to enable the plugin.
>
> Reason 5: For users who already have their own EZoom configuration set up the removed
> quilt patch also makes no difference.
>
> Are there *any* reasons to keep the quilt patch for EZoom ? I do not see any...
>

I think didrocks made it quite clear why we patch the xml files in the
distribution. I don't think its worth going over those arguments
again. What I want to know is - is there any reason why you are not
able to refresh the patches? It seems like a fairly straightforward
thing to do.

--
Sam Spilsbury

[MC Return (mc-return)]

>
> I think didrocks made it quite clear why we patch the xml files in the
> distribution. I don't think its worth going over those arguments
> again. What I want to know is - is there any reason why you are not
> able to refresh the patches? It seems like a fairly straightforward
> thing to do.
>
> --
> Sam Spilsbury

Sam, maybe I misunderstood didrocks here:

Apr 25 10:12:35 <didrocks> MCR: want to tackle that? :)
Apr 25 10:12:40 <MCR> didrocks, soon it won't be risky anymore
Apr 25 10:16:34 <MCR> didrocks, I want to reduce Ubuntu-distro-patches against Compiz to a minimum... it makes changes to .xml.in files and code to a small horror-trip ;)

He explained the rationale behind 'why it is as it is', which is that it once was risky to enable ezoom in CCSM making Compiz crash by doing it. To prevent this crash from happening it was left enabled by default,
while removing the shortcuts, which make sense as they are in upstream and thus should not be changed, but would conflict with Unity.
To enable EZoom the user then just had to make new shortcuts to reenable EZoom.

I will have to do quilting anyway for the other plugins' changes, but I think we really should use quilt patching just for things that are inevitable and need quilt to be solved, here this is simply not the case at all...

review: Needs Information

[MC Return (mc-return)]

Further investigation also shows, that there is no other way than CCSM or GConf Editor to change the shortcuts (ezoom shortcuts are not even in gnome/unity keyboard shortcut settings) -> which means the user needs CCSM or some other editor in any case to change the shortcuts of ezoom...

[MC Return (mc-return)]

Here I still propose the no-quilt-involvement version of things...
(just needs the okay to be implemented)

[Sam Spilsbury (smspillaz)]

> Here I still propose the no-quilt-involvement version of things...
> (just needs the okay to be implemented)

Just refresh the patches. Its really not hard and I really don't want to be involved in yet another debate about how we should change ubuntu policy over this.

review: Needs Fixing

[MC Return (mc-return)]

>
> Just refresh the patches. Its really not hard and I really don't want to be
> involved in yet another debate about how we should change ubuntu policy over
> this.

I can talk to didrocks again to ask for the green light here, I really would prefer eliminating the quilt patch in this case, because it makes things worse, not better...
All 5 arguments against the patch are still valid.

[MC Return (mc-return)]

Talked with didrocks again, short summary:

1. Default should not be changed.
2. If possible the EZoom settings should be exposed in gcc, which they are currently not.
   Instead some gnome-zoom settings are exposed in gcc, which of course do not work at all ("Zoom in/Zoom out" and "Turn zoom on or off"), because they seem to be designed for gnome-shell only.

So I will have to refresh the patch in our case here for now, so this can finally land...

WIP

[MC Return (mc-return)]

Finally ready and re-quilted.

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

All good now, thanks!

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'debian/patches/ubuntu-config.patch'
--- debian/patches/ubuntu-config.patch	2013-05-12 15:39:28 +0000
+++ debian/patches/ubuntu-config.patch	2013-05-14 21:54:27 +0000
@@ -337,41 +337,6 @@
  		    </option>
  		    <option name="ground_color1" type="color">
  			<_short>Ground color(near)</_short>
---- a/plugins/ezoom/ezoom.xml.in
-+++ b/plugins/ezoom/ezoom.xml.in
-@@ -29,7 +29,6 @@
- 		<option type="button" name="zoom_in_button">
- 		    <_short>Zoom In</_short>
- 		    <_long>Zoom In</_long>
--		    <default>&lt;Super&gt;Button4</default>
- 		</option>
- 		<option type="key" name="zoom_in_key">
- 		    <_short>Zoom In</_short>
-@@ -38,7 +37,6 @@
- 		<option type="button" name="zoom_out_button">
- 		    <_short>Zoom Out</_short>
- 		    <_long>Zoom Out</_long>
--		    <default>&lt;Super&gt;Button5</default>
- 		</option>
- 		<option type="key" name="zoom_out_key">
- 		    <_short>Zoom Out</_short>
-@@ -47,7 +45,6 @@
- 		<option type="button" name="zoom_box_button">
- 		    <_short>Zoom Box</_short>
- 		    <_long>Zoom in on a boxed area</_long>
--		    <default>&lt;Super&gt;Button2</default>
- 		</option>
- 	    </group>
- 	    <group>
-@@ -141,7 +138,7 @@
- 		    <option type="key" name="fit_to_window_key">
- 			<_short>Fit zoomed area to window</_short>
- 			<_long>Zooms in/out so the focused window is zoomed to the maximum while still being fully visible.</_long>
--			<default>&lt;Super&gt;r</default>
-+			<default></default>
- 		    </option>
- 		</subgroup>
- 	    </group>
 --- a/plugins/fade/fade.xml.in
 +++ b/plugins/fade/fade.xml.in
 @@ -10,6 +10,7 @@
@@ -835,3 +800,41 @@
  		</option>
  	    </group>
  	    <group>
+--- a/plugins/ezoom/ezoom.xml.in
++++ b/plugins/ezoom/ezoom.xml.in
+@@ -30,7 +30,7 @@
+ 		<option type="button" name="zoom_in_button">
+ 		    <_short>Zoom In Button</_short>
+ 		    <_long>Mouse button shortcut to invoke zooming in.</_long>
+-		    <default>&lt;Super&gt;Button4</default>
++		    <default></default>
+ 		</option>
+ 		<option type="key" name="zoom_in_key">
+ 		    <_short>Zoom In Key</_short>
+@@ -39,7 +39,7 @@
+ 		<option type="button" name="zoom_out_button">
+ 		    <_short>Zoom Out Button</_short>
+ 		    <_long>Mouse button shortcut to invoke zooming out.</_long>
+-		    <default>&lt;Super&gt;Button5</default>
++		    <default></default>
+ 		</option>
+ 		<option type="key" name="zoom_out_key">
+ 		    <_short>Zoom Out Key</_short>
+@@ -48,7 +48,7 @@
+ 		<option type="button" name="zoom_box_button">
+ 		    <_short>Invoke Zoom Box Button</_short>
+ 		    <_long>Define a rectangle area and zoom into it.</_long>
+-		    <default>&lt;Super&gt;Button2</default>
++		    <default></default>
+ 		</option>
+ 		<option name="zoom_box_outline_color" type="color">
+ 			<_short>Zoom Box Outline Color</_short>
+@@ -162,7 +162,7 @@
+ 		    <option type="key" name="fit_to_window_key">
+ 			<_short>Fit zoomed area to window</_short>
+ 			<_long>Zooms in/out so the focused window is zoomed to the maximum while still being fully visible.</_long>
+-			<default>&lt;Super&gt;r</default>
++			<default></default>
+ 		    </option>
+ 		</subgroup>
+ 	    </group>

=== modified file 'plugins/ezoom/ezoom.xml.in'
--- plugins/ezoom/ezoom.xml.in	2012-05-16 17:40:27 +0000
+++ plugins/ezoom/ezoom.xml.in	2013-05-14 21:54:27 +0000
@@ -6,6 +6,7 @@
 		<plugin>opengl</plugin>
 		<plugin>expo</plugin>
 		<plugin>decor</plugin>
+		<plugin>mousepoll</plugin>
 	    </relation>
 	    <relation type="before">
 		<plugin>staticswitcher</plugin>
@@ -27,28 +28,48 @@
 		<option type="action" name="ensure_visibility">
 		</option>
 		<option type="button" name="zoom_in_button">
-		    <_short>Zoom In</_short>
-		    <_long>Zoom In</_long>
+		    <_short>Zoom In Button</_short>
+		    <_long>Mouse button shortcut to invoke zooming in.</_long>
 		    <default>&lt;Super&gt;Button4</default>
 		</option>
 		<option type="key" name="zoom_in_key">
-		    <_short>Zoom In</_short>
-		    <_long>Zoom In</_long>
+		    <_short>Zoom In Key</_short>
+		    <_long>Key shortcut to invoke zooming in.</_long>
 		</option>
 		<option type="button" name="zoom_out_button">
-		    <_short>Zoom Out</_short>
-		    <_long>Zoom Out</_long>
+		    <_short>Zoom Out Button</_short>
+		    <_long>Mouse button shortcut to invoke zooming out.</_long>
 		    <default>&lt;Super&gt;Button5</default>
 		</option>
 		<option type="key" name="zoom_out_key">
-		    <_short>Zoom Out</_short>
-		    <_long>Zoom Out</_long>
+		    <_short>Zoom Out Key</_short>
+		    <_long>Key shortcut to invoke zooming in.</_long>
 		</option>
 		<option type="button" name="zoom_box_button">
-		    <_short>Zoom Box</_short>
-		    <_long>Zoom in on a boxed area</_long>
+		    <_short>Invoke Zoom Box Button</_short>
+		    <_long>Define a rectangle area and zoom into it.</_long>
 		    <default>&lt;Super&gt;Button2</default>
 		</option>
+		<option name="zoom_box_outline_color" type="color">
+			<_short>Zoom Box Outline Color</_short>
+			<_long>Color and opacity of the zoom box outline.</_long>
+			<default>
+			    <red>0x2fff</red>
+			    <green>0x2fff</green>
+			    <blue>0x4fff</blue>
+			    <alpha>0x9fff</alpha>
+			</default>
+		</option>
+		<option name="zoom_box_fill_color" type="color">
+			<_short>Zoom Box Fill Color</_short>
+			<_long>Fill color and opacity of the zoom box.</_long>
+			<default>
+			    <red>0x2fff</red>
+			    <green>0x2fff</green>
+			    <blue>0x2fff</blue>
+			    <alpha>0x4fff</alpha>
+			</default>
+		</option>
 	    </group>
 	    <group>
 		<_short>Mouse Behaviour</_short>

=== modified file 'plugins/ezoom/src/ezoom.cpp'
--- plugins/ezoom/src/ezoom.cpp	2013-05-09 13:43:07 +0000
+++ plugins/ezoom/src/ezoom.cpp	2013-05-14 21:54:27 +0000
@@ -79,7 +79,7 @@
 
 /*
  * This toggles paint functions. We don't need to continually run code when we
- * are not doing anything
+ * are not doing anything.
  */
 static inline void
 toggleFunctions (bool state)
@@ -102,6 +102,7 @@
 	return false;
     else if ((unsigned int) out >= zs->zooms.size ())
 	zs->zooms.resize (screen->outputDevs ().size ());
+
     return true;
 }
 
@@ -113,13 +114,15 @@
 
     if (!outputIsZoomArea (out))
 	return false;
+
     if (zs->grabbed & (1 << zs->zooms.at (out).output))
 	return true;
+
     return false;
 }
 
 /* Check if we are zoomed out and not going anywhere
- * (similar to isActive but based on actual zoom, not grab)
+ * (similar to isActive but based on actual zoom, not grab).
  */
 static inline bool
 isZoomed (int out)
@@ -129,11 +132,9 @@
     if (!outputIsZoomArea (out))
 	return false;
 
-    if (zs->zooms.at (out).currentZoom != 1.0f
-	|| zs->zooms.at (out).newZoom != 1.0f)
-	return true;
-
-    if (zs->zooms.at (out).zVelocity != 0.0f)
+    if (zs->zooms.at (out).currentZoom != 1.0f	||
+	zs->zooms.at (out).newZoom != 1.0f	||
+	zs->zooms.at (out).zVelocity != 0.0f)
 	return true;
 
     return false;
@@ -143,11 +144,12 @@
 int
 EZoomScreen::distanceToEdge (int out, EZoomScreen::ZoomEdge edge)
 {
-    int        x1,y1,x2,y2;
+    int        x1, y1, x2, y2;
     CompOutput *o = &screen->outputDevs ().at (out);
 
     if (!isActive (out))
 	return 0;
+
     convertToZoomedTarget (out, o->region ()->extents.x2,
 			   o->region ()->extents.y2, &x2, &y2);
     convertToZoomedTarget (out, o->region ()->extents.x1,
@@ -162,7 +164,7 @@
     return 0; // Never reached.
 }
 
-/* Update/set translations based on zoom level and real translate.  */
+/* Update/set translations based on zoom level and real translate. */
 void
 EZoomScreen::ZoomArea::updateActualTranslates ()
 {
@@ -178,17 +180,19 @@
 bool
 EZoomScreen::isInMovement (int out)
 {
-    if (zooms.at (out).currentZoom == 1.0f &&
-	zooms.at (out).newZoom == 1.0f &&
+    if (zooms.at (out).currentZoom == 1.0f  &&
+	zooms.at (out).newZoom == 1.0f	    &&
 	zooms.at (out).zVelocity == 0.0f)
 	return false;
-    if (zooms.at (out).currentZoom != zooms.at (out).newZoom ||
-	zooms.at (out).xVelocity || zooms.at (out).yVelocity ||
-	zooms.at (out).zVelocity)
-	return true;
-    if (zooms.at (out).xTranslate != zooms.at (out).realXTranslate ||
+
+    if (zooms.at (out).currentZoom != zooms.at (out).newZoom	    ||
+	zooms.at (out).xVelocity				    ||
+	zooms.at (out).yVelocity				    ||
+	zooms.at (out).zVelocity				    ||
+	zooms.at (out).xTranslate != zooms.at (out).realXTranslate  ||
 	zooms.at (out).yTranslate != zooms.at (out).realYTranslate)
 	return true;
+
     return false;
 }
 
@@ -225,16 +229,15 @@
     locked (false)
 {
 }
-/* Adjust the velocity in the z-direction.  */
+/* Adjust the velocity in the z-direction. */
 void
 EZoomScreen::adjustZoomVelocity (int out, float chunk)
 {
-    float d, adjust, amount;
-
-    d = (zooms.at (out).newZoom - zooms.at (out).currentZoom) * 75.0f;
-
-    adjust = d * 0.002f;
-    amount = fabs (d);
+    float d = (zooms.at (out).newZoom - zooms.at (out).currentZoom) * 75.0f;
+
+    float adjust = d * 0.002f;
+    float amount = fabs (d);
+
     if (amount < 1.0f)
 	amount = 1.0f;
     else if (amount > 5.0f)
@@ -249,10 +252,8 @@
 	zooms.at (out).zVelocity = 0.0f;
     }
     else
-    {
 	zooms.at (out).currentZoom += (zooms.at (out).zVelocity * chunk) /
-	    cScreen->redrawTime ();
-    }
+				      cScreen->redrawTime ();
 }
 
 /* Adjust the X/Y velocity based on target translation and real
@@ -260,32 +261,28 @@
 void
 EZoomScreen::adjustXYVelocity (int out, float chunk)
 {
-    float xdiff, ydiff;
-    float xadjust, yadjust;
-    float xamount, yamount;
-
     zooms.at (out).xVelocity /= 1.25f;
     zooms.at (out).yVelocity /= 1.25f;
-    xdiff =
-	(zooms.at (out).xTranslate - zooms.at (out).realXTranslate) *
-	75.0f;
-    ydiff =
-	(zooms.at (out).yTranslate - zooms.at (out).realYTranslate) *
-	75.0f;
-    xadjust = xdiff * 0.002f;
-    yadjust = ydiff * 0.002f;
-    xamount = fabs (xdiff);
-    yamount = fabs (ydiff);
+
+    float xdiff =
+	(zooms.at (out).xTranslate - zooms.at (out).realXTranslate) * 75.0f;
+    float ydiff =
+	(zooms.at (out).yTranslate - zooms.at (out).realYTranslate) * 75.0f;
+
+    float xadjust = xdiff * 0.002f;
+    float yadjust = ydiff * 0.002f;
+    float xamount = fabs (xdiff);
+    float yamount = fabs (ydiff);
 
     if (xamount < 1.0f)
-	    xamount = 1.0f;
+	xamount = 1.0f;
     else if (xamount > 5.0)
-	    xamount = 5.0f;
+	xamount = 5.0f;
 
     if (yamount < 1.0f)
-	    yamount = 1.0f;
+	yamount = 1.0f;
     else if (yamount > 5.0)
-	    yamount = 5.0f;
+	yamount = 5.0f;
 
     zooms.at (out).xVelocity =
 	(xamount * zooms.at (out).xVelocity + xadjust) / (xamount + 1.0f);
@@ -308,24 +305,23 @@
 	(zooms.at (out).yVelocity * chunk) / cScreen->redrawTime ();
 }
 
-/* Animate the movement (if any) in preparation of a paint screen.  */
+/* Animate the movement (if any) in preparation of a paint screen. */
 void
-EZoomScreen::preparePaint (int	   msSinceLastPaint)
+EZoomScreen::preparePaint (int msSinceLastPaint)
 {
     if (grabbed)
     {
-	int   steps;
-	float amount, chunk;
+	float amount = msSinceLastPaint * 0.05f * optionGetSpeed ();
+	int steps  = amount / (0.5f * optionGetTimestep ());
 
-	amount = msSinceLastPaint * 0.05f * optionGetSpeed ();
-	steps  = amount / (0.5f * optionGetTimestep ());
 	if (!steps)
-	       	steps = 1;
-	chunk  = amount / (float) steps;
+	    steps = 1;
+
+	float chunk  = amount / (float) steps;
+
 	while (steps--)
 	{
-	    unsigned int out;
-	    for (out = 0; out < zooms.size (); out++)
+	    for (unsigned int out = 0; out < zooms.size (); ++out)
 	    {
 		if (!isInMovement (out) || !isActive (out))
 		    continue;
@@ -333,11 +329,13 @@
 		adjustXYVelocity (out, chunk);
 		adjustZoomVelocity (out, chunk);
 		zooms.at (out).updateActualTranslates ();
+
 		if (!isZoomed (out))
 		{
 		    zooms.at (out).xVelocity = zooms.at (out).yVelocity =
 			0.0f;
 		    grabbed &= ~(1 << zooms.at (out).output);
+
 		    if (!grabbed)
 		    {
 			cScreen->damageScreen ();
@@ -353,14 +351,13 @@
     cScreen->preparePaint (msSinceLastPaint);
 }
 
-/* Damage screen if we're still moving.  */
+/* Damage screen if we're still moving. */
 void
 EZoomScreen::donePaint ()
 {
     if (grabbed)
     {
-	unsigned int out;
-	for (out = 0; out < zooms.size (); out++)
+	for (unsigned int out = 0; out < zooms.size (); ++out)
 	{
 	    if (isInMovement (out) && isActive (out))
 	    {
@@ -372,41 +369,48 @@
     else if (grabIndex)
 	cScreen->damageScreen ();
     else
-        toggleFunctions (false);
+	toggleFunctions (false);
 
     cScreen->donePaint ();
 }
-/* Draws a box from the screen coordinates inx1,iny1 to inx2,iny2 */
+
+/* Draws a box from the screen coordinates inx1, iny1 to inx2, iny2. */
 void
 EZoomScreen::drawBox (const GLMatrix &transform,
-		     CompOutput          *output,
-		     CompRect             box)
+		      CompOutput     *output,
+		      CompRect       box)
 {
-    GLMatrix zTransform (transform);
-    int           x1,x2,y1,y2;
-    int		  inx1, inx2, iny1, iny2;
-    int	          out = output->id ();
-    GLushort colorData[4];
-    GLfloat  vertexData[12];
+    GLMatrix       zTransform (transform);
+    int            inx1, inx2, iny1, iny2;
+    int            out = output->id ();
+    GLushort       colorData[4];
+    GLushort       *color;
+    GLfloat        vertexData[12];
     GLVertexBuffer *streamingBuffer = GLVertexBuffer::streamingBuffer ();
 
     zTransform.toScreenSpace (output, -DEFAULT_Z_CAMERA);
     convertToZoomed (out, box.x1 (), box.y1 (), &inx1, &iny1);
     convertToZoomed (out, box.x2 (), box.y2 (), &inx2, &iny2);
 
-    x1 = MIN (inx1, inx2);
-    y1 = MIN (iny1, iny2);
-    x2 = MAX (inx1, inx2);
-    y2 = MAX (iny1, iny2);
-
-    streamingBuffer->begin (GL_TRIANGLE_STRIP);
-
-    colorData[0] = 0x2fff;
-    colorData[1] = 0x2fff;
-    colorData[2] = 0x2fff;
-    colorData[3] = 0x4fff;
-
-    streamingBuffer->addColors (1, colorData);
+    /* We can move in both directions from our starting point
+     * so we need to calculate the right coordinates first. */
+    int x1 = MIN (inx1, inx2);
+    int y1 = MIN (iny1, iny2);
+    int x2 = MAX (inx1, inx2);
+    int y2 = MAX (iny1, iny2);
+
+    const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();
+
+    glEnable (GL_BLEND);
+
+    /* Draw filled rectangle */
+    float alpha = optionGetZoomBoxFillColorAlpha () / MaxUShortFloat;
+    color = optionGetZoomBoxFillColor ();
+
+    colorData[0] = alpha * color[0];
+    colorData[1] = alpha * color[1];
+    colorData[2] = alpha * color[2];
+    colorData[3] = alpha * MaxUShortFloat;
 
     vertexData[0]  = x1;
     vertexData[1]  = y1;
@@ -421,20 +425,22 @@
     vertexData[10] = y2;
     vertexData[11] = 0.0f;
 
+    streamingBuffer->begin (GL_TRIANGLE_STRIP);
+
+    streamingBuffer->addColors (1, colorData);
     streamingBuffer->addVertices (4, vertexData);
 
     streamingBuffer->end ();
     streamingBuffer->render (zTransform);
 
-
-    streamingBuffer->begin (GL_LINE_LOOP);
-
-    colorData[0] = 0x2fff;
-    colorData[1] = 0x2fff;
-    colorData[2] = 0x4fff;
-    colorData[3] = 0x9fff;
-
-    streamingBuffer->addColors (1, colorData);
+    /* Draw outline */
+    alpha = optionGetZoomBoxOutlineColorAlpha () / MaxUShortFloat;
+    color = optionGetZoomBoxOutlineColor ();
+
+    colorData[0] = alpha * color[0];
+    colorData[1] = alpha * color[1];
+    colorData[2] = alpha * color[2];
+    colorData[3] = alpha * MaxUShortFloat;
 
     vertexData[0]  = x1;
     vertexData[1]  = y1;
@@ -449,20 +455,34 @@
     vertexData[10] = y2;
     vertexData[11] = 0.0f;
 
+    glLineWidth (2.0);
+
+    streamingBuffer->begin (GL_LINE_LOOP);
+
+    streamingBuffer->addColors (1, colorData);
     streamingBuffer->addVertices (4, vertexData);
 
     streamingBuffer->end ();
     streamingBuffer->render (zTransform);
+
+    glDisable (GL_BLEND);
+
+    /* Damage the zoom selection box region during draw. */
+    cScreen->damageRegion (CompRegion (x1 - 1,
+				       y1 - 1,
+				       x2 - x1 + 1,
+				       y2 - y1 + 1));
 }
+
 /* Apply the zoom if we are grabbed.
  * Make sure to use the correct filter.
  */
 bool
 EZoomScreen::glPaintOutput (const GLScreenPaintAttrib &attrib,
-			   const GLMatrix	     &transform,
-			   const CompRegion	     &region,
-			   CompOutput 		     *output,
-			   unsigned int		     mask)
+			    const GLMatrix            &transform,
+			    const CompRegion          &region,
+			    CompOutput                *output,
+			    unsigned int              mask)
 {
     bool status;
     int	 out = output->id ();
@@ -476,8 +496,8 @@
 	mask |= PAINT_SCREEN_CLEAR_MASK;
 
 	zTransform.scale (1.0f / zooms.at (out).currentZoom,
-		          1.0f / zooms.at (out).currentZoom,
-		          1.0f);
+			  1.0f / zooms.at (out).currentZoom,
+			  1.0f);
 	zTransform.translate (zooms.at (out).xtrans,
 			      zooms.at (out).ytrans,
 			      0);
@@ -490,10 +510,9 @@
 
     }
     else
-    {
 	status = gScreen->glPaintOutput (attrib, transform, region, output,
-									mask);
-    }
+					 mask);
+
     if (grabIndex)
 	drawBox (transform, output, box);
 
@@ -502,14 +521,13 @@
 
 /* Makes sure we're not attempting to translate too far.
  * We are restricted to 0.5 to not go beyond the end
- * of the screen/head.  */
+ * of the screen/head. */
 static inline void
 constrainZoomTranslate ()
 {
-    unsigned int out;
     ZOOM_SCREEN (screen);
 
-    for (out = 0; out < zs->zooms.size (); out++)
+    for (unsigned int out = 0; out < zs->zooms.size (); ++out)
     {
 	if (zs->zooms.at (out).xTranslate > 0.5f)
 	    zs->zooms.at (out).xTranslate = 0.5f;
@@ -526,7 +544,7 @@
 /* Functions for adjusting the zoomed area.
  * These are the core of the zoom plug-in; Anything wanting
  * to adjust the zoomed area must use setCenter or setZoomArea
- * and setScale or front ends to them.  */
+ * and setScale or front ends to them. */
 
 /* Sets the center of the zoom area to X,Y.
  * We have to be able to warp the pointer here: If we are moved by
@@ -568,20 +586,19 @@
  * The math could probably be cleaned up, but should be correct now. */
 void
 EZoomScreen::setZoomArea (int        x,
-	     		 int        y,
-	     		 int        width,
-	     		 int        height,
-	     		 bool       instant)
+			  int        y,
+			  int        width,
+			  int        height,
+			  bool       instant)
 {
     CompWindow::Geometry outGeometry (x, y, width, height, 0);
     int         out = screen->outputDeviceForGeometry (outGeometry);
     CompOutput  *o = &screen->outputDevs ().at (out);
 
-    if (zooms.at (out).newZoom == 1.0f)
+    if (zooms.at (out).newZoom == 1.0f ||
+	zooms.at (out).locked)
 	return;
 
-    if (zooms.at (out).locked)
-	return;
     zooms.at (out).xTranslate =
 	 (float) -((o->width () / 2) - (x + (width / 2) - o->x1 ()))
 	/ (o->width ());
@@ -607,9 +624,9 @@
 void
 EZoomScreen::areaToWindow (CompWindow *w)
 {
-    int left = w->serverX () - w->border ().left;
-    int width = w->width () + w->border ().left + w->border ().right;
-    int top = w->serverY () - w->border ().top;
+    int left   = w->serverX () - w->border ().left;
+    int width  = w->width () + w->border ().left + w->border ().right;
+    int top    = w->serverY () - w->border ().top;
     int height = w->height ()  + w->border ().top + w->border ().bottom;
 
     setZoomArea (left, top, width, height, false);
@@ -620,9 +637,7 @@
 void
 EZoomScreen::panZoom (int xvalue, int yvalue)
 {
-    unsigned int out;
-
-    for (out = 0; out < zooms.size (); out++)
+    for (unsigned int out = 0; out < zooms.size (); ++out)
     {
 	zooms.at (out).xTranslate +=
 	    optionGetPanFactor () * xvalue *
@@ -661,6 +676,7 @@
     {
 	if (!pollHandle.active ())
 	    enableMousePolling ();
+
 	grabbed |= (1 << zooms.at (out).output);
 	cursorZoomActive (out);
     }
@@ -702,23 +718,22 @@
 void
 EZoomScreen::syncCenterToMouse ()
 {
-    int         x, y;
-    int         out;
     CompOutput  *o;
 
-    out = screen->outputDeviceForPoint (mouse.x (), mouse.y ());
+    int out = screen->outputDeviceForPoint (mouse.x (), mouse.y ());
     o = &screen->outputDevs ().at (out);
 
     if (!isInMovement (out))
 	return;
 
-    x = (int) ((zooms.at (out).realXTranslate * o->width ()) +
-	       (o->width () / 2) + o->x1 ());
-    y = (int) ((zooms.at (out).realYTranslate * o->height ()) +
-	       (o->height () / 2) + o->y1 ());
+    int x = (int) ((zooms.at (out).realXTranslate * o->width ()) +
+		   (o->width () / 2) + o->x1 ());
+    int y = (int) ((zooms.at (out).realYTranslate * o->height ()) +
+		   (o->height () / 2) + o->y1 ());
 
-    if ((x != mouse.x () || y != mouse.y ())
-	&& grabbed && zooms.at (out).newZoom != 1.0f)
+    if ((x != mouse.x () || y != mouse.y ())	&&
+	grabbed					&&
+	zooms.at (out).newZoom != 1.0f)
     {
 	screen->warpPointer (x - pointerX , y - pointerY );
 	mouse.setX (x);
@@ -726,13 +741,13 @@
     }
 }
 
-/* Convert the point X,Y to where it would be when zoomed.  */
+/* Convert the point X, Y to where it would be when zoomed. */
 void
 EZoomScreen::convertToZoomed (int        out,
-			     int        x,
-			     int        y,
-			     int        *resultX,
-			     int        *resultY)
+			      int        x,
+			      int        y,
+			      int        *resultX,
+			      int        *resultY)
 {
     CompOutput *o;
 
@@ -747,25 +762,26 @@
 
     x -= o->x1 ();
     y -= o->y1 ();
+
     *resultX = x - (za.realXTranslate *
 		    (1.0f - za.currentZoom) * o->width ()) - o->width () / 2;
     *resultX /= za.currentZoom;
     *resultX += o->width () / 2;
     *resultX += o->x1 ();
     *resultY = y - (za.realYTranslate *
-		    (1.0f - za.currentZoom) * o->height ()) - o->height ()/ 2;
+		    (1.0f - za.currentZoom) * o->height ()) - o->height () / 2;
     *resultY /= za.currentZoom;
-    *resultY += o->height ()/ 2;
+    *resultY += o->height () / 2;
     *resultY += o->y1 ();
 }
 
-/* Same but use targeted translation, not real */
+/* Same but use targeted translation, not real one. */
 void
 EZoomScreen::convertToZoomedTarget (int	  out,
-			           int	  x,
-			           int	  y,
-			           int	  *resultX,
-			           int	  *resultY)
+				    int	  x,
+				    int	  y,
+				    int	  *resultX,
+				    int	  *resultY)
 {
     CompOutput *o = &screen->outputDevs ().at (out);
 
@@ -779,13 +795,14 @@
 
     x -= o->x1 ();
     y -= o->y1 ();
+
     *resultX = x - (za.xTranslate *
 		    (1.0f - za.newZoom) * o->width ()) - o->width () / 2;
     *resultX /= za.newZoom;
     *resultX += o->width () / 2;
     *resultX += o->x1 ();
     *resultY = y - (za.yTranslate *
-		    (1.0f - za.newZoom) * o->height ()) - o->height ()/2;
+		    (1.0f - za.newZoom) * o->height ()) - o->height () / 2;
     *resultY /= za.newZoom;
     *resultY += o->height () / 2;
     *resultY += o->y1 ();
@@ -799,16 +816,17 @@
 EZoomScreen::ensureVisibility (int x, int y, int margin)
 {
     int         zoomX, zoomY;
-    int         out;
     CompOutput  *o;
 
-    out = screen->outputDeviceForPoint (x, y);
+    int out = screen->outputDeviceForPoint (x, y);
+
     if (!isActive (out))
 	return false;
 
     o = &screen->outputDevs ().at (out);
     convertToZoomedTarget (out, x, y, &zoomX, &zoomY);
     ZoomArea &za = zooms.at (out);
+
     if (za.locked)
 	return false;
 
@@ -846,24 +864,23 @@
  */
 void
 EZoomScreen::ensureVisibilityArea (int         x1,
-				  int         y1,
-				  int         x2,
-				  int         y2,
-				  int         margin,
-				  ZoomGravity gravity)
+				   int         y1,
+				   int         x2,
+				   int         y2,
+				   int         margin,
+				   ZoomGravity gravity)
 {
     int        targetX, targetY, targetW, targetH;
-    int        out;
-    CompOutput *o;
-
-    out = screen->outputDeviceForPoint (x1 + (x2-x1/2), y1 + (y2-y1/2));
-    o = &screen->outputDevs ().at (out);
-
-#define WIDTHOK (float)(x2-x1) / (float)o->width () < zooms.at (out).newZoom
-#define HEIGHTOK (float)(y2-y1) / (float)o->height () < zooms.at (out).newZoom
-
-    if (WIDTHOK &&
-	HEIGHTOK) {
+
+    int out = screen->outputDeviceForPoint (x1 + (x2 - x1 / 2), y1 + (y2 - y1 / 2));
+    CompOutput *o = &screen->outputDevs ().at (out);
+
+    bool widthOkay  = (float)(x2-x1) / (float)o->width () < zooms.at (out).newZoom;
+    bool heightOkay = (float)(y2-y1) / (float)o->height () < zooms.at (out).newZoom;
+
+    if (widthOkay &&
+	heightOkay)
+    {
 	ensureVisibility (x1, y1, margin);
 	ensureVisibility (x2, y2, margin);
 	return;
@@ -874,18 +891,23 @@
 	case NORTHWEST:
 	    targetX = x1;
 	    targetY = y1;
-	    if (WIDTHOK)
+
+	    if (widthOkay)
 		targetW = x2 - x1;
 	    else
 		targetW = o->width () * zooms.at (out).newZoom;
-	    if (HEIGHTOK)
+
+	    if (heightOkay)
 		targetH = y2 - y1;
 	    else
 		targetH = o->height () * zooms.at (out).newZoom;
+
 	    break;
+
 	case NORTHEAST:
 	    targetY = y1;
-	    if (WIDTHOK)
+
+	    if (widthOkay)
 	    {
 		targetX = x1;
 		targetW = x2-x1;
@@ -896,18 +918,22 @@
 		targetW = o->width () * zooms.at (out).newZoom;
 	    }
 
-	    if (HEIGHTOK)
+	    if (heightOkay)
 		targetH = y2-y1;
 	    else
 		targetH = o->height () * zooms.at (out).newZoom;
+
 	    break;
+
 	case SOUTHWEST:
 	    targetX = x1;
-	    if (WIDTHOK)
+
+	    if (widthOkay)
 		targetW = x2-x1;
 	    else
 		targetW = o->width () * zooms.at (out).newZoom;
-	    if (HEIGHTOK)
+
+	    if (heightOkay)
 	    {
 		targetY = y1;
 		targetH = y2-y1;
@@ -917,9 +943,11 @@
 		targetY = y2 - (o->width () * zooms.at (out).newZoom);
 		targetH = o->width () * zooms.at (out).newZoom;
 	    }
+
 	    break;
+
 	case SOUTHEAST:
-	    if (WIDTHOK)
+	    if (widthOkay)
 	    {
 		targetX = x1;
 		targetW = x2-x1;
@@ -930,7 +958,7 @@
 		targetX = x2 - targetW;
 	    }
 
-	    if (HEIGHTOK)
+	    if (heightOkay)
 	    {
 		targetY = y1;
 		targetH = y2 - y1;
@@ -940,15 +968,19 @@
 		targetH = o->height () * zooms.at (out).newZoom;
 		targetY = y2 - targetH;
 	    }
+
 	    break;
+
 	case CENTER:
 	default:
 	    setCenter (x1 + (x2 - x1 / 2), y1 + (y2 - y1 / 2), false);
 	    return;
+
 	    break;
     }
 
     setZoomArea (targetX, targetY, targetW, targetH, false);
+
     return ;
 }
 
@@ -960,18 +992,16 @@
 void
 EZoomScreen::restrainCursor (int out)
 {
-    int         x1, y1, x2, y2, margin;
+    int         x1, y1, x2, y2;
     int         diffX = 0, diffY = 0;
-    int         north, south, east, west;
-    float       z;
     CompOutput  *o = &screen->outputDevs ().at (out);
 
-    z = zooms.at (out).newZoom;
-    margin = optionGetRestrainMargin ();
-    north = distanceToEdge (out, NORTH);
-    south = distanceToEdge (out, SOUTH);
-    east = distanceToEdge (out, EAST);
-    west = distanceToEdge (out, WEST);
+    float z      = zooms.at (out).newZoom;
+    int   margin = optionGetRestrainMargin ();
+    int   north  = distanceToEdge (out, NORTH);
+    int   south  = distanceToEdge (out, SOUTH);
+    int   east   = distanceToEdge (out, EAST);
+    int   west   = distanceToEdge (out, WEST);
 
     if (zooms.at (out).currentZoom == 1.0f)
     {
@@ -990,6 +1020,7 @@
     if ((x2 - x1 > o->x2 () - o->x1 ()) ||
        (y2 - y1 > o->y2 () - o->y1 ()))
 	return;
+
     if (x2 > o->x2 () - margin && east > 0)
 	diffX = x2 - o->x2 () + margin;
     else if (x1 < o->x1 () + margin && west > 0)
@@ -1002,9 +1033,9 @@
 
     if (abs(diffX)*z > 0  || abs(diffY)*z > 0)
 	screen->warpPointer ((int) (mouse.x () - pointerX) -
-						       (int) ((float)diffX * z),
-		     	     (int) (mouse.y () - pointerY) -
-						      (int) ((float)diffY * z));
+				 (int) ((float)diffX * z),
+			     (int) (mouse.y () - pointerY) -
+				 (int) ((float)diffY * z));
 }
 
 /* Check if the cursor is still visible.
@@ -1016,16 +1047,14 @@
 void
 EZoomScreen::cursorMoved ()
 {
-    int         out;
+    int out = screen->outputDeviceForPoint (mouse.x (), mouse.y ());
 
-    out = screen->outputDeviceForPoint (mouse.x (), mouse.y ());
     if (isActive (out))
     {
 	if (optionGetRestrainMouse ())
 	    restrainCursor (out);
 
 	if (optionGetZoomMode () == EzoomOptions::ZoomModePanArea)
-	{
 	    ensureVisibilityArea (mouse.x () - cursor.hotX,
 				  mouse.y () - cursor.hotY,
 				  mouse.x () + cursor.width -
@@ -1034,14 +1063,11 @@
 				  cursor.hotY,
 				  optionGetRestrainMargin (),
 				  NORTHWEST);
-	}
 
 	cursorZoomActive (out);
     }
     else
-    {
 	cursorZoomInactive ();
-    }
 }
 
 /* Update the mouse position.
@@ -1051,14 +1077,16 @@
 void
 EZoomScreen::updateMousePosition (const CompPoint &p)
 {
-    int out;
     mouse.setX (p.x ());
     mouse.setY (p.y ());
-    out = screen->outputDeviceForPoint (mouse.x (), mouse.y ());
+
+    int out = screen->outputDeviceForPoint (mouse.x (), mouse.y ());
     lastChange = time(NULL);
+
     if (optionGetZoomMode () == EzoomOptions::ZoomModeSyncMouse &&
-        !isInMovement (out))
+	!isInMovement (out))
 	setCenter (mouse.x (), mouse.y (), true);
+
     cursorMoved ();
     cScreen->damageScreen ();
 }
@@ -1073,6 +1101,7 @@
     if (!grabbed)
     {
 	cursorMoved ();
+
 	if (pollHandle.active ())
 	    pollHandle.stop ();
     }
@@ -1092,25 +1121,25 @@
 
 /* Translate into place and draw the scaled cursor.  */
 void
-EZoomScreen::drawCursor (CompOutput          *output,
-	    		const GLMatrix      &transform)
+EZoomScreen::drawCursor (CompOutput    *output,
+			const GLMatrix &transform)
 {
-    int         out = output->id ();
+    int out = output->id ();
 
     if (cursor.isSet)
     {
-	GLMatrix      sTransform = transform;
-	float	      scaleFactor;
-	int           ax, ay, x, y;
-	GLfloat         textureData[8];
-	GLfloat         vertexData[12];
+	GLMatrix       sTransform = transform;
+	float          scaleFactor;
+	int            ax, ay;
+	GLfloat        textureData[8];
+	GLfloat        vertexData[12];
 	GLVertexBuffer *streamingBuffer = GLVertexBuffer::streamingBuffer ();
 
 	/*
 	 * XXX: expo knows how to handle mouse when zoomed, so we back off
 	 * when expo is active.
 	 */
-	if (screen->grabExist ( "expo"))
+	if (screen->grabExist ("expo"))
 	{
 	    cursorZoomInactive ();
 	    return;
@@ -1126,11 +1155,10 @@
 	    scaleFactor = 1.0f / optionGetScaleMouseStatic ();
 
 	sTransform.scale (scaleFactor, scaleFactor, 1.0f);
-	x = -cursor.hotX;
-	y = -cursor.hotY;
+	int x = -cursor.hotX;
+	int y = -cursor.hotY;
 
 	glEnable (GL_BLEND);
-	glEnable (GL_TEXTURE_2D);
 	glBindTexture (GL_TEXTURE_2D, cursor.texture);
 
 	streamingBuffer->begin (GL_TRIANGLE_STRIP);
@@ -1165,45 +1193,40 @@
 	streamingBuffer->render (sTransform);
 
 	glBindTexture (GL_TEXTURE_2D, 0);
-	glDisable (GL_TEXTURE_2D);
 	glDisable (GL_BLEND);
     }
 }
 
 /* Create (if necessary) a texture to store the cursor,
- * fetch the cursor with XFixes. Store it.  */
+ * fetch the cursor with XFixes. Store it. */
 void
 EZoomScreen::updateCursor (CursorTexture * cursor)
 {
     unsigned char *pixels;
-    int           i;
-    Display       *dpy = screen->dpy ();
+    int		  i;
+    Display	  *dpy = screen->dpy ();
 
     if (!cursor->isSet)
     {
 	cursor->isSet = true;
 	cursor->screen = screen;
 
-	glEnable (GL_TEXTURE_2D);
 	glGenTextures (1, &cursor->texture);
 	glBindTexture (GL_TEXTURE_2D, cursor->texture);
 
 	glTexParameteri (GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
 	glTexParameteri (GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
 	glTexParameteri (GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
-	                 gScreen->textureFilter ());
-	glTexParameteri (GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, 
-	                 gScreen->textureFilter ());
-    }
-    else {
-	glEnable (GL_TEXTURE_2D);
+			 gScreen->textureFilter ());
+	glTexParameteri (GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
+			 gScreen->textureFilter ());
     }
 
     XFixesCursorImage *ci = XFixesGetCursorImage (dpy);
 
     if (ci)
     {
-	cursor->width = ci->width;
+	cursor->width  = ci->width;
 	cursor->height = ci->height;
 	cursor->hotX = ci->xhot;
 	cursor->hotY = ci->yhot;
@@ -1215,7 +1238,7 @@
 	    return;
 	}
 
-	for (i = 0; i < ci->width * ci->height; i++)
+	for (i = 0; i < ci->width * ci->height; ++i)
 	{
 	    unsigned long pix = ci->pixels[i];
 	    pixels[i * 4] = pix & 0xff;
@@ -1240,7 +1263,7 @@
 	if (!pixels)
 	    return;
 
-	for (i = 0; i < cursor->width * cursor->height; i++)
+	for (i = 0; i < cursor->width * cursor->height; ++i)
 	{
 	    unsigned long pix = 0x00ffffff;
 	    pixels[i * 4] = pix & 0xff;
@@ -1256,8 +1279,7 @@
     glTexImage2D (GL_TEXTURE_2D, 0, GL_RGBA, cursor->width,
 		  cursor->height, 0, GL_BGRA, GL_UNSIGNED_BYTE, pixels);
     glBindTexture (GL_TEXTURE_2D, 0);
-    glDisable (GL_TEXTURE_2D);
-	
+
     free (pixels);
 }
 
@@ -1275,9 +1297,7 @@
     }
 
     if (cursor.isSet)
-    {
 	freeCursor (&cursor);
-    }
 
     if (cursorHidden)
     {
@@ -1300,7 +1320,6 @@
     /* Force cursor hiding and mouse panning if this output is locked
      * and cursor hiding is not enabled and we are syncing the mouse
      */
-
     if (!optionGetScaleMouse () &&
         (optionGetZoomMode () == EzoomOptions::ZoomModeSyncMouse &&
 	 optionGetHideOriginalMouse () &&
@@ -1314,6 +1333,7 @@
 				 XFixesDisplayCursorNotifyMask);
 	updateCursor (&cursor);
     }
+
     if (canHideCursor && !cursorHidden &&
 	(optionGetHideOriginalMouse () ||
 	 zooms.at (out).locked))
@@ -1336,39 +1356,40 @@
  */
 bool
 EZoomScreen::setZoomAreaAction (CompAction         *action,
-			       CompAction::State  state,
-			       CompOption::Vector options)
+				CompAction::State  state,
+				CompOption::Vector options)
 {
-    int        x1, y1, x2, y2, out;
-    bool       scale, restrain;
     CompOutput *o;
 
-    x1 = CompOption::getIntOptionNamed (options, "x1", -1);
-    y1 = CompOption::getIntOptionNamed (options, "y1", -1);
-    x2 = CompOption::getIntOptionNamed (options, "x2", -1);
-    y2 = CompOption::getIntOptionNamed (options, "y2", -1);
-    scale = CompOption::getBoolOptionNamed (options, "scale", false);
-    restrain = CompOption::getBoolOptionNamed (options, "restrain", false);
+    int x1 = CompOption::getIntOptionNamed (options, "x1", -1);
+    int y1 = CompOption::getIntOptionNamed (options, "y1", -1);
+    int x2 = CompOption::getIntOptionNamed (options, "x2", -1);
+    int y2 = CompOption::getIntOptionNamed (options, "y2", -1);
+
+    bool scale    = CompOption::getBoolOptionNamed (options, "scale", false);
+    bool restrain = CompOption::getBoolOptionNamed (options, "restrain", false);
 
     if (x1 < 0 || y1 < 0)
-        return false;
+	return false;
 
     if (x2 < 0)
-        x2 = x1 + 1;
+	x2 = x1 + 1;
 
     if (y2 < 0)
-        y2 = y1 + 1;
-
-    out = screen->outputDeviceForPoint (x1, y1);
-#define WIDTH (x2 - x1)
-#define HEIGHT (y2 - y1)
-    setZoomArea (x1, y1, WIDTH, HEIGHT, false);
+	y2 = y1 + 1;
+
+    int out = screen->outputDeviceForPoint (x1, y1);
+
+    int width  = x2 - x1;
+    int height = y2 - y1;
+
+    setZoomArea (x1, y1, width, height, false);
     o = &screen->outputDevs (). at(out);
-    if (scale && WIDTH && HEIGHT)
-        setScaleBigger (out, (float) WIDTH / o->width (),
-		        (float) HEIGHT / o->height ());
-#undef WIDTH
-#undef HEIGHT
+
+    if (scale && width && height)
+	setScaleBigger (out, width / static_cast <float> (o->width ()),
+			height / static_cast <float> (o->height ()));
+
 	if (restrain)
 	    restrainCursor (out);
 
@@ -1389,36 +1410,40 @@
  */
 bool
 EZoomScreen::ensureVisibilityAction (CompAction         *action,
-			            CompAction::State  state,
-			            CompOption::Vector options)
+				     CompAction::State  state,
+				     CompOption::Vector options)
 {
-    int        x1, y1, x2, y2, margin, out;
-    bool       scale, restrain;
     CompOutput *o;
 
-    x1 = CompOption::getIntOptionNamed (options, "x1", -1);
-    y1 = CompOption::getIntOptionNamed (options, "y1", -1);
-    x2 = CompOption::getIntOptionNamed (options, "x2", -1);
-    y2 = CompOption::getIntOptionNamed (options, "y2", -1);
-    margin = CompOption::getBoolOptionNamed (options, "margin", 0);
-    scale = CompOption::getBoolOptionNamed (options, "scale", false);
-    restrain = CompOption::getBoolOptionNamed (options, "restrain", false);
+    int x1 = CompOption::getIntOptionNamed (options, "x1", -1);
+    int y1 = CompOption::getIntOptionNamed (options, "y1", -1);
+    int x2 = CompOption::getIntOptionNamed (options, "x2", -1);
+    int y2 = CompOption::getIntOptionNamed (options, "y2", -1);
+    int margin    = CompOption::getBoolOptionNamed (options, "margin", 0);
+    bool scale    = CompOption::getBoolOptionNamed (options, "scale", false);
+    bool restrain = CompOption::getBoolOptionNamed (options, "restrain", false);
+
     if (x1 < 0 || y1 < 0)
-        return false;
+	return false;
+
     if (x2 < 0)
-        y2 = y1 + 1;
-    out = screen->outputDeviceForPoint (x1, y1);
+	y2 = y1 + 1;
+
+    int out = screen->outputDeviceForPoint (x1, y1);
     ensureVisibility (x1, y1, margin);
+
     if (x2 >= 0 && y2 >= 0)
-        ensureVisibility (x2, y2, margin);
+	ensureVisibility (x2, y2, margin);
+
     o = &screen->outputDevs (). at(out);
-#define WIDTH (x2 - x1)
-#define HEIGHT (y2 - y1)
-    if (scale && WIDTH && HEIGHT)
-        setScaleBigger (out, (float) WIDTH / o->width (),
-		        (float) HEIGHT / o->height ());
-#undef WIDTH
-#undef HEIGHT
+
+    int width  = x2 - x1;
+    int height = y2 - y1;
+
+    if (scale && width && height)
+	setScaleBigger (out, width / static_cast <float> (o->width ()),
+			height / static_cast <float> (o->height ()));
+
     if (restrain)
 	restrainCursor (out);
 
@@ -1431,15 +1456,16 @@
 
 bool
 EZoomScreen::zoomBoxActivate (CompAction         *action,
-			     CompAction::State  state,
-			     CompOption::Vector options)
+			      CompAction::State  state,
+			      CompOption::Vector options)
 {
     grabIndex = screen->pushGrab (None, "ezoom");
     clickPos.setX (pointerX);
     clickPos.setY (pointerY);
     box.setGeometry (pointerX, pointerY, 0, 0);
+
     if (state & CompAction::StateInitButton)
-        action->setState (action->state () | CompAction::StateTermButton);
+	action->setState (action->state () | CompAction::StateTermButton);
 
     toggleFunctions (true);
 
@@ -1448,50 +1474,44 @@
 
 bool
 EZoomScreen::zoomBoxDeactivate (CompAction         *action,
-			       CompAction::State  state,
-			       CompOption::Vector options)
+				CompAction::State  state,
+				CompOption::Vector options)
 {
     if (grabIndex)
     {
-        int        out;
-	int	   x, y, width, height;
-        CompOutput *o;
-
-        screen->removeGrab (grabIndex, NULL);
-        grabIndex = 0;
-
-        if (pointerX < clickPos.x ())
-        {
+	CompOutput *o;
+
+	screen->removeGrab (grabIndex, NULL);
+	grabIndex = 0;
+
+	if (pointerX < clickPos.x ())
+	{
 	    box.setX (pointerX);
 	    box.setWidth (clickPos.x () - pointerX);
-        }
-        else
-        {
+	}
+	else
 	    box.setWidth (pointerX - clickPos.x ());
-        }
 
-        if (pointerY < clickPos.y ())
-        {
+	if (pointerY < clickPos.y ())
+	{
 	    box.setY (pointerY);
 	    box.setHeight (clickPos.y () - pointerY);
-        }
-        else
-        {
+	}
+	else
 	    box.setHeight (pointerY - clickPos.y ());
-        }
-
-        x = MIN (box.x1 (), box.x2 ());
-        y = MIN (box.y1 (), box.y2 ());
-        width = MAX (box.x1 (), box.x2 ()) - x;
-        height = MAX (box.y1 (), box.y2 ()) - y;
-
-        CompWindow::Geometry outGeometry (x, y, width, height, 0);
-
-        out = screen->outputDeviceForGeometry (outGeometry);
-        o = &screen->outputDevs (). at (out);
-        setScaleBigger (out, (float) width/o->width (), (float)
-		        height/o->height ());
-        setZoomArea (x,y,width,height,false);
+
+	int x = MIN (box.x1 (), box.x2 ());
+	int y = MIN (box.y1 (), box.y2 ());
+	int width  = MAX (box.x1 (), box.x2 ()) - x;
+	int height = MAX (box.y1 (), box.y2 ()) - y;
+
+	CompWindow::Geometry outGeometry (x, y, width, height, 0);
+
+	int out = screen->outputDeviceForGeometry (outGeometry);
+	o = &screen->outputDevs (). at (out);
+	setScaleBigger (out, width / static_cast <float> (o->width ()),
+			height / static_cast <float> (o->height ()));
+	setZoomArea (x, y, width, height, false);
     }
 
     toggleFunctions (true);
@@ -1503,14 +1523,14 @@
  */
 bool
 EZoomScreen::zoomIn (CompAction         *action,
-		    CompAction::State  state,
-		    CompOption::Vector options)
+		     CompAction::State  state,
+		     CompOption::Vector options)
 {
     int out = screen->outputDeviceForPoint (pointerX, pointerY);
 
     if (optionGetZoomMode () == EzoomOptions::ZoomModeSyncMouse &&
 	!isInMovement (out))
-        setCenter (pointerX, pointerY, true);
+	setCenter (pointerX, pointerY, true);
 
     setScale (out,
 	      zooms.at (out).newZoom /
@@ -1525,8 +1545,8 @@
  */
 bool
 EZoomScreen::lockZoomAction (CompAction         *action,
-			    CompAction::State  state,
-			    CompOption::Vector options)
+			     CompAction::State  state,
+			     CompOption::Vector options)
 {
     int out = screen->outputDeviceForPoint (pointerX, pointerY);
     zooms.at (out).locked = !zooms.at (out).locked;
@@ -1543,12 +1563,12 @@
  */
 bool
 EZoomScreen::zoomSpecific (CompAction         *action,
-			  CompAction::State  state,
-			  CompOption::Vector options,
-			  SpecificZoomTarget target)
+			   CompAction::State  state,
+			   CompOption::Vector options,
+			   SpecificZoomTarget target)
 {
-    int          out = screen->outputDeviceForPoint (pointerX, pointerY);
-    float        zoom_level;
+    int		 out = screen->outputDeviceForPoint (pointerX, pointerY);
+    float	 zoom_level;
     CompWindow   *w;
 
     switch (target)
@@ -1556,34 +1576,36 @@
 	case ZoomTargetFirst:
 	    zoom_level = optionGetZoomSpec1 ();
 	    break;
+
 	case ZoomTargetSecond:
 	    zoom_level = optionGetZoomSpec2 ();
 	    break;
+
 	case ZoomTargetThird:
 	    zoom_level = optionGetZoomSpec3 ();
 	    break;
+
 	default:
 	    return false;
     }
 
     if (zoom_level == 1.0f && zooms.at (out).newZoom == 1.0f)
-        return false;
+	return false;
+
     if (screen->otherGrabExist (NULL))
-        return false;
+	return false;
 
     setScale (out, zoom_level);
 
     w = screen->findWindow (screen->activeWindow ());
-    if (optionGetSpecTargetFocus ()
-        && w)
-    {
-        areaToWindow (w);
-    }
+
+    if (optionGetSpecTargetFocus () && w)
+	areaToWindow (w);
     else
     {
-        int x = CompOption::getIntOptionNamed (options, "x", 0);
-        int y = CompOption::getIntOptionNamed (options, "y", 0);
-        setCenter (x, y, false);
+	int x = CompOption::getIntOptionNamed (options, "x", 0);
+	int y = CompOption::getIntOptionNamed (options, "y", 0);
+	setCenter (x, y, false);
     }
 
     toggleFunctions (true);
@@ -1598,24 +1620,24 @@
  */
 bool
 EZoomScreen::zoomToWindow (CompAction         *action,
-			  CompAction::State  state,
-			  CompOption::Vector options)
+			   CompAction::State  state,
+			   CompOption::Vector options)
 {
-    int        width, height, out;
-    Window     xid;
     CompWindow *w;
     CompOutput *o;
 
-    xid = CompOption::getIntOptionNamed (options, "window", 0);
+    Window xid = CompOption::getIntOptionNamed (options, "window", 0);
     w = screen->findWindow (xid);
+
     if (!w)
 	return true;
-    width = w->width () + w->border ().left + w->border ().right;
-    height = w->height () + w->border ().top + w->border ().bottom;
-    out = screen->outputDeviceForGeometry (w->geometry ());
+
+    int width = w->width () + w->border ().left + w->border ().right;
+    int height = w->height () + w->border ().top + w->border ().bottom;
+    int out = screen->outputDeviceForGeometry (w->geometry ());
     o = &screen->outputDevs ().at (out);
-    setScaleBigger (out, (float) width/o->width (),
-		    (float) height/o->height ());
+    setScaleBigger (out, width / static_cast <float> (o->width ()),
+		    height / static_cast <float> (o->height ()));
     areaToWindow (w);
     toggleFunctions (true);
 
@@ -1624,10 +1646,10 @@
 
 bool
 EZoomScreen::zoomPan (CompAction         *action,
-		     CompAction::State  state,
-		     CompOption::Vector options,
-		     float		horizAmount,
-		     float		vertAmount)
+		      CompAction::State  state,
+		      CompOption::Vector options,
+		      float		 horizAmount,
+		      float		 vertAmount)
 {
     panZoom (horizAmount, vertAmount);
 
@@ -1641,18 +1663,16 @@
 			     CompAction::State  state,
 			     CompOption::Vector options)
 {
-    int        out;
-
-
-    out = screen->outputDeviceForPoint (pointerX, pointerY);
-    screen->warpPointer ((int) (screen->outputDevs ().at (out).width ()/2 +
-			screen->outputDevs ().at (out).x1 () - pointerX)
+    int out = screen->outputDeviceForPoint (pointerX, pointerY);
+
+    screen->warpPointer ((int) (screen->outputDevs ().at (out).width () / 2 +
+				screen->outputDevs ().at (out).x1 () - pointerX)
 			 + ((float) screen->outputDevs ().at (out).width () *
-				-zooms.at (out).xtrans),
-			 (int) (screen->outputDevs ().at (out).height ()/2 +
+			    -zooms.at (out).xtrans),
+			 (int) (screen->outputDevs ().at (out).height () / 2 +
 				screen->outputDevs ().at (out).y1 () - pointerY)
 			 + ((float) screen->outputDevs ().at (out).height () *
-				zooms.at (out).ytrans));
+			    zooms.at (out).ytrans));
     return true;
 }
 
@@ -1663,10 +1683,9 @@
  */
 bool
 EZoomScreen::zoomFitWindowToZoom (CompAction         *action,
-			         CompAction::State  state,
-			         CompOption::Vector options)
+				  CompAction::State  state,
+				  CompOption::Vector options)
 {
-    int            out;
     unsigned int   mask = CWWidth | CWHeight;
     XWindowChanges xwc;
     CompWindow     *w;
@@ -1676,7 +1695,7 @@
     if (!w)
 	return true;
 
-    out = screen->outputDeviceForGeometry (w->geometry ());
+    int out = screen->outputDeviceForGeometry (w->geometry ());
     xwc.x = w->serverX ();
     xwc.y = w->serverY ();
     xwc.width = (int) (screen->outputDevs ().at (out).width () *
@@ -1691,7 +1710,6 @@
 			       &xwc.width,
 			       &xwc.height);
 
-
     if (xwc.width == w->serverWidth ())
 	mask &= ~CWWidth;
 
@@ -1710,8 +1728,8 @@
 
 bool
 EZoomScreen::initiate (CompAction         *action,
-		      CompAction::State  state,
-		      CompOption::Vector options)
+		       CompAction::State  state,
+		       CompOption::Vector options)
 {
     zoomIn (action, state, options);
 
@@ -1728,8 +1746,8 @@
 
 bool
 EZoomScreen::zoomOut (CompAction         *action,
-		     CompAction::State  state,
-		     CompOption::Vector options)
+		      CompAction::State  state,
+		      CompOption::Vector options)
 {
     int out = screen->outputDeviceForPoint (pointerX, pointerY);
 
@@ -1747,14 +1765,12 @@
 		       CompAction::State  state,
 		       CompOption::Vector options)
 {
-    int out;
-
-    out = screen->outputDeviceForPoint (pointerX, pointerY);
+    int out = screen->outputDeviceForPoint (pointerX, pointerY);
 
     if (grabbed)
     {
-        zooms.at (out).newZoom = 1.0f;
-        cScreen->damageScreen ();
+	zooms.at (out).newZoom = 1.0f;
+	cScreen->damageScreen ();
     }
 
     toggleFunctions (true);
@@ -1777,7 +1793,6 @@
 void
 EZoomScreen::focusTrack (XEvent *event)
 {
-    int           out;
     static Window lastMapped = 0;
 
     CompWindow    *w;
@@ -1790,31 +1805,34 @@
     else if (event->type != FocusIn)
 	return;
 
-    if ((event->xfocus.mode != NotifyNormal)
-	&& (lastMapped != event->xfocus.window))
+    if ((event->xfocus.mode != NotifyNormal) &&
+	(lastMapped != event->xfocus.window))
 	return;
 
     lastMapped = 0;
     w = screen->findWindow (event->xfocus.window);
-    if (w == NULL || w->id () == screen->activeWindow ())
-	return;
 
-    if (time(NULL) - lastChange < optionGetFollowFocusDelay () ||
+    if (w == NULL						||
+	w->id () == screen->activeWindow ()			||
+	time(NULL) - lastChange < optionGetFollowFocusDelay ()	||
 	!optionGetFollowFocus ())
 	return;
 
-    out = screen->outputDeviceForGeometry (w->geometry ());
+    int out = screen->outputDeviceForGeometry (w->geometry ());
+
     if (!isActive (out) &&
 	!optionGetAlwaysFocusFitWindow ())
 	return;
+
     if (optionGetFocusFitWindow ())
     {
 	int width = w->width () + w->border ().left + w->border ().right;
 	int height = w->height () + w->border ().top + w->border ().bottom;
-	float scale = MAX ((float) width/screen->outputDevs ().at(out).width (),
-			   (float) height/screen->outputDevs ().at (out).height ());
+	float scale = MAX (width / static_cast <float> (screen->outputDevs ().at(out).width ()),
+			   height / static_cast <float> (screen->outputDevs ().at (out).height ()));
+
 	if (scale > optionGetAutoscaleMin ())
-		setScale (out, scale);
+	    setScale (out, scale);
     }
 
     areaToWindow (w);
@@ -1822,35 +1840,32 @@
     toggleFunctions (true);
 }
 
-
 /* Event handler. Pass focus-related events on and handle XFixes events. */
 void
 EZoomScreen::handleEvent (XEvent *event)
 {
-    switch (event->type) {
+    switch (event->type)
+    {
 	case MotionNotify:
 	    if (grabIndex)
 	    {
-	        if (pointerX < clickPos.x ())
-	        {
+		if (pointerX < clickPos.x ())
+		{
 		    box.setX (pointerX);
 		    box.setWidth (clickPos.x () - pointerX);
-	        }
-	        else
-	        {
+		}
+		else
 		    box.setWidth (pointerX - clickPos.x ());
-	        }
 
-	        if (pointerY < clickPos.y ())
-	        {
+		if (pointerY < clickPos.y ())
+		{
 		    box.setY (pointerY);
 		    box.setHeight (clickPos.y () - pointerY);
-	        }
-	        else
-	        {
+		}
+		else
 		    box.setHeight (pointerY - clickPos.y ());
-	        }
-	        cScreen->damageScreen ();
+
+		cScreen->damageScreen ();
 	    }
 	    break;
 
@@ -1858,13 +1873,14 @@
 	case MapNotify:
 	    focusTrack (event);
 	    break;
+
 	default:
 	    if (event->type == fixesEventBase + XFixesCursorNotify)
 	    {
 		//XFixesCursorNotifyEvent *cev = (XFixesCursorNotifyEvent *)
-		    //event;
-		    if (cursor.isSet)
-			updateCursor (&cursor);
+		//event;
+		if (cursor.isSet)
+		    updateCursor (&cursor);
 	    }
 	    break;
     }
@@ -1900,6 +1916,7 @@
 
     int major, minor;
     unsigned int n;
+
     fixesSupported =
 	XFixesQueryExtension(screen->dpy (),
 			     &fixesEventBase,
@@ -1914,11 +1931,12 @@
 
     n = screen->outputDevs ().size ();
 
-    for (unsigned int i = 0; i < n; i++)
+    for (unsigned int i = 0; i < n; ++i)
     {
 	/* zs->grabbed is a mask ... Thus this limit */
 	if (i > sizeof (long int) * 8)
 	    break;
+
 	ZoomArea za (i);
 	zooms.push_back (za);
     }
@@ -1962,7 +1980,6 @@
 					&EZoomScreen::zoomFitWindowToZoom, this,
 					_1, _2, _3));
 
-
     optionSetLockZoomKeyInitiate (boost::bind (&EZoomScreen::lockZoomAction,
 						this, _1, _2, _3));
     optionSetZoomBoxButtonInitiate (boost::bind (&EZoomScreen::zoomBoxActivate,
@@ -1976,7 +1993,6 @@
     optionSetEnsureVisibilityInitiate (boost::bind (
 					&EZoomScreen::ensureVisibilityAction, this,
 					_1, _2, _3));
-
 }
 
 EZoomScreen::~EZoomScreen ()

=== modified file 'plugins/ezoom/src/ezoom.h'
--- plugins/ezoom/src/ezoom.h	2012-10-15 12:51:26 +0000
+++ plugins/ezoom/src/ezoom.h	2013-05-14 21:54:27 +0000
@@ -75,7 +75,8 @@
 
     public:
 
-	typedef enum {
+	typedef enum
+	{
 	    NORTHEAST,
 	    NORTHWEST,
 	    SOUTHEAST,
@@ -83,7 +84,8 @@
 	    CENTER
 	} ZoomGravity;
 
-	typedef enum {
+	typedef enum
+	{
 	    NORTH,
 	    SOUTH,
 	    EAST,
@@ -135,6 +137,7 @@
 		GLfloat           xtrans;
 		GLfloat           ytrans;
 		bool              locked;
+
 	    public:
 
 		ZoomArea (int out);
@@ -146,21 +149,21 @@
 
     public:
 
-	std::vector <ZoomArea>   zooms; // list of zooms (different zooms for
+	std::vector <ZoomArea>	zooms;	// list of zooms (different zooms for
 					// each output
-	CompPoint		 mouse; // we get this from mousepoll
-	unsigned long int	 grabbed;
-	CompScreen::GrabHandle   grabIndex; // for zoomBox
-	time_t			 lastChange;
-	CursorTexture		 cursor; // the texture for the faux-cursor
-					 // we paint to do fake input
-					 // handling
-	bool			 cursorInfoSelected;
-	bool			 cursorHidden;
-	CompRect		 box;
-	CompPoint	         clickPos;
+	CompPoint		mouse;	// we get this from mousepoll
+	unsigned long int	grabbed;
+	CompScreen::GrabHandle	grabIndex; // for zoomBox
+	time_t			lastChange;
+	CursorTexture		cursor;	// the texture for the faux-cursor
+					// we paint to do fake input
+					// handling
+	bool			cursorInfoSelected;
+	bool			cursorHidden;
+	CompRect		box;
+	CompPoint		clickPos;
 
-	MousePoller		 pollHandle; // mouse poller object
+	MousePoller		pollHandle; // mouse poller object
 
      private: