Merge lp:~sjakthol/compiz/fix-1066187 into lp:compiz/0.9.9

[Sami Jaktholm]

Problem:
Specific Zoom Factor settings were ignored by the Enhanced Zoom Desktop
(LP: #1066187) because the values were only read once during initial load of
the plugin probably before the user defined configuration was read (and thus
it always used the default values).

Fix:
Instead of sending the (original) value of the initiated zoom level, send a
zoom level identifier. The identifier is used to determine which zoom level
the user wants and a fresh setting for that zoom level is used.

.

Tested the changes locally and they seem to compile and work fine. Please let me know if there's any problems with the coding style or implementation and I'll try to fix them.

Test case:
1. Open CCSM
2. Choose 'Enhanced Zoom Desktop' plugin (and make sure it's in use)
3. Go to 'Specific Zoom' tab
4. Add key bindings for Specific Zoom Level 1, 2 and 3
5. Set same values to Specific Zoom factor 2 and 3 (e.g. 0.7)
6. Press the key binding for Specific Zoom Level 2
7. Press the key binding for Specific Zoom Level 3
8. Change the zoom factors and test what happens by pressing the corresponding key bindings

You might want to keep the factor 1 at 1.00 so that you can reset the zoom once done.

Current behavior:
At (6) the screen gets zoomed in (like it should) but at (7) screen is zoomed even more (incorrect behavior, the values were the same). At (8) you'll see how the zoom for a certain level (factor 1 = 1.00, factor 2 = 0.5, factor 3 = 0.2) is always the same no matter what your input is.

Behavior with the fix:
At (6) the screen gets zoomed in (like it should) and at (7) nothing happens (correct behavior as the zoom factors were the same). At (8) the zoom clearly depends on the values you specified.

Possible problems:
Not sure if this is possible but if something outside ezoom plugin is directly calling EZoomScreen::zoomSpecific they will be broken as the input variable type of the target changed.

[MC Return (mc-return)]

sampo555, great :)
Thanks a lot for your fix. I have yet to test it until I can approve it, but looks quite ok to me.

Regarding coding style: Compiz uses C++11 coding style, mixing spaces with tabs.

The first indent are 4 spaces, then a tab 8 spaces wide, then a mix of a tab and 4 spaces.
If you intend to help more with Compiz coding and want to use QTCreator, I am sure we can share the QTC settings file with you to make it easier to follow this style.

Otherwise I can take over your branch and fix the indentation there, before we merge this.

Thanks a lot for your contribution - you are very welcome to eliminate more of those nasty small bugs ;).
(I'll try to test and approve your fix ASAP.)

[MC Return (mc-return)]

I have tested this fix now and can confirm that it works like intended. :)

Regarding possible problems: It seems no other plug-in in current trunk requires ezoom (<requirement>), also a search for EZoomScreen::zoomSpecific through the code did not find any results except the ones in plugins/ezoom/src/ezoom.cpp.

The only stopper before merge, that I can spot, is the wrong indentation.

[Daniel van Vugt (vanvugt)]

Verified, the fix works.

Please just change the opening braces on lines 16 and 65 to match the Compiz convention. So:
    foo {
should be:
    foo
    {

review: Needs Fixing

[lp:~sjakthol/compiz/fix-1066187]

3416. By Sami Jaktholm on 2012-10-15

Fix brackets.

[Sami Jaktholm (sjakthol)]

Fixed.

[MC Return (mc-return)]

LGTM.

review: Approve

[Daniel van Vugt (vanvugt)]

review: Approve

=== modified file 'plugins/ezoom/src/ezoom.cpp'
--- plugins/ezoom/src/ezoom.cpp	2012-09-04 15:33:44 +0000
+++ plugins/ezoom/src/ezoom.cpp	2012-10-15 12:54:23 +0000
@@ -1544,17 +1544,33 @@
 EZoomScreen::zoomSpecific (CompAction         *action,
 			  CompAction::State  state,
 			  CompOption::Vector options,
-			  float		     target)
+			  SpecificZoomTarget target)
 {
     int          out = screen->outputDeviceForPoint (pointerX, pointerY);
+    float        zoom_level;
     CompWindow   *w;
 
-    if (target == 1.0f && zooms.at (out).newZoom == 1.0f)
+    switch (target)
+    {
+	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;
     if (screen->otherGrabExist (NULL))
         return false;
 
-    setScale (out, target);
+    setScale (out, zoom_level);
 
     w = screen->findWindow (screen->activeWindow ());
     if (optionGetSpecTargetFocus ()
@@ -1915,13 +1931,13 @@
 
     optionSetZoomSpecific1KeyInitiate (boost::bind (&EZoomScreen::zoomSpecific,
 						    this, _1, _2, _3,
-						    optionGetZoomSpec1 ()));
+						    ZoomTargetFirst));
     optionSetZoomSpecific2KeyInitiate (boost::bind (&EZoomScreen::zoomSpecific,
 						    this, _1, _2, _3,
-						    optionGetZoomSpec2 ()));
+						    ZoomTargetSecond));
     optionSetZoomSpecific3KeyInitiate (boost::bind (&EZoomScreen::zoomSpecific,
 						    this, _1, _2, _3,
-						    optionGetZoomSpec3 ()));
+						    ZoomTargetThird));
 
     optionSetPanLeftKeyInitiate (boost::bind (&EZoomScreen::zoomPan, this, _1,
 					      _2, _3, -1, 0));

=== modified file 'plugins/ezoom/src/ezoom.h'
--- plugins/ezoom/src/ezoom.h	2012-09-04 15:33:44 +0000
+++ plugins/ezoom/src/ezoom.h	2012-10-15 12:54:23 +0000
@@ -49,6 +49,13 @@
 
 #include <cmath>
 
+enum SpecificZoomTarget
+{
+    ZoomTargetFirst = 0,
+    ZoomTargetSecond,
+    ZoomTargetThird
+};
+
 class EZoomScreen :
     public PluginClassHandler <EZoomScreen, CompScreen>,
     public EzoomOptions,
@@ -314,7 +321,7 @@
 	zoomSpecific (CompAction         *action,
 		      CompAction::State  state,
 		      CompOption::Vector options,
-		      float		     target);
+		      SpecificZoomTarget target);
 
 	bool
 	zoomToWindow (CompAction         *action,