kicad-developers team mailing list archive

[Ian McInerney <Ian.S.McInerney@xxxxxxxx>]

For some context menu actions, there was an issue when starting the tool
immediately where the first click to prime the tool would be at the menu
item's position instead of where the user clicked to open the menu. The
main tool I found that it happens to are the zone drawing tools
(specifically the draw cutout menu item). These two patches fix that issue.

The first patch cleans up the position handling for the tool events by
including mouse positions in the command events (since it is flagged to
include positions). It also implements a memory of menu positioning, so the
position used for a menu selection event is the position where the root
menu was opened. It also introduces a new event action type TA_PRIME that
is used to represent a tool prime click event (it is generated by the tool
manager using a position passed into the function).

The second patch updates some of the Pcbnew tools to use this new framework
to fix the issue of starting the drawing at the wrong position.

-Ian

From efe7cfc85ec2db66be44fcd5cf0edab102ce0133 Mon Sep 17 00:00:00 2001
From: Ian McInerney <Ian.S.McInerney@xxxxxxxx>
Date: Thu, 3 Oct 2019 17:55:05 +0200
Subject: [PATCH 1/2] Cleanup position handling for TOOL_EVENTs

* Make the events generated by the selection of context menu items
  have the position where the menu was opened
* Ensure that TC_COMMAND type events have their position set to
  be the cursor position where the event originated
---
 common/tool/action_menu.cpp  | 18 ++++++++++++++++++
 common/tool/tool_event.cpp   |  6 +++++-
 common/tool/tool_manager.cpp | 20 ++++++++++++++++++++
 include/tool/tool_event.h    |  8 ++++++++
 include/tool/tool_manager.h  |  9 +++++++++
 pcbnew/pcb_edit_frame.cpp    |  1 +
 6 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/common/tool/action_menu.cpp b/common/tool/action_menu.cpp
index 57416304b..e40a7d928 100644
--- a/common/tool/action_menu.cpp
+++ b/common/tool/action_menu.cpp
@@ -339,10 +339,16 @@ void ACTION_MENU::updateHotKeys()
 
 static int g_last_menu_highlighted_id = 0;
 
+// We need to store the position of the mouse when the menu was opened so it can be passed
+// to the command event generated when the menu item is selected.
+static VECTOR2D g_menu_open_position;
+
 
 void ACTION_MENU::OnIdle( wxIdleEvent& event )
 {
     g_last_menu_highlighted_id = 0;
+    g_menu_open_position.x = 0.0;
+    g_menu_open_position.y = 0.0;
 }
 
 
@@ -357,6 +363,12 @@ void ACTION_MENU::OnMenuEvent( wxMenuEvent& aEvent )
         if( m_dirty && m_tool )
             getToolManager()->RunAction( ACTIONS::updateMenu, true, this );
 
+        wxMenu* parent = dynamic_cast<wxMenu*>( GetParent() );
+
+        // Don't update the position if this menu has a parent
+        if( !parent && m_tool )
+            g_menu_open_position = getToolManager()->GetViewControls()->GetMousePosition();
+
         g_last_menu_highlighted_id = 0;
     }
     else if( type == wxEVT_MENU_HIGHLIGHT )
@@ -441,6 +453,12 @@ void ACTION_MENU::OnMenuEvent( wxMenuEvent& aEvent )
 
         TOOL_MANAGER* toolMgr = m_tool->GetManager();
 
+        // Pass the position the menu was opened from into the generated event if it is a select event
+        if( type == wxEVT_COMMAND_MENU_SELECTED )
+            evt->SetMousePosition( g_menu_open_position );
+        else
+            evt->SetMousePosition( getToolManager()->GetViewControls()->GetMousePosition() );
+
         if( g_last_menu_highlighted_id == aEvent.GetId() && !m_isContextMenu )
             evt->SetHasPosition( false );
 
diff --git a/common/tool/tool_event.cpp b/common/tool/tool_event.cpp
index ed7fdf95d..a4bece277 100644
--- a/common/tool/tool_event.cpp
+++ b/common/tool/tool_event.cpp
@@ -57,6 +57,10 @@ void TOOL_EVENT::init()
     m_passEvent = m_category == TC_MESSAGE || IsCancelInteractive() || IsActivate();
 
     m_hasPosition = ( m_category == TC_MOUSE || m_category == TC_COMMAND );
+
+    // Cancel tool doesn't contain a position
+    if( IsCancel() )
+        m_hasPosition = false;
 }
 
 
@@ -173,7 +177,7 @@ const std::string TOOL_EVENT_LIST::Format() const
 
 bool TOOL_EVENT::IsClick( int aButtonMask ) const
 {
-    return m_actions == TA_MOUSE_CLICK && ( m_mouseButtons & aButtonMask ) == m_mouseButtons;
+    return ( m_actions & TA_MOUSE_CLICK ) && ( m_mouseButtons & aButtonMask ) == m_mouseButtons;
 }
 
 
diff --git a/common/tool/tool_manager.cpp b/common/tool/tool_manager.cpp
index f56f6ce37..df3f14e83 100644
--- a/common/tool/tool_manager.cpp
+++ b/common/tool/tool_manager.cpp
@@ -283,6 +283,9 @@ bool TOOL_MANAGER::RunAction( const TOOL_ACTION& aAction, bool aNow, void* aPara
     bool       handled = false;
     TOOL_EVENT event = aAction.MakeEvent();
 
+    if( event.Category() == TC_COMMAND )
+        event.SetMousePosition( m_viewControls->GetCursorPosition() );
+
     // Allow to override the action parameter
     if( aParam )
         event.SetParameter( aParam );
@@ -303,6 +306,20 @@ bool TOOL_MANAGER::RunAction( const TOOL_ACTION& aAction, bool aNow, void* aPara
 }
 
 
+void TOOL_MANAGER::PrimeTool( const VECTOR2D& aPosition )
+{
+    int modifiers = 0;
+    modifiers |= wxGetKeyState( WXK_SHIFT ) ? MD_SHIFT : 0;
+    modifiers |= wxGetKeyState( WXK_CONTROL ) ? MD_CTRL : 0;
+    modifiers |= wxGetKeyState( WXK_ALT ) ? MD_ALT : 0;
+
+    TOOL_EVENT evt( TC_MOUSE, TA_PRIME, BUT_LEFT | modifiers );
+    evt.SetMousePosition( aPosition );
+
+    PostEvent( evt );
+}
+
+
 const std::map<std::string, TOOL_ACTION*>& TOOL_MANAGER::GetActions()
 {
     return m_actionMgr->GetActions();
@@ -320,6 +337,7 @@ bool TOOL_MANAGER::invokeTool( TOOL_BASE* aTool )
     wxASSERT( aTool != NULL );
 
     TOOL_EVENT evt( TC_COMMAND, TA_ACTIVATE, aTool->GetName() );
+    evt.SetMousePosition( m_viewControls->GetCursorPosition() );
     processEvent( evt );
 
     if( TOOL_STATE* active = GetCurrentToolState() )
@@ -742,6 +760,7 @@ void TOOL_MANAGER::DispatchContextMenu( const TOOL_EVENT& aEvent )
         else
         {
             TOOL_EVENT evt( TC_COMMAND, TA_CHOICE_MENU_CHOICE, -1 );
+            evt.SetHasPosition( false );
             evt.SetParameter( m );
             dispatchInternal( evt );
         }
@@ -751,6 +770,7 @@ void TOOL_MANAGER::DispatchContextMenu( const TOOL_EVENT& aEvent )
 
         // Notify the tools that menu has been closed
         TOOL_EVENT evt( TC_COMMAND, TA_CHOICE_MENU_CLOSED );
+        evt.SetHasPosition( false );
         evt.SetParameter( m );
         dispatchInternal( evt );
 
diff --git a/include/tool/tool_event.h b/include/tool/tool_event.h
index 36b70b47c..c1fcbac5b 100644
--- a/include/tool/tool_event.h
+++ b/include/tool/tool_event.h
@@ -115,6 +115,9 @@ enum TOOL_ACTIONS
     // Model has changed (partial update).
     TA_MODEL_CHANGE         = 0x200000,
 
+    // Tool priming event (a special mouse click)
+    TA_PRIME                = 0x400001,
+
     TA_ANY = 0xffffffff
 };
 
@@ -330,6 +333,11 @@ public:
         return m_actions & TA_CHOICE_MENU;
     }
 
+    bool IsPrime() const
+    {
+        return m_actions == TA_PRIME;
+    }
+
     ///> Returns information about key modifiers state (Ctrl, Alt, etc.)
     int Modifier( int aMask = MD_MODIFIER_MASK ) const
     {
diff --git a/include/tool/tool_manager.h b/include/tool/tool_manager.h
index 14622d994..3d65afd25 100644
--- a/include/tool/tool_manager.h
+++ b/include/tool/tool_manager.h
@@ -148,6 +148,15 @@ public:
 
     const std::map<std::string, TOOL_ACTION*>& GetActions();
 
+    /**
+     * Function PrimeTool()
+     * "Primes" a tool by sending a cursor left-click event with the mouse position set
+     * to the passed in position.
+     *
+     * @param aPosition is the mouse position to use in the event
+     */
+    void PrimeTool( const VECTOR2D& aPosition );
+
     ///> @copydoc ACTION_MANAGER::GetHotKey()
     int GetHotKey( const TOOL_ACTION& aAction );
 
diff --git a/pcbnew/pcb_edit_frame.cpp b/pcbnew/pcb_edit_frame.cpp
index a6e67b865..12010f796 100644
--- a/pcbnew/pcb_edit_frame.cpp
+++ b/pcbnew/pcb_edit_frame.cpp
@@ -568,6 +568,7 @@ void PCB_EDIT_FRAME::DoShowBoardSetupDialog( const wxString& aInitialPage,
 
         //this event causes the routing tool to reload its design rules information
         TOOL_EVENT toolEvent( TC_COMMAND, TA_MODEL_CHANGE, AS_ACTIVE );
+        toolEvent.SetHasPosition( false );
         m_toolManager->ProcessEvent( toolEvent );
 
         OnModify();
-- 
2.21.0

From 23dffcdd9e0f0f9d44b7e65cf8c83240ae710cbe Mon Sep 17 00:00:00 2001
From: Ian McInerney <Ian.S.McInerney@xxxxxxxx>
Date: Thu, 3 Oct 2019 18:21:52 +0200
Subject: [PATCH 2/2] pcbnew: Switch over some drawing tools to use PrimeTool

Before, if the tools were activated from the context menu,
they would start drawing where the menu item was selected
instead of where the menu was opened.
---
 pcbnew/router/router_tool.cpp |  2 +-
 pcbnew/tools/drawing_tool.cpp | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/pcbnew/router/router_tool.cpp b/pcbnew/router/router_tool.cpp
index fe963bbd0..17c82da06 100644
--- a/pcbnew/router/router_tool.cpp
+++ b/pcbnew/router/router_tool.cpp
@@ -878,7 +878,7 @@ int ROUTER_TOOL::MainLoop( const TOOL_EVENT& aEvent )
 
     // Prime the pump
     if( aEvent.HasPosition() )
-        m_toolMgr->RunAction( ACTIONS::cursorClick );
+        m_toolMgr->PrimeTool( m_startSnapPoint );
 
     // Main loop: keep receiving events
     while( TOOL_EVENT* evt = Wait() )
diff --git a/pcbnew/tools/drawing_tool.cpp b/pcbnew/tools/drawing_tool.cpp
index f0bd1e365..d518607ba 100644
--- a/pcbnew/tools/drawing_tool.cpp
+++ b/pcbnew/tools/drawing_tool.cpp
@@ -493,7 +493,7 @@ int DRAWING_TOOL::DrawDimension( const TOOL_EVENT& aEvent )
     m_toolMgr->RunAction( ACTIONS::refreshPreview );
 
     if( aEvent.HasPosition() )
-        m_toolMgr->RunAction( ACTIONS::cursorClick );
+        m_toolMgr->PrimeTool( aEvent.Position() );
 
     // Main loop: keep receiving events
     while( TOOL_EVENT* evt = Wait() )
@@ -504,7 +504,8 @@ int DRAWING_TOOL::DrawDimension( const TOOL_EVENT& aEvent )
         grid.SetSnap( !evt->Modifier( MD_SHIFT ) );
         grid.SetUseGrid( !evt->Modifier( MD_ALT ) );
         m_controls->SetSnapping( !evt->Modifier( MD_ALT ) );
-        VECTOR2I cursorPos = grid.BestSnapAnchor( m_controls->GetMousePosition(), nullptr );
+        VECTOR2I cursorPos = grid.BestSnapAnchor(
+                evt->IsPrime() ? evt->Position() : m_controls->GetMousePosition(), nullptr );
         m_controls->ForceCursorPosition( true, cursorPos );
 
         auto cleanup = [&] () {
@@ -1435,7 +1436,7 @@ int DRAWING_TOOL::DrawZone( const TOOL_EVENT& aEvent )
 
     // Prime the pump
     if( aEvent.HasPosition() )
-        m_toolMgr->RunAction( ACTIONS::cursorClick );
+        m_toolMgr->PrimeTool( aEvent.Position() );
 
     // Main loop: keep receiving events
     while( TOOL_EVENT* evt = Wait() )
@@ -1445,7 +1446,8 @@ int DRAWING_TOOL::DrawZone( const TOOL_EVENT& aEvent )
         grid.SetSnap( !evt->Modifier( MD_SHIFT ) );
         grid.SetUseGrid( !evt->Modifier( MD_ALT ) );
         m_controls->SetSnapping( !evt->Modifier( MD_ALT ) );
-        VECTOR2I cursorPos = grid.BestSnapAnchor( m_controls->GetMousePosition(), layers );
+        VECTOR2I cursorPos = grid.BestSnapAnchor(
+                evt->IsPrime() ? evt->Position() : m_controls->GetMousePosition(), layers );
         m_controls->ForceCursorPosition( true, cursorPos );
 
         auto cleanup = [&] () {
-- 
2.21.0