From 1eb0f70de5738f549452a8d659ef7476a9fae27e Mon Sep 17 00:00:00 2001 From: John Beard Date: Thu, 22 Nov 2018 17:01:59 +0000 Subject: [PATCH] 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 | 63 +++++++++++++++++---- qa/common/view/test_zoom_controller.cpp | 74 ++++++++++++++++++++++--- 4 files changed, 157 insertions(+), 32 deletions(-) diff --git a/common/view/wx_view_controls.cpp b/common/view/wx_view_controls.cpp index 6ef3784698..7ed899165e 100644 --- a/common/view/wx_view_controls.cpp +++ b/common/view/wx_view_controls.cpp @@ -45,7 +45,7 @@ static std::unique_ptr GetZoomControllerForPlatform() // based on the rotation amount rather than the time difference. return std::make_unique( CONSTANT_ZOOM_CONTROLLER::MAC_SCALE ); #else - return std::make_unique( 500 ); + return std::make_unique(); #endif } diff --git a/common/view/zoom_controller.cpp b/common/view/zoom_controller.cpp index 9a426cfad0..2d01fa6954 100644 --- a/common/view/zoom_controller.cpp +++ b/common/view/zoom_controller.cpp @@ -28,19 +28,45 @@ #include +#include #include #include -#include // For timestamping events #include 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(); + 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( 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 a367c10bff..fa2eed6ab7 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 +#include namespace KIGFX { @@ -58,24 +60,65 @@ public: class ACCELERATING_ZOOM_CONTROLLER : public ZOOM_CONTROLLER { public: - /** - * @param aAccTimeout the timeout - if a scoll happens within this timeframe, - * the zoom will be faster + /// 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; + + /// 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 */ - ACCELERATING_ZOOM_CONTROLLER( unsigned aAccTimeout ); + 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 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( 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 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 a1f7b209e1..ff89f0e43a 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; + + 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 stamps; // NB includes the initial stamp! + std::vector scrolls; + std::vector zooms; +}; + +static const std::vector 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()