Store VIEW_CONTROLS settings when tools are switched

Fixes: lp:1663783
* https://bugs.launchpad.net/kicad/+bug/1663783

Fixes: lp:1667580
* https://bugs.launchpad.net/kicad/+bug/1667580

common/tool/tool_manager.cpp

@@ -70,6 +70,7 @@ struct TOOL_MANAGER::TOOL_STATE
         wakeupEvent = aState.wakeupEvent;
         waitEvents = aState.waitEvents;
         transitions = aState.transitions;
+        vcSettings = aState.vcSettings;
         // do not copy stateStack
     }
 
@@ -110,6 +111,9 @@ struct TOOL_MANAGER::TOOL_STATE
     /// upon the event reception
     std::vector<TRANSITION> transitions;
 
+    /// VIEW_CONTROLS settings to preserve settings when the tools are switched
+    KIGFX::VIEW_CONTROLS::SETTINGS vcSettings;
+
     void operator=( const TOOL_STATE& aState )
     {
         theTool = aState.theTool;
@@ -122,6 +126,7 @@ struct TOOL_MANAGER::TOOL_STATE
         wakeupEvent = aState.wakeupEvent;
         waitEvents = aState.waitEvents;
         transitions = aState.transitions;
+        vcSettings = aState.vcSettings;
         // do not copy stateStack
     }
 
@@ -142,8 +147,9 @@ struct TOOL_MANAGER::TOOL_STATE
      */
     void Push()
     {
-        stateStack.push( new TOOL_STATE( *this ) );
+        auto state = std::make_unique<TOOL_STATE>( *this );
-
+        //state->vcSettings = theTool->getViewControls()->GetSettings();
+        stateStack.push( std::move( state ) );
         clear();
     }
 
@@ -159,8 +165,7 @@ struct TOOL_MANAGER::TOOL_STATE
 
         if( !stateStack.empty() )
         {
-            *this = *stateStack.top();
+            *this = *stateStack.top().get();
-            delete stateStack.top();
             stateStack.pop();
             return true;
         }
@@ -173,7 +178,7 @@ struct TOOL_MANAGER::TOOL_STATE
 
 private:
     ///> Stack preserving previous states of a TOOL.
-    std::stack<TOOL_STATE*> stateStack;
+    std::stack<std::unique_ptr<TOOL_STATE>> stateStack;
 
     ///> Restores the initial state.
     void clear()
@@ -184,6 +189,7 @@ private:
         cofunc = NULL;
         contextMenu = NULL;
         contextMenuTrigger = CMENU_OFF;
+        vcSettings.Reset();
         transitions.clear();
     }
 };
@@ -365,13 +371,13 @@ bool TOOL_MANAGER::runTool( TOOL_BASE* aTool )
         return false;
     }
 
+    TOOL_ID id = aTool->GetId();
+
     // If the tool is already active, bring it to the top of the active tools stack
     if( isActive( aTool ) )
     {
-        m_activeTools.erase( std::find( m_activeTools.begin(), m_activeTools.end(),
+        m_activeTools.erase( std::find( m_activeTools.begin(), m_activeTools.end(), id ) );
-                                        aTool->GetId() ) );
+        m_activeTools.push_front( id );
-        m_activeTools.push_front( aTool->GetId() );
-
         return false;
     }
 
@@ -379,7 +385,7 @@ bool TOOL_MANAGER::runTool( TOOL_BASE* aTool )
     aTool->SetTransitions();
 
     // Add the tool on the front of the processing queue (it gets events first)
-    m_activeTools.push_front( aTool->GetId() );
+    m_activeTools.push_front( id );
 
     return true;
 }
@@ -409,7 +415,8 @@ TOOL_BASE* TOOL_MANAGER::FindTool( const std::string& aName ) const
 
 void TOOL_MANAGER::DeactivateTool()
 {
-    TOOL_EVENT evt( TC_COMMAND, TA_ACTIVATE, "" );      // deactivate the active tool
+    // Deactivate the active tool, but do not run anything new
+    TOOL_EVENT evt( TC_COMMAND, TA_CANCEL_TOOL );
     ProcessEvent( evt );
 }
 
@@ -508,11 +515,9 @@ optional<TOOL_EVENT> TOOL_MANAGER::ScheduleWait( TOOL_BASE* aTool,
 void TOOL_MANAGER::dispatchInternal( const TOOL_EVENT& aEvent )
 {
     // iterate over all registered tools
-    for( auto it = m_activeTools.begin(); it != m_activeTools.end(); /* iteration is done inside */)
+    for( auto it = m_activeTools.begin(); it != m_activeTools.end(); ++it )
     {
-        auto curIt = it;
         TOOL_STATE* st = m_toolIdIndex[*it];
-        ++it;       // it might be overwritten, if the tool is removed the m_activeTools deque
 
         // the tool state handler is waiting for events (i.e. called Wait() method)
         if( st->pendingWait )
@@ -528,10 +533,7 @@ void TOOL_MANAGER::dispatchInternal( const TOOL_EVENT& aEvent )
                 st->waitEvents.clear();
 
                 if( st->cofunc && !st->cofunc->Resume() )
-                {
+                    it = finishTool( st );
-                    if( finishTool( st, false ) ) // The couroutine has finished
-                        it = m_activeTools.erase( curIt );
-                }
 
                 // If the tool did not request to propagate
                 // the event to other tools, we should stop it now
@@ -557,7 +559,13 @@ void TOOL_MANAGER::dispatchInternal( const TOOL_EVENT& aEvent )
 
                     // if there is already a context, then store it
                     if( st->cofunc )
+                    {
+                        // store the VIEW_CONTROLS settings if we launch a subtool
+                        if( GetCurrentToolState() == st )
+                            st->vcSettings = m_viewControls->GetSettings();
+
                         st->Push();
+                    }
 
                     st->cofunc = new COROUTINE<int, const TOOL_EVENT&>( std::move( func_copy ) );
 
@@ -670,30 +678,29 @@ void TOOL_MANAGER::dispatchContextMenu( const TOOL_EVENT& aEvent )
 }
 
 
-bool TOOL_MANAGER::finishTool( TOOL_STATE* aState, bool aDeactivate )
+TOOL_MANAGER::ID_LIST::iterator TOOL_MANAGER::finishTool( TOOL_STATE* aState )
 {
-    bool shouldDeactivate = false;
+    auto it = std::find( m_activeTools.begin(), m_activeTools.end(), aState->theTool->GetId() );
 
-    if( !aState->Pop() )        // if there are no other contexts saved on the stack
+    // Store the current VIEW_CONTROLS settings
+    if( TOOL_STATE* state = GetCurrentToolState() )
+        state->vcSettings = m_viewControls->GetSettings();
+
+    if( !aState->Pop() )
     {
-        // find the tool and deactivate it
+        // Deactivate the tool if there are no other contexts saved on the stack
-        auto tool = std::find( m_activeTools.begin(), m_activeTools.end(),
+        if( it != m_activeTools.end() )
-                                                        aState->theTool->GetId() );
+            it = m_activeTools.erase( it );
-
-        if( tool != m_activeTools.end() )
-        {
-            shouldDeactivate = true;
-            m_viewControls->Reset();
-
-            if( aDeactivate )
-                m_activeTools.erase( tool );
-        }
     }
 
+    // Restore VIEW_CONTROLS settings stored by the previous tool
+    if( TOOL_STATE* state = GetCurrentToolState() )
+        m_viewControls->ApplySettings( state->vcSettings );
+
     // Set transitions to be ready for future TOOL_EVENTs
     aState->theTool->SetTransitions();
 
-    return shouldDeactivate;
+    return it;
 }
 
 

common/view/view_controls.cpp

@@ -70,7 +70,6 @@ void VIEW_CONTROLS::SETTINGS::Reset()
 void VIEW_CONTROLS::ApplySettings( const SETTINGS& aSettings )
 {
     ShowCursor( aSettings.m_showCursor );
-    ForceCursorPosition( aSettings.m_forceCursorPosition, aSettings.m_forcedPosition );
     CaptureCursor( aSettings.m_cursorCaptured );
     SetSnapping( aSettings.m_snappingEnabled );
     SetGrabMouse( aSettings.m_grabMouse );
@@ -79,4 +78,9 @@ void VIEW_CONTROLS::ApplySettings( const SETTINGS& aSettings )
     SetAutoPanSpeed( aSettings.m_autoPanSpeed );
     EnableCursorWarping( aSettings.m_warpCursor );
     EnableMousewheelPan( aSettings.m_enableMousewheelPan );
+
+    // disable 'force cursor position' to prevent awkward cursor jumping    // TODO comment
+    m_settings.m_forceCursorPosition = false;
+    VECTOR2D cursorPos = GetCursorPosition();
+    ForceCursorPosition( aSettings.m_forceCursorPosition, cursorPos );
 }

include/tool/tool_manager.h

@@ -47,11 +47,20 @@ class wxWindow;
  */
 class TOOL_MANAGER
 {
+private:
+    struct TOOL_STATE;
+
 public:
     TOOL_MANAGER();
 
     ~TOOL_MANAGER();
 
+    // Helper typedefs
+    typedef std::map<TOOL_BASE*, TOOL_STATE*> TOOL_STATE_MAP;
+    typedef std::map<std::string, TOOL_STATE*> NAME_STATE_MAP;
+    typedef std::map<TOOL_ID, TOOL_STATE*> ID_STATE_MAP;
+    typedef std::list<TOOL_ID> ID_LIST;
+
     /**
      * Generates a unique ID from for a tool with given name.
      */
@@ -257,7 +266,7 @@ public:
      */
     inline int GetCurrentToolId() const
     {
-        return m_activeTools.front();
+        return m_activeTools.empty() ? -1 : m_activeTools.front();
     }
 
     /**
@@ -270,6 +279,16 @@ public:
         return FindTool( GetCurrentToolId() );
     }
 
+    /**
+     * Returns the TOOL_STATE object representing the state of the active tool. If there are no
+     * tools active, it returns nullptr.
+     */
+    TOOL_STATE* GetCurrentToolState() const
+    {
+        auto it = m_toolIdIndex.find( GetCurrentToolId() );
+        return ( it != m_toolIdIndex.end() ) ? it->second : nullptr;
+    }
+
     /**
      * Returns priority of a given tool. Higher number means that the tool is closer to the
      * beginning of the active tools queue (i.e. receives events earlier, tools with lower
@@ -332,7 +351,6 @@ public:
     std::string GetClipboard() const;
 
 private:
-    struct TOOL_STATE;
     typedef std::pair<TOOL_EVENT_LIST, TOOL_STATE_FUNC> TRANSITION;
 
     /**
@@ -406,11 +424,10 @@ private:
      * Deactivates a tool and does the necessary clean up.
      *
      * @param aState is the state variable of the tool to be stopped.
-     * @param aDeactivate decides if the tool should be removed from the active tools set.
+     * @return m_activeTools iterator. If the tool has been completely deactivated, it points
-     * @return True if the tool should be deactivated (note it does not necessarily  mean it has
+     * to the next active tool on the list. Otherwise it is an iterator pointing to aState.
-     * been deactivated, aDeactivate parameter decides).
      */
-    bool finishTool( TOOL_STATE* aState, bool aDeactivate = true );
+    ID_LIST::iterator finishTool( TOOL_STATE* aState );
 
     /**
      * Function isRegistered()
@@ -434,19 +451,19 @@ private:
     bool isActive( TOOL_BASE* aTool );
 
     /// Index of registered tools current states, associated by tools' objects.
-    std::map<TOOL_BASE*, TOOL_STATE*> m_toolState;
+    TOOL_STATE_MAP m_toolState;
 
     /// Index of the registered tools current states, associated by tools' names.
-    std::map<std::string, TOOL_STATE*> m_toolNameIndex;
+    NAME_STATE_MAP m_toolNameIndex;
+
+    /// Index of the registered tools current states, associated by tools' ID numbers.
+    ID_STATE_MAP m_toolIdIndex;
 
     /// Index of the registered tools to easily lookup by their type.
     std::map<const char*, TOOL_BASE*> m_toolTypes;
 
-    /// Index of the registered tools current states, associated by tools' ID numbers.
-    std::map<TOOL_ID, TOOL_STATE*> m_toolIdIndex;
-
     /// Stack of the active tools
-    std::list<TOOL_ID> m_activeTools;
+    ID_LIST m_activeTools;
 
     /// Instance of ACTION_MANAGER that handles TOOL_ACTIONs
     ACTION_MANAGER* m_actionMgr;

include/view/view_controls.h

@@ -288,10 +288,7 @@ public:
 
 protected:
     ///> Pointer to controlled VIEW.
-    VIEW*  m_view;
+    VIEW* m_view;
-
-    ///> Current cursor position (world coordinates)
-    VECTOR2D m_cursorPosition;
 
     ///> Current VIEW_CONTROLS settings
     SETTINGS m_settings;