Compiz

Code review comment for lp:~mc-return/compiz/compiz.merge-ezoom-cleanup

compiz.merge-ezoom-cleanup
Merge into 0.9.10

Revision history for this message

MC Return (mc-return) wrote on 2013-05-09:

> 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.

> 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:

> 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.

« Back to merge proposal