kicad-developers team mailing list archive

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

Jeff, I do think it makes more sense to have an accurate handled flag
rather than the current status. I have gone ahead and made the changes I
think correspond to changing this in the tool framework, and have attached
a patch that does that. It does require the prior patch I sent for the tool
trace to be applied first though.

So far, I haven't seen any problems but I haven't put all the actions
through testing.

Thoughts?

-Ian

On Sun, Jul 28, 2019 at 5:28 PM Jeff Young <jeff@xxxxxxxxx> wrote:

> Thanks for the extra info, Ian.  I’m now convinced that the hotkey
> dispatcher should return an accurate “handled” rather than the current
> “probably handled”.  Whatever bugs fall out from that we’ll just have to
> fix.
>
> Just to further JP’s wxEVT_CHAR_HOOK vs wxEVT_CHAR info, I sent out
> another email on it a couple of days ago [1].  If the
> dispatcher/tool-manager ends up skipping the wxEVT_CHAR_HOOK event,
> wxWidgets will generate a wxEVT_CHAR event (which is far more useful for
> hotkeys).  So often the way to “fix” a hotkey that’s not working due to
> modifier keys or keyboard differences is to make sure that the
> wxEVT_CHAR_HOOK event ends up getting skipped.  (That’s done in the
> dispatcher/tool-manager realm by calling evt->SetPassEvent(), which is
> why I added a bunch of them to the various tool event loops.)
>
> Cheers,
> Jeff.
>
> [1] https://lists.launchpad.net/kicad-developers/msg41706.html
>
> PS: while I now know my way around it to a certain extent, I’m no expert
> on the event dispatching stuff.  So healthy pinches of salt may be in
> order...
>
>
> On 28 Jul 2019, at 06:27, jp charras <jp.charras@xxxxxxxxxx> wrote:
>
> Le 28/07/2019 à 02:22, Jeff Young a écrit :
>
> I think JP ran into this earlier, so he might know more about it.
>
>
> In fact issue I h=worked on was the fact the Char events where no
> correctly skipped.
>
> If I correctly understand the issue related by this mail, this is a
> dispatch issue.
>
> I am thinking I already saw this issue.
> The key event dispatch incorrectly try to dispatch the key events:
> For me it try to find an action that matches the corresponding hotkey in
> its lists, regardless the fact the action is attached to the active
> frame (the frame having the focus) or not.
> If this is true, this dispatch behavior is really not correct:
> Only actions depending on the active frame must be taken in account.
> This is what the wxWidgets menu accelerators work.
>
>
> Now, about wxEVT_CHAR_HOOK and wxEVT_CHAR events (and related hotkeys):
> Among differences between these events, there is one major diff:
> wxEVT_CHAR_HOOK identify the keyboard key (i.e. the not shifted key code).
> wxEVT_CHAR identify the key code (depending on the fact the shift key is
> pressed or not).
> They are very different for keys that have 2 very different key codes.
>
> I explain:
>
> On a French keyboard, all "digit" keys have 2 key code (like in many
> other keyboards), but digit keys are shifted.
> For instance the '3' key is also the '"' key.
> In other words '3' and '3' use the same key ( I will call it 3")
> '3' needs to type SHIFT + 3" key
> '"' needs to type the 3" key
>
> Now what about wxEVT_CHAR_HOOK:
> wxEVT_CHAR_HOOK returns the key '"' with the modifier Shift.
> wxEVT_CHAR returns the key '3' with the modifier Shift.
>
> As a result: to show the 3D viewer in PCBNEW:
> - on an English keyboard you use Alt+3
> - on an French keyboard you use Alt+"
>
> And I am not sure this is a bug in wxWidgets:
> some other applications have this annoying behavior, for instance
> Firefox and its zoom commands (but I used also a few other apps showing
> this behavior).
>
>
> Another issue might be that at the end of the tool loop the event will get
> skipped which will send it to wxWidgets for processing (at which point it
> will probably come back as a hotkey again).  We might already have logic to
> deal with that, but it’s something to look out for.
>
> At the end of the day, though, we need to design around what is “right”.
> If what’s right breaks other stuff then we need to fix the other stuff.
>
> Cheers,
> Jeff.
>
> On 27 Jul 2019, at 17:48, Ian McInerney <Ian.S.McInerney@xxxxxxxx> wrote:
>
> In the tool dispatcher currently, if any action is associated with a key
> combination (even if the action is not handled by any active tools) then a
> key event with that combination will not progress further than the
> dispatchHotKey function in the dispatcher. For instance, this means that
> the letter 'B' will not make it into any of the dispatchInternal calls
> inside any program (e.g. eeschema, pleditor, cvpcb, etc.) because it is
> assigned to a pcbnew action to refill the zones, even when those other
> programs do not use that action at all so it goes unhandled in the
> dispatchHotkey function. That event therefore is inaccessible in any tool
> loops that may be running.
>
> Is there a reason the dispatchHotkey logic looks only at the fact an
> action with that hotkey exists rather than if any tool has handled the
> associated action? For my work in cvpcb it would be better if it were the
> latter, so that any key events not handled by the tools continue processing
> (e.g. for down/up/left/right keys, single letter keys, etc.). It should be
> possible to know if the action is handled by the hotkey handler, since they
> actions are spawned immediately and the handled return value is then
> available.
>
> Would a change to this system break any of the existing tool loops? e.g.,
> are any unable to cope with receiving key pressed events (I don't think any
> would be problematic, since some keys such as 'C' don't have an associated
> action and would therefore generate key pressed events in them)?
>
> -Ian
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>
>
>
> --
> Jean-Pierre CHARRAS
>
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>

From c9d72f526490d1517b7b8d2fda7da4d0e186d23f Mon Sep 17 00:00:00 2001
From: Ian McInerney <Ian.S.McInerney@xxxxxxxx>
Date: Sun, 28 Jul 2019 17:15:34 +0200
Subject: [PATCH] Modify dispatchHotkey to return if the event was actually
 handled

This will allow keyboard actions for hotkeys that map to actions
not used in any current tools to continue procesing instead of
being stopped just because they have an action assigned.

See: https://lists.launchpad.net/kicad-developers/msg41725.html
---
 common/tool/action_manager.cpp |  6 ++----
 common/tool/tool_manager.cpp   |  7 +++++--
 include/tool/tool_manager.h    | 21 +++++++++++++--------
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/common/tool/action_manager.cpp b/common/tool/action_manager.cpp
index cd14118ee..4849cc58d 100644
--- a/common/tool/action_manager.cpp
+++ b/common/tool/action_manager.cpp
@@ -149,8 +149,7 @@ bool ACTION_MANAGER::RunHotKey( int aHotKey ) const
                 "ACTION_MANAGER::RunHotKey Running action %s for hotkey %s", context->GetName(),
                 KeyNameFromKeyCode( aHotKey ) );
 
-        m_toolMgr->RunAction( *context, true );
-        return true;
+        return m_toolMgr->RunAction( *context, true );
     }
     else if( global )
     {
@@ -158,8 +157,7 @@ bool ACTION_MANAGER::RunHotKey( int aHotKey ) const
                 "ACTION_MANAGER::RunHotKey Running action: %s for hotkey %s", global->GetName(),
                 KeyNameFromKeyCode( aHotKey ) );
 
-        m_toolMgr->RunAction( *global, true );
-        return true;
+        return m_toolMgr->RunAction( *global, true );
     }
 
     wxLogTrace( kicadTraceToolStack, "ACTION_MANAGER::RunHotKey No action found for key %s",
diff --git a/common/tool/tool_manager.cpp b/common/tool/tool_manager.cpp
index 71870b267..e82e12668 100644
--- a/common/tool/tool_manager.cpp
+++ b/common/tool/tool_manager.cpp
@@ -278,8 +278,9 @@ bool TOOL_MANAGER::RunAction( const std::string& aActionName, bool aNow, void* a
 }
 
 
-void TOOL_MANAGER::RunAction( const TOOL_ACTION& aAction, bool aNow, void* aParam )
+bool TOOL_MANAGER::RunAction( const TOOL_ACTION& aAction, bool aNow, void* aParam )
 {
+    bool       handled = false;
     TOOL_EVENT event = aAction.MakeEvent();
 
     // Allow to override the action parameter
@@ -289,7 +290,7 @@ void TOOL_MANAGER::RunAction( const TOOL_ACTION& aAction, bool aNow, void* aPara
     if( aNow )
     {
         TOOL_STATE* current = m_activeState;
-        processEvent( event );
+        handled = processEvent( event );
         setActiveState( current );
         UpdateUI( event );
     }
@@ -297,6 +298,8 @@ void TOOL_MANAGER::RunAction( const TOOL_ACTION& aAction, bool aNow, void* aPara
     {
         PostEvent( event );
     }
+
+    return handled;
 }
 
 
diff --git a/include/tool/tool_manager.h b/include/tool/tool_manager.h
index d1056bdba..14622d994 100644
--- a/include/tool/tool_manager.h
+++ b/include/tool/tool_manager.h
@@ -122,30 +122,35 @@ public:
      * Function RunAction()
      * Runs the specified action.
      *
+     * This function will only return if the action has been handled when the action is run
+     * immediately (aNow = true), otherwise it will always return false.
+     *
      * @param aAction is the action to be invoked.
      * @param aNow decides if the action has to be run immediately or after the current coroutine
      * is preemptied.
      * @param aParam is an optional parameter that might be used by the invoked action. Its meaning
      * depends on the action.
+     *
+     * @return True if the action was handled immediately
      */
-    template<typename T>
-    void RunAction( const TOOL_ACTION& aAction, bool aNow = false, T aParam = NULL )
+    template <typename T>
+    bool RunAction( const TOOL_ACTION& aAction, bool aNow = false, T aParam = NULL )
     {
-        RunAction( aAction, aNow, reinterpret_cast<void*>( aParam ) );
+        return RunAction( aAction, aNow, reinterpret_cast<void*>( aParam ) );
     }
 
-    void RunAction( const TOOL_ACTION& aAction, bool aNow, void* aParam );
+    bool RunAction( const TOOL_ACTION& aAction, bool aNow, void* aParam );
 
-    void RunAction( const TOOL_ACTION& aAction, bool aNow = false )
+    bool RunAction( const TOOL_ACTION& aAction, bool aNow = false )
     {
-        RunAction( aAction, aNow, (void*) NULL );
+        return RunAction( aAction, aNow, (void*) NULL );
     }
 
     const std::map<std::string, TOOL_ACTION*>& GetActions();
-    
+
     ///> @copydoc ACTION_MANAGER::GetHotKey()
     int GetHotKey( const TOOL_ACTION& aAction );
-    
+
     ACTION_MANAGER* GetActionManager() { return m_actionMgr; }
 
     /**
-- 
2.21.0