kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #38405
[PATCH] GTK+3 zooming
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
From 86529229172eab9af7cacbc0508d784cd058bccd 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/view/wx_view_controls.cpp | 65 ++++++--------
common/view/zoom_controller.cpp | 110 ++++++++++++++++++++++++
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 +++++++++++++++++++
8 files changed, 347 insertions(+), 46 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/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..172151ff6
--- /dev/null
+++ b/common/view/zoom_controller.cpp
@@ -0,0 +1,110 @@
+/*
+ * 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 <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( "SCROLL_ZOOM",
+ 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 )
+ {
+ 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( "SCROLL_ZOOM", 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( "SCROLL_ZOOM", 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( "SCROLL_ZOOM", 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/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.1
From f58a1bc642163cad9619e1270e68f54a993683c7 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 | 43 +++++++++-----
include/view/zoom_controller.h | 46 ++++++++++++---
qa/common/view/test_zoom_controller.cpp | 74 ++++++++++++++++++++++---
4 files changed, 136 insertions(+), 29 deletions(-)
diff --git a/common/view/wx_view_controls.cpp b/common/view/wx_view_controls.cpp
index 6ef378469..764c70069 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>( std::chrono::milliseconds( 500 ) );
#endif
}
diff --git a/common/view/zoom_controller.cpp b/common/view/zoom_controller.cpp
index 172151ff6..b7eaafc3e 100644
--- a/common/view/zoom_controller.cpp
+++ b/common/view/zoom_controller.cpp
@@ -28,17 +28,40 @@
#include <view/zoom_controller.h>
+#include <make_unique.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 )
+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(
+ 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();
}
@@ -47,10 +70,10 @@ 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( "SCROLL_ZOOM",
wxString::Format( "Rot %d, time diff: %ldms", aRotation, timeDiff.count() ) );
@@ -58,7 +81,7 @@ double ACCELERATING_ZOOM_CONTROLLER::GetScaleForRotation( int aRotation )
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;
@@ -79,12 +102,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 )
{
}
diff --git a/include/view/zoom_controller.h b/include/view/zoom_controller.h
index a367c10bf..59e32d73b 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,52 @@ 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>;
+
+ /*
+ * 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(
+ TIMEOUT aAccTimeout, TIMESTAMP_PROVIDER* aTimestampProv = nullptr );
double GetScaleForRotation( int aRotation ) override;
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..a3ee1bf63 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
+ { 560, { 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.1
From 3a00b0baefd8cf2deab3dc1ef8d2ff003ac9aa63 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.1
Follow ups
