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 <[email protected]> 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 : [email protected] > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> + * @author Maciej Suminski <[email protected]> + * + * 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 <[email protected]> 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
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
