kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #38437
Re: [PATCH] GTK+3 zooming
Hi Wayne, Seth,
Here is the patchset with:
* A default ctor param and get/setters
* Trace mask in the header. I somehow haven't noticed this file before, sorry!
Cheers,
John
On Sun, Nov 25, 2018 at 9:35 PM Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
>
> Hey John,
>
> On 11/23/2018 8:26 AM, John Beard wrote:
> > Hi,
> >
> > This is a patch to refactor the zooming of WX_VIEW_CONTROL. This is
> > related to lp:1786515 [1], but it's not a fix, it's just a refactor to
> > help debug the problem, and also tidy the code.
> >
> > The refactor is done to avoid big chunks of conditionally compiled
> > code kicking around in functions, and to make the platform behaviours
> > clearer. The platform-specific bit is still ifdeff'ed, as making that
> > run-time is a little bit more verbose than is required, I feel.
> >
> > There's also a patch (#2) for the libcommon tests to init WX.This
> > means you can see WXTRACE messages. The teardown of WX prints some
> > glib stuff on GTK3 [2], but it should be harmless, and doing the
> > teardown keeps valgrind happier.
> >
> > Cheers,
> >
> > John
> >
> > [1]: https://bugs.launchpad.net/kicad/+bug/1786515
> > [2]: http://trac.wxwidgets.org/ticket/18274
> >
>
> I tested your patches on windows and it looks good to me. Along with
> Seth's suggestions, I would like to ask you to please define the
> wxLogTrace flag string "SCROLL_ZOOM" in include/trace_helpers.h and
> common/trace_helpers.cpp so they will be added to the "Trace Environment
> Variables" section in the developer docs[1].
>
> Cheers,
>
> Wayne
>
> [1]: http://docs.kicad-pcb.org/doxygen/group__trace__env__vars.html
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help : https://help.launchpad.net/ListHelp
From 317bb05511f079024de78058c72e8e3b974c70bd Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Fri, 23 Nov 2018 10:26:19 +0000
Subject: [PATCH 2/4] QA: Initialise WX for the libcommon tests
If this is not done, things like logging and trace don't work,
as they need WX to be set up first.
---
qa/common/test_module.cpp | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/qa/common/test_module.cpp b/qa/common/test_module.cpp
index 38f18fd03..36183b2db 100644
--- a/qa/common/test_module.cpp
+++ b/qa/common/test_module.cpp
@@ -22,11 +22,27 @@
*/
/**
- * Main file for the geometry tests to be compiled
+ * Main file for the libcommon tests to be compiled
*/
+#include <boost/test/unit_test.hpp>
-#define BOOST_TEST_MAIN
-#define BOOST_TEST_MODULE "Common library module tests"
+#include <wx/init.h>
-#include <boost/test/unit_test.hpp>
+bool init_unit_test()
+{
+ boost::unit_test::framework::master_test_suite().p_name.value = "Common library module tests";
+ return wxInitialize();
+}
+
+
+int main( int argc, char* argv[] )
+{
+ int ret = boost::unit_test::unit_test_main( &init_unit_test, argc, argv );
+
+ // This causes some glib warnings on GTK3 (http://trac.wxwidgets.org/ticket/18274)
+ // but without it, Valgrind notices a lot of leaks from WX
+ wxUninitialize();
+
+ return ret;
+}
\ No newline at end of file
--
2.19.2
From 01cc5b1cbed9c57c388ada79c7781f42df1f9654 Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Thu, 22 Nov 2018 16:24:17 +0000
Subject: [PATCH 1/4] 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 | 1 +
common/CMakeLists.txt | 1 +
common/trace_helpers.cpp | 6 +-
common/view/wx_view_controls.cpp | 65 +++++---------
common/view/zoom_controller.cpp | 112 ++++++++++++++++++++++++
include/trace_helpers.h | 11 ++-
include/view/wx_view_controls.h | 14 +--
include/view/zoom_controller.h | 110 +++++++++++++++++++++++
qa/common/CMakeLists.txt | 2 +
qa/common/view/test_zoom_controller.cpp | 90 +++++++++++++++++++
10 files changed, 360 insertions(+), 52 deletions(-)
create mode 100644 common/view/zoom_controller.cpp
create mode 100644 include/view/zoom_controller.h
create mode 100644 qa/common/view/test_zoom_controller.cpp
diff --git a/Documentation/development/testing.md b/Documentation/development/testing.md
index 81fd22701..7fddb2050 100644
--- a/Documentation/development/testing.md
+++ b/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`
diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt
index 924599753..7c1db83df 100644
--- a/common/CMakeLists.txt
+++ b/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
diff --git a/common/trace_helpers.cpp b/common/trace_helpers.cpp
index 8586ef356..b9d084013 100644
--- a/common/trace_helpers.cpp
+++ b/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 )
diff --git a/common/view/wx_view_controls.cpp b/common/view/wx_view_controls.cpp
index 81e0e5752..6ef378469 100644
--- a/common/view/wx_view_controls.cpp
+++ b/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
- wxLongLong timeStamp = wxGetLocalTimeMillis();
- 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
+ int rotation = aEvent.GetWheelRotation();
+ double zoomScale = m_zoomController->GetScaleForRotation( rotation );
if( IsCursorWarpingEnabled() )
{
diff --git a/common/view/zoom_controller.cpp b/common/view/zoom_controller.cpp
new file mode 100644
index 000000000..9a426cfad
--- /dev/null
+++ b/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@xxxxxxx>
+ * @author Maciej Suminski <maciej.suminski@xxxxxxx>
+ *
+ * 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;
\ No newline at end of file
diff --git a/include/trace_helpers.h b/include/trace_helpers.h
index 115d87d80..050ab1daf 100644
--- a/include/trace_helpers.h
+++ b/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;
+
///@}
/**
diff --git a/include/view/wx_view_controls.h b/include/view/wx_view_controls.h
index b71d4b1ea..d3b7d0733 100644
--- a/include/view/wx_view_controls.h
+++ b/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
diff --git a/include/view/zoom_controller.h b/include/view/zoom_controller.h
new file mode 100644
index 000000000..a367c10bf
--- /dev/null
+++ b/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
diff --git a/qa/common/CMakeLists.txt b/qa/common/CMakeLists.txt
index f6a7298cc..20979fb46 100644
--- a/qa/common/CMakeLists.txt
+++ b/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(
diff --git a/qa/common/view/test_zoom_controller.cpp b/qa/common/view/test_zoom_controller.cpp
new file mode 100644
index 000000000..a1f7b209e
--- /dev/null
+++ b/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()
--
2.19.2
From 0aa78a5e958b7b32218cfb588c4a365bbe2b53ec Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Thu, 22 Nov 2018 17:01:59 +0000
Subject: [PATCH 3/4] Zoom: Use std::chrono for the timestamping
The reduces a little bit of WX dependency, and makes
the timing code a bit more type-safe.
Also adds a more testable interface for the accelerated
zoom controller.
---
common/view/wx_view_controls.cpp | 2 +-
common/view/zoom_controller.cpp | 50 ++++++++++++-----
include/view/zoom_controller.h | 59 +++++++++++++++++---
qa/common/view/test_zoom_controller.cpp | 74 ++++++++++++++++++++++---
4 files changed, 155 insertions(+), 30 deletions(-)
diff --git a/common/view/wx_view_controls.cpp b/common/view/wx_view_controls.cpp
index 6ef378469..7ed899165 100644
--- a/common/view/wx_view_controls.cpp
+++ b/common/view/wx_view_controls.cpp
@@ -45,7 +45,7 @@ static std::unique_ptr<ZOOM_CONTROLLER> GetZoomControllerForPlatform()
// 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 );
+ return std::make_unique<ACCELERATING_ZOOM_CONTROLLER>();
#endif
}
diff --git a/common/view/zoom_controller.cpp b/common/view/zoom_controller.cpp
index 9a426cfad..2d01fa695 100644
--- a/common/view/zoom_controller.cpp
+++ b/common/view/zoom_controller.cpp
@@ -28,19 +28,45 @@
#include <view/zoom_controller.h>
+#include <make_unique.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 )
+/**
+ * A very simple timestamper that uses the #KIGFX::ACCELERATING_ZOOM_CONTROLLER::CLOCK
+ * to provide a timestamp. Since that's a steady_clock, it's monotonic.
+ */
+class SIMPLE_TIMESTAMPER : public ACCELERATING_ZOOM_CONTROLLER::TIMESTAMP_PROVIDER
{
+public:
+ ACCELERATING_ZOOM_CONTROLLER::TIME_PT GetTimestamp() override
+ {
+ return ACCELERATING_ZOOM_CONTROLLER::CLOCK::now();
+ }
+};
+
+
+ACCELERATING_ZOOM_CONTROLLER::ACCELERATING_ZOOM_CONTROLLER(
+ const TIMEOUT& aAccTimeout, TIMESTAMP_PROVIDER* aTimestampProv )
+ : m_accTimeout( aAccTimeout )
+{
+ if( aTimestampProv )
+ {
+ m_timestampProv = aTimestampProv;
+ }
+ else
+ {
+ m_ownTimestampProv = std::make_unique<SIMPLE_TIMESTAMPER>();
+ m_timestampProv = m_ownTimestampProv.get();
+ }
+
+ m_lastTimestamp = m_timestampProv->GetTimestamp();
}
@@ -49,18 +75,18 @@ 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;
+ const auto timestamp = m_timestampProv->GetTimestamp();
+ auto timeDiff = std::chrono::duration_cast<TIMEOUT>( timestamp - m_lastTimestamp );
- m_lastTimeStamp = timeStamp;
+ m_lastTimestamp = timestamp;
wxLogTrace( traceZoomScroll,
- wxString::Format( "Rot %d, time diff: %ldms", aRotation, timeDiff ) );
+ wxString::Format( "Rot %d, time diff: %ldms", aRotation, timeDiff.count() ) );
double zoomScale;
// Set scaling speed depending on scroll wheel event interval
- if( timeDiff < m_accTimeout && timeDiff > 0 )
+ if( timeDiff < m_accTimeout )
{
zoomScale = 2.05 - timeDiff / m_accTimeout;
@@ -81,12 +107,6 @@ double ACCELERATING_ZOOM_CONTROLLER::GetScaleForRotation( int aRotation )
}
-double ACCELERATING_ZOOM_CONTROLLER::getTimeStamp() const
-{
- return wxGetLocalTimeMillis().ToDouble();
-}
-
-
CONSTANT_ZOOM_CONTROLLER::CONSTANT_ZOOM_CONTROLLER( double aScale ) : m_scale( aScale )
{
}
@@ -108,5 +128,7 @@ double CONSTANT_ZOOM_CONTROLLER::GetScaleForRotation( int aRotation )
}
// need these until C++17
+constexpr ACCELERATING_ZOOM_CONTROLLER::TIMEOUT ACCELERATING_ZOOM_CONTROLLER::DEFAULT_TIMEOUT;
+
constexpr double CONSTANT_ZOOM_CONTROLLER::MAC_SCALE;
constexpr double CONSTANT_ZOOM_CONTROLLER::GTK3_SCALE;
\ No newline at end of file
diff --git a/include/view/zoom_controller.h b/include/view/zoom_controller.h
index a367c10bf..fa2eed6ab 100644
--- a/include/view/zoom_controller.h
+++ b/include/view/zoom_controller.h
@@ -30,6 +30,8 @@
#ifndef __ZOOM_CONTROLLER_H
#define __ZOOM_CONTROLLER_H
+#include <chrono>
+#include <memory>
namespace KIGFX
{
@@ -58,24 +60,65 @@ public:
class ACCELERATING_ZOOM_CONTROLLER : public ZOOM_CONTROLLER
{
public:
+ /// The type of the acceleration timeout
+ using TIMEOUT = std::chrono::milliseconds;
+
+ /// The clock used for the timestamp (guaranteed to be monotonic)
+ using CLOCK = std::chrono::steady_clock;
+
+ /// The type of the time stamps
+ using TIME_PT = std::chrono::time_point<CLOCK>;
+
+ /// The default timeout, after which a another scroll will not be accelerated
+ static constexpr TIMEOUT DEFAULT_TIMEOUT = std::chrono::milliseconds( 500 );
+
+ /*
+ * A class interface that provides timestamps for events
+ */
+ class TIMESTAMP_PROVIDER
+ {
+ public:
+ virtual ~TIMESTAMP_PROVIDER() = default;
+
+ /*
+ * @return the timestamp at the current time
+ */
+ virtual TIME_PT GetTimestamp() = 0;
+ };
+
/**
- * @param aAccTimeout the timeout - if a scoll happens within this timeframe,
+ * @param aAccTimeout the timeout - if a scroll happens within this timeframe,
* the zoom will be faster
+ * @param aTimestampProv a provider for timestamps. If null, a default will
+ * be provided, which is the main steady_clock (this is probably what you
+ * want for real usage)
*/
- ACCELERATING_ZOOM_CONTROLLER( unsigned aAccTimeout );
+ ACCELERATING_ZOOM_CONTROLLER( const TIMEOUT& aAccTimeout = DEFAULT_TIMEOUT,
+ TIMESTAMP_PROVIDER* aTimestampProv = nullptr );
double GetScaleForRotation( int aRotation ) override;
+ TIMEOUT GetTimeout() const
+ {
+ return m_accTimeout;
+ }
+
+ void SetTimeout( const TIMEOUT& aNewTimeout )
+ {
+ m_accTimeout = aNewTimeout;
+ }
+
private:
- /**
- * @return the timestamp of an event at the current time. Monotonic.
- */
- double getTimeStamp() const;
+ /// The timestamp provider to use (might be provided externally)
+ TIMESTAMP_PROVIDER* m_timestampProv;
+
+ /// Any provider owned by this class (the default one, if used)
+ std::unique_ptr<TIMESTAMP_PROVIDER> m_ownTimestampProv;
/// The timestamp of the last event
- double m_lastTimeStamp;
+ TIME_PT m_lastTimestamp;
/// The timeout value
- unsigned m_accTimeout;
+ TIMEOUT m_accTimeout;
};
diff --git a/qa/common/view/test_zoom_controller.cpp b/qa/common/view/test_zoom_controller.cpp
index a1f7b209e..ff89f0e43 100644
--- a/qa/common/view/test_zoom_controller.cpp
+++ b/qa/common/view/test_zoom_controller.cpp
@@ -31,9 +31,6 @@
using namespace KIGFX;
-/**
- * Declares a struct as the Boost test fixture.
- */
BOOST_AUTO_TEST_SUITE( ZoomController )
@@ -81,10 +78,73 @@ BOOST_AUTO_TEST_CASE( ConstController )
}
}
-/*
- * 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.
+/**
+ * Timestamper that returns predefined values from a vector
+ */
+class PREDEF_TIMESTAMPER : public ACCELERATING_ZOOM_CONTROLLER::TIMESTAMP_PROVIDER
+{
+public:
+ using STAMP_LIST = std::vector<int>;
+
+ PREDEF_TIMESTAMPER( const STAMP_LIST& aStamps )
+ : m_stamps( aStamps ), m_iter( m_stamps.begin() )
+ {
+ }
+
+ /**
+ * @return the next time point in the predefined sequence
+ */
+ ACCELERATING_ZOOM_CONTROLLER::TIME_PT GetTimestamp() override
+ {
+ // Don't ask for more samples than given
+ BOOST_REQUIRE( m_iter != m_stamps.end() );
+
+ return ACCELERATING_ZOOM_CONTROLLER::TIME_PT( std::chrono::milliseconds( *m_iter++ ) );
+ }
+
+ const STAMP_LIST m_stamps;
+ STAMP_LIST::const_iterator m_iter;
+};
+
+
+struct ACCEL_ZOOM_CASE
+{
+ int timeout;
+ std::vector<int> stamps; // NB includes the initial stamp!
+ std::vector<int> scrolls;
+ std::vector<double> zooms;
+};
+
+static const std::vector<ACCEL_ZOOM_CASE> accel_cases = {
+ // Scrolls widely spaced, just go up and down by a constant factor
+ { 500, { 0, 1000, 2000, 3000 }, { 120, 120, -120 }, { 1.2, 1.2, 1 / 1.2 } },
+ // Close scrolls - acceleration on the latter
+ { 500, { 0, 1000, 1100 }, { 120, 120 }, { 1.2, 2.05 } },
+};
+
+
+/**
+ * Check basic setting and getting of values
*/
+BOOST_AUTO_TEST_CASE( AccelController )
+{
+ const double tol_percent = 10.0;
+
+ for( const auto& c : accel_cases )
+ {
+ PREDEF_TIMESTAMPER timestamper( c.stamps );
+
+ ACCELERATING_ZOOM_CONTROLLER zoom_ctrl(
+ std::chrono::milliseconds( c.timeout ), ×tamper );
+
+ for( unsigned i = 0; i < c.scrolls.size(); i++ )
+ {
+ const auto zoom_scale = zoom_ctrl.GetScaleForRotation( c.scrolls[i] );
+
+ BOOST_CHECK_CLOSE( zoom_scale, c.zooms[i], tol_percent );
+ }
+ }
+}
+
BOOST_AUTO_TEST_SUITE_END()
--
2.19.2
Follow ups
References
