Break zoom control into a self-contained controller

This is done to avoid a big chunk of conditionally-compiled code
in the middle of the event function.

Also separates the zoom logic from the WX_VIEW_CONTROLS object
and isolates it in a separate class behind a clearer interface.

Add some simple tests for sane steps on GTK+3-sized scroll
steps.

Documentation/development/testing.md

@@ -243,6 +243,7 @@ Some available masks:
     * `GAL_CACHED_CONTAINER`
     * `PNS`
     * `CN`
+    * `SCROLL_ZOOM` - for the scroll-wheel zooming logic in GAL
 * Plugin-specific (including "standard" KiCad formats):
     * `3D_CACHE`
     * `3D_SG`

common/CMakeLists.txt

@@ -46,6 +46,7 @@ set( GAL_SRCS
     view/view_controls.cpp
     view/view_overlay.cpp
     view/wx_view_controls.cpp
+    view/zoom_controller.cpp
 
     # OpenGL GAL
     gal/opengl/opengl_gal.cpp

common/trace_helpers.cpp

@@ -27,11 +27,6 @@
  * @brief wxLogTrace helper implementation.
  */
 
-#include <wx/defs.h>
-#include <wx/string.h>
-#include <wx/event.h>
-#include <wx/arrstr.h>
-
 #include <trace_helpers.h>
 
 
@@ -48,6 +43,7 @@ const wxChar* const traceAutoSave = wxT( "KICAD_AUTOSAVE" );
 const wxChar* const tracePathsAndFiles = wxT( "KICAD_PATHS_AND_FILES" );
 const wxChar* const traceLocale = wxT( "KICAD_LOCALE" );
 const wxChar* const traceScreen = wxT( "KICAD_SCREEN" );
+const wxChar* const traceZoomScroll = wxT( "KICAD_ZOOM_SCROLL" );
 
 
 wxString dump( const wxArrayString& aArray )

common/view/wx_view_controls.cpp

@@ -28,6 +28,7 @@
 
 #include <view/view.h>
 #include <view/wx_view_controls.h>
+#include <view/zoom_controller.h>
 #include <gal/graphics_abstraction_layer.h>
 #include <tool/tool_dispatcher.h>
 
@@ -35,6 +36,20 @@ using namespace KIGFX;
 
 const wxEventType WX_VIEW_CONTROLS::EVT_REFRESH_MOUSE = wxNewEventType();
 
+
+static std::unique_ptr<ZOOM_CONTROLLER> GetZoomControllerForPlatform()
+{
+#ifdef __WXMAC__
+    // On Apple pointer devices, wheel events occur frequently and with
+    // smaller rotation values.  For those devices, let's handle zoom
+    // based on the rotation amount rather than the time difference.
+    return std::make_unique<CONSTANT_ZOOM_CONTROLLER>( CONSTANT_ZOOM_CONTROLLER::MAC_SCALE );
+#else
+    return std::make_unique<ACCELERATING_ZOOM_CONTROLLER>( 500 );
+#endif
+}
+
+
 WX_VIEW_CONTROLS::WX_VIEW_CONTROLS( VIEW* aView, wxScrolledCanvas* aParentPanel ) :
     VIEW_CONTROLS( aView ), m_state( IDLE ), m_parentPanel( aParentPanel ),
     m_scrollScale( 1.0, 1.0 ), m_cursorPos( 0, 0 ), m_updateCursor( true )
@@ -72,12 +87,19 @@ WX_VIEW_CONTROLS::WX_VIEW_CONTROLS( VIEW* aView, wxScrolledCanvas* aParentPanel
     m_parentPanel->Connect( wxEVT_SCROLLWIN_PAGEDOWN,
                             wxScrollWinEventHandler( WX_VIEW_CONTROLS::onScroll ), NULL, this );
 
+    m_zoomController = GetZoomControllerForPlatform();
+
     m_panTimer.SetOwner( this );
     this->Connect( wxEVT_TIMER,
                    wxTimerEventHandler( WX_VIEW_CONTROLS::onTimer ), NULL, this );
 }
 
 
+WX_VIEW_CONTROLS::~WX_VIEW_CONTROLS()
+{
+}
+
+
 void WX_VIEW_CONTROLS::onMotion( wxMouseEvent& aEvent )
 {
     bool isAutoPanning = false;
@@ -110,7 +132,6 @@ void WX_VIEW_CONTROLS::onMotion( wxMouseEvent& aEvent )
 void WX_VIEW_CONTROLS::onWheel( wxMouseEvent& aEvent )
 {
     const double wheelPanSpeed = 0.001;
-    const double zoomLevelScale = 1.2;      // The minimal step value when changing the current zoom level
 
     // mousewheelpan disabled:
     //      wheel + ctrl    -> horizontal scrolling;
@@ -153,46 +174,8 @@ void WX_VIEW_CONTROLS::onWheel( wxMouseEvent& aEvent )
     }
     else
     {
-        // Zooming
+        int    rotation = aEvent.GetWheelRotation();
-        wxLongLong  timeStamp   = wxGetLocalTimeMillis();
+        double zoomScale = m_zoomController->GetScaleForRotation( rotation );
-        double      timeDiff    = timeStamp.ToDouble() - m_timeStamp.ToDouble();
-        int         rotation    = aEvent.GetWheelRotation();
-        double      zoomScale = 1.0;
-
-#ifdef __WXMAC__
-        // On Apple pointer devices, wheel events occur frequently and with
-        // smaller rotation values.  For those devices, let's handle zoom
-        // based on the rotation amount rather than the time difference.
-
-        // Unused
-        ( void )timeDiff;
-
-        rotation = ( rotation > 0 ) ? std::min( rotation , 100 )
-                                    : std::max( rotation , -100 );
-
-        double dscale = rotation * 0.01;
-        zoomScale = ( rotation > 0 ) ? (1 + dscale)  : 1/(1 - dscale);
-
-#else
-
-        m_timeStamp = timeStamp;
-
-        // Set scaling speed depending on scroll wheel event interval
-        if( timeDiff < 500 && timeDiff > 0 )
-        {
-            zoomScale = 2.05 - timeDiff / 500;
-
-            // be sure zoomScale value is significant
-            zoomScale = std::max( zoomScale, zoomLevelScale );
-
-            if( rotation < 0 )
-                zoomScale = 1.0 / zoomScale;
-        }
-        else
-        {
-            zoomScale = ( rotation > 0 ) ? zoomLevelScale : 1/zoomLevelScale;
-        }
-#endif
 
         if( IsCursorWarpingEnabled() )
         {

common/view/zoom_controller.cpp

@@ -0,0 +1,112 @@
+/*
+ * This program source code file is part of KiCad, a free EDA CAD application.
+ *
+ * Copyright (C) 2012 Torsten Hueter, torstenhtr <at> gmx.de
+ * Copyright (C) 2013-2015 CERN
+ * Copyright (C) 2012-2016 KiCad Developers, see AUTHORS.txt for contributors.
+ *
+ * @author Tomasz Wlostowski <tomasz.wlostowski@cern.ch>
+ * @author Maciej Suminski <maciej.suminski@cern.ch>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, you may find one here:
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+ * or you may search the http://www.gnu.org website for the version 2 license,
+ * or you may write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
+ */
+
+#include <view/zoom_controller.h>
+
+#include <trace_helpers.h>
+
+#include <wx/log.h>
+#include <wx/time.h> // For timestamping events
+
+#include <algorithm>
+
+using namespace KIGFX;
+
+
+ACCELERATING_ZOOM_CONTROLLER::ACCELERATING_ZOOM_CONTROLLER( unsigned aAccTimeout )
+        : m_lastTimeStamp( getTimeStamp() ), m_accTimeout( aAccTimeout )
+{
+}
+
+
+double ACCELERATING_ZOOM_CONTROLLER::GetScaleForRotation( int aRotation )
+{
+    // The minimal step value when changing the current zoom level
+    const double zoomLevelScale = 1.2;
+
+    const auto timeStamp = getTimeStamp();
+    auto       timeDiff = timeStamp - m_lastTimeStamp;
+
+    m_lastTimeStamp = timeStamp;
+
+    wxLogTrace( traceZoomScroll,
+            wxString::Format( "Rot %d, time diff: %ldms", aRotation, timeDiff ) );
+
+    double zoomScale;
+
+    // Set scaling speed depending on scroll wheel event interval
+    if( timeDiff < m_accTimeout && timeDiff > 0 )
+    {
+        zoomScale = 2.05 - timeDiff / m_accTimeout;
+
+        // be sure zoomScale value is significant
+        zoomScale = std::max( zoomScale, zoomLevelScale );
+
+        if( aRotation < 0 )
+            zoomScale = 1.0 / zoomScale;
+    }
+    else
+    {
+        zoomScale = ( aRotation > 0 ) ? zoomLevelScale : 1 / zoomLevelScale;
+    }
+
+    wxLogTrace( traceZoomScroll, wxString::Format( "  Zoom factor: %f", zoomScale ) );
+
+    return zoomScale;
+}
+
+
+double ACCELERATING_ZOOM_CONTROLLER::getTimeStamp() const
+{
+    return wxGetLocalTimeMillis().ToDouble();
+}
+
+
+CONSTANT_ZOOM_CONTROLLER::CONSTANT_ZOOM_CONTROLLER( double aScale ) : m_scale( aScale )
+{
+}
+
+
+double CONSTANT_ZOOM_CONTROLLER::GetScaleForRotation( int aRotation )
+{
+    wxLogTrace( traceZoomScroll, wxString::Format( "Rot %d", aRotation ) );
+
+    aRotation = ( aRotation > 0 ) ? std::min( aRotation, 100 ) : std::max( aRotation, -100 );
+
+    double dscale = aRotation * m_scale;
+
+    double zoom_scale = ( aRotation > 0 ) ? ( 1 + dscale ) : 1 / ( 1 - dscale );
+
+    wxLogTrace( traceZoomScroll, wxString::Format( "  Zoom factor: %f", zoom_scale ) );
+
+    return zoom_scale;
+}
+
+// need these until C++17
+constexpr double CONSTANT_ZOOM_CONTROLLER::MAC_SCALE;
+constexpr double CONSTANT_ZOOM_CONTROLLER::GTK3_SCALE;

include/trace_helpers.h

@@ -30,6 +30,10 @@
 #ifndef _TRACE_HELPERS_H_
 #define _TRACE_HELPERS_H_
 
+#include <wx/arrstr.h>
+#include <wx/event.h>
+#include <wx/string.h>
+
 /**
  * @defgroup trace_env_vars Trace Environment Variables
  *
@@ -58,7 +62,6 @@ extern const wxChar* const traceFindReplace;
  */
 extern const wxChar* const kicadTraceCoords;
 
-
 /**
  * Flag to enable wxKeyEvent debug tracing.
  */
@@ -109,6 +112,12 @@ extern const wxChar* const traceLocale;
  */
 extern const wxChar* const traceScreen;
 
+/**
+ * Flag to enable debug output of zoom-scrolling calculations in
+ * #KIGFX::ZOOM_CONTROLER and derivatives.
+ */
+extern const wxChar* const traceZoomScroll;
+
 ///@}
 
 /**

include/view/wx_view_controls.h

@@ -35,10 +35,15 @@
 
 #include <view/view_controls.h>
 
+#include <memory>
+
 class EDA_DRAW_PANEL_GAL;
 
 namespace KIGFX
 {
+
+class ZOOM_CONTROLLER;
+
 /**
  * Class WX_VIEW_CONTROLS
  * is a specific implementation of class VIEW_CONTROLS for wxWidgets library.
@@ -47,8 +52,7 @@ class WX_VIEW_CONTROLS : public VIEW_CONTROLS, public wxEvtHandler
 {
 public:
     WX_VIEW_CONTROLS( VIEW* aView, wxScrolledCanvas* aParentPanel );
-    virtual ~WX_VIEW_CONTROLS()
+    virtual ~WX_VIEW_CONTROLS();
-    {}
 
     /// Handler functions
     void onWheel( wxMouseEvent& aEvent );
@@ -146,9 +150,6 @@ private:
     /// Current direction of panning (only autopanning mode)
     VECTOR2D    m_panDirection;
 
-    /// Used for determining time intervals between scroll & zoom events
-    wxLongLong  m_timeStamp;
-
     /// Timer repsonsible for handling autopanning
     wxTimer     m_panTimer;
 
@@ -163,6 +164,9 @@ private:
 
     /// Flag deciding whether the cursor position should be calculated using the mouse position
     bool        m_updateCursor;
+
+    /// a ZOOM_CONTROLLER that determines zoom steps. This is platform-specific.
+    std::unique_ptr<ZOOM_CONTROLLER> m_zoomController;
 };
 } // namespace KIGFX
 

include/view/zoom_controller.h

@@ -0,0 +1,110 @@
+/*
+ * This program source code file is part of KiCad, a free EDA CAD application.
+ *
+ * Copyright (C) 2013-2018 KiCad Developers, see AUTHORS.txt for contributors.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, you may find one here:
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+ * or you may search the http://www.gnu.org website for the version 2 license,
+ * or you may write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
+ *
+ */
+
+/**
+ * @file zoom_control.h
+ * @brief ZOOM_CONTROLLER class definition.
+ */
+
+#ifndef __ZOOM_CONTROLLER_H
+#define __ZOOM_CONTROLLER_H
+
+
+namespace KIGFX
+{
+
+/**
+ * Class that handles the response of the zoom scale to external inputs
+ */
+class ZOOM_CONTROLLER
+{
+public:
+    virtual ~ZOOM_CONTROLLER() = default;
+
+    /**
+     * Gets the scale factor produced by a given mousewheel rotation
+     * @param  aRotation rotation of the mouse wheel (this comes from
+     *                    wxMouseEvent::GetWheelRotation())
+     * @return            the scale factor to scroll by
+     */
+    virtual double GetScaleForRotation( int aRotation ) = 0;
+};
+
+
+/**
+ * Class that zooms faster if scroll events happen very close together.
+ */
+class ACCELERATING_ZOOM_CONTROLLER : public ZOOM_CONTROLLER
+{
+public:
+    /**
+     * @param aAccTimeout the timeout - if a scoll happens within this timeframe,
+     *                    the zoom will be faster
+     */
+    ACCELERATING_ZOOM_CONTROLLER( unsigned aAccTimeout );
+
+    double GetScaleForRotation( int aRotation ) override;
+
+private:
+    /**
+     * @return the timestamp of an event at the current time. Monotonic.
+     */
+    double getTimeStamp() const;
+
+    /// The timestamp of the last event
+    double   m_lastTimeStamp;
+    /// The timeout value
+    unsigned m_accTimeout;
+};
+
+
+/**
+ * A ZOOM_CONTROLLER that zooms by a fixed factor based only on the magnitude
+ * of the scroll wheel rotation.
+ */
+class CONSTANT_ZOOM_CONTROLLER : public ZOOM_CONTROLLER
+{
+public:
+    /**
+     * @param aScale a scaling parameter that adjusts the magnitude of the
+     * scroll. This factor might be dependent on the platform for comfort.
+     */
+    CONSTANT_ZOOM_CONTROLLER( double aScale );
+
+    double GetScaleForRotation( int aRotation ) override;
+
+    /// A suitable (magic) scale factor for GTK3 systems
+    static constexpr double GTK3_SCALE = 0.001;
+
+    /// A suitable (magic) scale factor for Mac systems
+    static constexpr double MAC_SCALE = 0.01;
+
+private:
+    /// The scale factor set by the constructor.
+    double m_scale;
+};
+
+} // namespace KIGFX
+
+#endif

qa/common/CMakeLists.txt

@@ -41,6 +41,8 @@ add_executable( qa_common
     test_utf8.cpp
 
     geometry/test_fillet.cpp
+
+    view/test_zoom_controller.cpp
 )
 
 include_directories(

qa/common/view/test_zoom_controller.cpp

@@ -0,0 +1,90 @@
+/*
+ * This program source code file is part of KiCad, a free EDA CAD application.
+ *
+ * Copyright (C) 2018 KiCad Developers, see CHANGELOG.TXT for contributors.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, you may find one here:
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+ * or you may search the http://www.gnu.org website for the version 2 license,
+ * or you may write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
+ */
+
+#include <boost/test/test_case_template.hpp>
+#include <boost/test/unit_test.hpp>
+
+#include <view/zoom_controller.h>
+
+
+// All these tests are of a class in KIGFX
+using namespace KIGFX;
+
+
+/**
+ * Declares a struct as the Boost test fixture.
+ */
+BOOST_AUTO_TEST_SUITE( ZoomController )
+
+
+struct CONST_ZOOM_CASE
+{
+    double scale_factor;
+    int    scroll_amount;
+    double exp_zoom_in;
+    double exp_zoom_out;
+};
+
+
+/*
+ * Some "sane" examples for steps, scale factors and results.
+ * These should be actual examples that could be encountered
+ *
+ * TODO: Add more cases for, eg, Mac
+ */
+static const std::vector<CONST_ZOOM_CASE> const_zoom_cases = {
+    // A single scroll step on a GTK3 Linux system
+    // 120 is the standard wheel delta, so it's what you might expect
+    // from a single scroll wheel detent.
+    { CONSTANT_ZOOM_CONTROLLER::GTK3_SCALE, 120, 1.1, 1 / 1.1 },
+};
+
+/**
+ * Check basic setting and getting of values
+ */
+BOOST_AUTO_TEST_CASE( ConstController )
+{
+    // How close we need to be (not very, this is a subjective thing anyway)
+    const double tol_percent = 10;
+
+    double scale_for_step;
+
+    for( const auto& c : const_zoom_cases )
+    {
+        CONSTANT_ZOOM_CONTROLLER zoom_ctrl( c.scale_factor );
+
+        scale_for_step = zoom_ctrl.GetScaleForRotation( c.scroll_amount );
+        BOOST_CHECK_CLOSE( scale_for_step, c.exp_zoom_in, tol_percent );
+
+        scale_for_step = zoom_ctrl.GetScaleForRotation( -c.scroll_amount );
+        BOOST_CHECK_CLOSE( scale_for_step, c.exp_zoom_out, tol_percent );
+    }
+}
+
+/*
+ * Testing the accelerated version without making a very slow test is a little
+ * tricky and would need a mock timestamping interface, which complicates the
+ * real interface a bit and does not really seem worth the effort.
+ */
+
+BOOST_AUTO_TEST_SUITE_END()