← Back to team overview

kicad-developers team mailing list archive

[Patch] Hotkey validation/checking for 5.1

 

Attached is a patch that adds in hotkey validation/checking to the 5.1
branch. This will display error messages with the hotkeys when the user
edits the hotkeys, and notifies the user that there are invalid hotkeys
when viewing the list. It also checks imported files for their validity,
and warns the user when errors are found.

Currently, it does not check on startup of the program. It will only check
when a user is already editing/viewing the hotkeys. The only time this
actively prevents the user from doing something is when they enter an
invalid hotkey when changing a hotkey, other than that it is a notification
system.

I have been on the fence about adding a startup check of the hotkeys that
will then direct the user to the preferences menu to fix them, but in 5.1
that would be so scattered it would be somewhat painful to do it nicely.

This does not apply to the master branch currently, since Jeff just pushed
the start of the hotkey infrastructure rework to it. Eventually I do want
to add this to master, but I need to analyze his changes first and figure
out the best way of implementing it on the new framework.

-Ian
From 560176efe371ba20721ef21c361bc8cb4e6ad44e Mon Sep 17 00:00:00 2001
From: Ian McInerney <Ian.S.McInerney@xxxxxxxx>
Date: Tue, 11 Jun 2019 17:38:00 +0100
Subject: [PATCH] Added hotkey validity checking to the preferences menu

---
 common/dialogs/panel_hotkeys_editor.cpp |  90 ++++++++++++++++--
 common/hotkey_store.cpp                 | 108 ++++++++++++++++++++-
 common/widgets/widget_hotkey_list.cpp   | 119 +++++++++++++++++++++---
 include/hotkey_store.h                  |  91 +++++++++++++++++-
 include/panel_hotkeys_editor.h          |  20 ++++
 include/widgets/widget_hotkey_list.h    |  25 ++++-
 6 files changed, 421 insertions(+), 32 deletions(-)

diff --git a/common/dialogs/panel_hotkeys_editor.cpp b/common/dialogs/panel_hotkeys_editor.cpp
index 21e0082b9..26d7796dd 100644
--- a/common/dialogs/panel_hotkeys_editor.cpp
+++ b/common/dialogs/panel_hotkeys_editor.cpp
@@ -21,13 +21,16 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
  */
 
-#include <panel_hotkeys_editor.h>
+#include <bitmaps.h>
+#include <confirm.h>
 #include <eda_base_frame.h>
+#include <panel_hotkeys_editor.h>
 
-#include <wx/srchctrl.h>
-#include <wx/panel.h>
 #include <wx/button.h>
+#include <wx/panel.h>
 #include <wx/sizer.h>
+#include <wx/srchctrl.h>
+#include <wx/statline.h>
 
 #include <widgets/button_row_panel.h>
 #include <widgets/ui_common.h>
@@ -70,7 +73,33 @@ PANEL_HOTKEYS_EDITOR::PANEL_HOTKEYS_EDITOR( EDA_BASE_FRAME* aFrame, wxWindow* aW
         m_hotkeyStore( aShowHotkeys )
 {
     const auto margin = KIUI::GetStdMargin();
-    auto mainSizer = new wxBoxSizer( wxVERTICAL );
+
+    m_mainSizer = new wxBoxSizer( wxVERTICAL );
+    m_errorMessageSizer = new wxBoxSizer( wxVERTICAL );
+
+    // Setup the sub-sizer to contain the bitmap and header text
+    wxBoxSizer* errImgHeadSizer = new wxBoxSizer( wxHORIZONTAL );
+
+    wxStaticBitmap* valid_img = new wxStaticBitmap(
+            this, wxID_ANY, KiBitmap( cancel_xpm ), wxDefaultPosition, wxDefaultSize, 0 );
+    wxStaticText* err_head = new wxStaticText( this, wxID_ANY, _( "Hotkey errors detected" ),
+            wxDefaultPosition, wxDefaultSize, wxALIGN_LEFT );
+    errImgHeadSizer->Add( valid_img, 0, wxALL, 5 );
+    errImgHeadSizer->Add( err_head, 0, wxALL | wxALIGN_CENTER_VERTICAL, 5 );
+    m_errorMessageSizer->Add( errImgHeadSizer, 0, wxTOP | wxLEFT | wxRIGHT, margin );
+
+    // Setup the error message to give the user information about any problems with the hotkeys,
+    // but only do this if they can actually change them
+    if( !m_readOnly )
+    {
+        m_errorMessage = new wxStaticText(
+                this, wxID_ANY, wxEmptyString, wxDefaultPosition, wxDefaultSize, wxALIGN_LEFT );
+        m_errorMessageSizer->Add( m_errorMessage, 0, wxALL, 5 );
+    }
+    m_errorMessageSizer->Add( new wxStaticLine( this ), 0, wxALL | wxEXPAND, 2 );
+
+    // Add the validity text to the main sizer and hide the entire sizer
+    m_mainSizer->Add( m_errorMessageSizer, 0, wxTOP | wxLEFT | wxRIGHT | wxEXPAND, margin );
 
     // Sub-sizer for setting a wider side margin
     // TODO: Can this be set by the parent widget- doesn't seem to be
@@ -87,9 +116,9 @@ PANEL_HOTKEYS_EDITOR::PANEL_HOTKEYS_EDITOR( EDA_BASE_FRAME* aFrame, wxWindow* aW
     if( !m_readOnly )
         installButtons( bMargins );
 
-    mainSizer->Add( bMargins, 1, wxEXPAND | wxRIGHT | wxLEFT, side_margins );
+    m_mainSizer->Add( bMargins, 1, wxEXPAND | wxRIGHT | wxLEFT, side_margins );
 
-    this->SetSizer( mainSizer );
+    this->SetSizer( m_mainSizer );
     this->Layout();
 
     // Connect Events
@@ -125,7 +154,7 @@ void PANEL_HOTKEYS_EDITOR::installButtons( wxSizer* aSizer )
             _( "Import..." ),
             _( "Import hotkey definitions from an external file, replacing the current values" ),
             [this]( wxCommandEvent& ){
-                m_frame->ImportHotkeyConfigFromFile( m_hotkeys, m_nickname );
+                onImportHotkeyConfigFromFile();
             }
         },
         {
@@ -144,6 +173,22 @@ void PANEL_HOTKEYS_EDITOR::installButtons( wxSizer* aSizer )
 }
 
 
+void PANEL_HOTKEYS_EDITOR::onImportHotkeyConfigFromFile()
+{
+    m_frame->ImportHotkeyConfigFromFile( m_hotkeys, m_nickname );
+
+    if( !m_hotkeyStore.TestStoreValidity() )
+    {
+        wxString msg = _( "The imported file contains invalid hotkeys. "
+                          "Please correct the errors before continuing." );
+
+        wxString errKeys;
+        m_hotkeyStore.GetStoreValidityMessage( errKeys );
+        DisplayErrorMessage( this, msg, errKeys );
+    }
+}
+
+
 bool PANEL_HOTKEYS_EDITOR::TransferDataToWindow()
 {
     return m_hotkeyListCtrl->TransferDataToControl();
@@ -167,3 +212,34 @@ void PANEL_HOTKEYS_EDITOR::OnFilterSearch( wxCommandEvent& aEvent )
     const auto searchStr = aEvent.GetString();
     m_hotkeyListCtrl->ApplyFilterString( searchStr );
 }
+
+
+void PANEL_HOTKEYS_EDITOR::UpdateErrorMessage()
+{
+    wxString validMessage;
+    bool     isValid = m_hotkeyStore.GetStoreValidityMessage( validMessage );
+
+    if( isValid )
+    {
+        // Hide the error message sizer if all the hotkeys are valid
+        if( !m_readOnly )
+        {
+            m_errorMessage->SetLabelText( wxEmptyString );
+            m_errorMessage->Update();
+        }
+
+        m_mainSizer->Hide( m_errorMessageSizer );
+    }
+    else
+    {
+        // Update the message text and ensure it is showing if there are errors
+        if( !m_readOnly )
+        {
+            m_errorMessage->SetLabelText( validMessage );
+            m_errorMessage->Update();
+        }
+
+        m_mainSizer->Show( m_errorMessageSizer );
+    }
+    m_mainSizer->Layout();
+}
diff --git a/common/hotkey_store.cpp b/common/hotkey_store.cpp
index 479d9c67a..f8c92488c 100644
--- a/common/hotkey_store.cpp
+++ b/common/hotkey_store.cpp
@@ -25,6 +25,9 @@
 
 HOTKEY_STORE::HOTKEY_STORE( EDA_HOTKEY_CONFIG* aHotkeys )
 {
+    m_isValid = false;
+    m_invalidityCauses = _( "Hotkeys not checked" );
+
     for( EDA_HOTKEY_CONFIG* section = aHotkeys; section->m_HK_InfoList; ++section )
     {
         m_hk_sections.push_back( genSection( *section ) );
@@ -113,13 +116,13 @@ void HOTKEY_STORE::ResetAllHotkeysToOriginal()
 }
 
 
-bool HOTKEY_STORE::CheckKeyConflicts( long aKey, const wxString& aSectionTag,
-        EDA_HOTKEY** aConfKey, EDA_HOTKEY_CONFIG** aConfSect )
+bool HOTKEY_STORE::CheckKeyConflicts( long aKey, const wxString& aSectionTag, EDA_HOTKEY** aConfKey,
+        EDA_HOTKEY_CONFIG** aConfSect, const int aIdCommand )
 {
     EDA_HOTKEY* conflicting_key = nullptr;
     EDA_HOTKEY_CONFIG* conflicting_section = nullptr;
 
-    for( auto& section: m_hk_sections )
+    for( auto& section : m_hk_sections )
     {
         const auto& sectionTag = *section.m_section.m_SectionTag;
 
@@ -135,7 +138,7 @@ bool HOTKEY_STORE::CheckKeyConflicts( long aKey, const wxString& aSectionTag,
         for( auto& hotkey: section.m_hotkeys )
         {
             auto& curr_hk = hotkey.GetCurrentValue();
-            if( aKey == curr_hk.m_KeyCode )
+            if( ( aKey == curr_hk.m_KeyCode ) && ( aIdCommand != curr_hk.m_Idcommand ) )
             {
                 conflicting_key = &curr_hk;
                 conflicting_section = &section.m_section;
@@ -151,4 +154,99 @@ bool HOTKEY_STORE::CheckKeyConflicts( long aKey, const wxString& aSectionTag,
         *aConfSect = conflicting_section;
 
     return conflicting_key == nullptr;
-}
\ No newline at end of file
+}
+
+
+bool HOTKEY_STORE::CheckKeyValidity( long aKey, wxString& aMessage )
+{
+    // Extract the modifiers and the keycode
+    int modifiers = aKey & ( GR_KB_SHIFT | GR_KB_CTRL | GR_KB_ALT );
+    int keycode = aKey & 0x00FFFFFF;
+
+    // Hotkeys may not contain the shift+SYMBOL sequence.
+    // This sequence gets mapped to (UPPER SYMBOL) in the hotkey logic
+    if( modifiers & GR_KB_SHIFT )
+    {
+        // These catch the ASCII codes for the special characters
+        if( ( keycode <= 64 && keycode >= 32 ) ||
+            ( keycode <= 96 && keycode >= 91 ) ||
+            ( keycode <= 126 && keycode >= 123 )  )
+        {
+            aMessage = _( "A hotkey cannot contain the shift key and a symbol key." );
+            return false;
+        }
+    }
+
+// This code block can be used to test the key validity checks
+#if 0
+    if( modifiers & GR_KB_CTRL )
+    {
+        if( keycode == 'K' )
+        {
+            aMessage = "A hotkey cannot be ctrl+K";
+            return false;
+        }
+    }
+#endif
+
+    return true;
+}
+
+
+bool HOTKEY_STORE::TestStoreValidity()
+{
+    m_isValid = true;
+    m_invalidityCauses.Clear();
+
+    // Iterate over every key to test it
+    for( HOTKEY_SECTION& section : m_hk_sections )
+    {
+        for( CHANGED_HOTKEY& hotkey : section.m_hotkeys )
+        {
+            EDA_HOTKEY& curr_hk = hotkey.GetCurrentValue();
+            wxString    validMessage;
+
+            bool isValid = CheckKeyValidity( curr_hk.m_KeyCode, validMessage );
+
+            // If the key isn't valid, set it and continue
+            if( !isValid )
+            {
+                hotkey.SetValidity( false, validMessage );
+
+                m_invalidityCauses << wxGetTranslation( curr_hk.m_InfoMsg );
+                m_invalidityCauses << ": " << validMessage << "\n";
+                m_isValid = false;
+                continue;
+            }
+
+            // Test for duplication
+            const wxString&    sectionTag = *section.m_section.m_SectionTag;
+            EDA_HOTKEY*        conflicting_key = nullptr;
+            EDA_HOTKEY_CONFIG* conflicting_section = nullptr;
+
+            CheckKeyConflicts( curr_hk.m_KeyCode, sectionTag, &conflicting_key,
+                    &conflicting_section, curr_hk.m_Idcommand );
+
+            // Not valid if a conflicting key was found
+            if( conflicting_key != nullptr )
+            {
+                wxString keyInfoMsg = wxGetTranslation( conflicting_key->m_InfoMsg );
+                validMessage =
+                        wxString::Format( _( "Duplicate of hotkey for \"%s\"" ), keyInfoMsg );
+
+                hotkey.SetValidity( false, validMessage );
+
+                m_invalidityCauses << wxGetTranslation( curr_hk.m_InfoMsg );
+                m_invalidityCauses << ": " << validMessage << "\n";
+                m_isValid = false;
+                continue;
+            }
+
+            // If it made it this far, it is a valid hotkey
+            validMessage = _( "Hotkey is valid" );
+            hotkey.SetValidity( true, validMessage );
+        }
+    }
+
+    return m_isValid;
+}
diff --git a/common/widgets/widget_hotkey_list.cpp b/common/widgets/widget_hotkey_list.cpp
index d89b005b4..d41a11881 100644
--- a/common/widgets/widget_hotkey_list.cpp
+++ b/common/widgets/widget_hotkey_list.cpp
@@ -28,8 +28,12 @@
 
 #include <wx/statline.h>
 
-#include <draw_frame.h>
+#include <bitmaps.h>
+#include <confirm.h>
 #include <dialog_shim.h>
+#include <draw_frame.h>
+
+#include <panel_hotkeys_editor.h>
 
 
 /**
@@ -80,13 +84,17 @@ class HK_PROMPT_DIALOG : public DIALOG_SHIM
 
 public:
     HK_PROMPT_DIALOG( wxWindow* aParent, wxWindowID aId, const wxString& aTitle,
-            const wxString& aName, const wxString& aCurrentKey )
-        :   DIALOG_SHIM( aParent, aId, aTitle, wxDefaultPosition, wxDefaultSize )
+            const wxString& aName, const wxString& aCurrentKey, const bool aValidKey,
+            const wxString& aValidMessage )
+            : DIALOG_SHIM( aParent, aId, aTitle, wxDefaultPosition, wxDefaultSize )
     {
         wxPanel* panel = new wxPanel( this, wxID_ANY, wxDefaultPosition, wxDefaultSize );
         wxBoxSizer* sizer = new wxBoxSizer( wxVERTICAL );
 
         /* Dialog layout:
+         *
+         * valid_img   valid_label...........
+         * ----------------------------------
          *
          * inst_label........................
          * ----------------------------------
@@ -96,6 +104,25 @@ public:
          * key_label_0      key_label_1         /
          */
 
+        // If there is a validity error, display the error message to the user
+        wxBoxSizer* valid_sizer = new wxBoxSizer( wxHORIZONTAL );
+        if( !aValidKey )
+        {
+            wxStaticBitmap* valid_img = new wxStaticBitmap(
+                    panel, wxID_ANY, KiBitmap( cancel_xpm ), wxDefaultPosition, wxDefaultSize, 0 );
+            valid_sizer->Add( valid_img, 0, wxALL, 5 );
+
+            wxStaticText* valid_label = new wxStaticText( panel, wxID_ANY, wxEmptyString,
+                    wxDefaultPosition, wxDefaultSize, wxALIGN_CENTER_HORIZONTAL );
+
+            valid_label->SetLabelText( aValidMessage );
+            valid_sizer->Add( valid_label, 0, wxALL, 5 );
+
+            // Add the validity text to the main sizer
+            sizer->Add( valid_sizer, 0, wxALL, 5 );
+            sizer->Add( new wxStaticLine( panel ), 0, wxALL | wxEXPAND, 2 );
+        }
+
         wxStaticText* inst_label = new wxStaticText( panel, wxID_ANY, wxEmptyString,
                 wxDefaultPosition, wxDefaultSize, wxALIGN_CENTRE_HORIZONTAL );
 
@@ -188,8 +215,21 @@ public:
 
     void OnChar( wxKeyEvent& aEvent )
     {
-        m_event = aEvent;
-        EndFlexible( wxID_OK );
+        int keyCode = WIDGET_HOTKEY_LIST::MapKeypressToKeycode( aEvent );
+
+        // Test for if the key is valid
+        wxString validMsg;
+        if( HOTKEY_STORE::CheckKeyValidity( keyCode, validMsg ) )
+        {
+            // Valid key, close the window and return
+            m_event = aEvent;
+            EndFlexible( wxID_OK );
+        }
+        else
+        {
+            // Invalid key, tell the user
+            DisplayErrorMessage( this, validMsg, wxEmptyString );
+        }
     }
 
 
@@ -206,9 +246,10 @@ public:
 
 
     static wxKeyEvent PromptForKey( wxWindow* aParent, const wxString& aName,
-            const wxString& aCurrentKey )
+            const wxString& aCurrentKey, const wxString& aValidMessage, const bool aValidKey )
     {
-        HK_PROMPT_DIALOG dialog( aParent, wxID_ANY, _( "Set Hotkey" ), aName, aCurrentKey );
+        HK_PROMPT_DIALOG dialog( aParent, wxID_ANY, _( "Set Hotkey" ), aName, aCurrentKey,
+                aValidKey, aValidMessage );
 
         if( dialog.ShowModal() == wxID_OK )
         {
@@ -311,6 +352,9 @@ WIDGET_HOTKEY_CLIENT_DATA* WIDGET_HOTKEY_LIST::getExpectedHkClientData( wxTreeLi
 
 void WIDGET_HOTKEY_LIST::UpdateFromClientData()
 {
+    // Run a validity check on the hotkey store before updating
+    m_hk_store.TestStoreValidity();
+
     for( wxTreeListItem i = GetFirstItem(); i.IsOk(); i = GetNextItem( i ) )
     {
         WIDGET_HOTKEY_CLIENT_DATA* hkdata = GetHKClientData( i );
@@ -328,12 +372,22 @@ void WIDGET_HOTKEY_LIST::UpdateFromClientData()
 
             SetItemText( i, 0, wxGetTranslation( hk.m_InfoMsg ) );
             SetItemText( i, 1, key_text);
+
+            // Add the image to the column if the item is invalid
+            if( changed_hk.IsValid() )
+                SetItemImage( i, NO_IMAGE, NO_IMAGE );
+            else
+                SetItemImage( i, 0, NO_IMAGE );
         }
     }
 
     // Trigger a resize in case column widths have changed
     wxSizeEvent dummy_evt;
     TWO_COLUMN_TREE_LIST::OnSize( dummy_evt );
+
+    // Update the panel's error message if it exists
+    if( m_parentPanel )
+        m_parentPanel->UpdateErrorMessage();
 }
 
 
@@ -367,8 +421,11 @@ void WIDGET_HOTKEY_LIST::EditItem( wxTreeListItem aItem )
 
     wxString    name = GetItemText( aItem, 0 );
     wxString    current_key = GetItemText( aItem, 1 );
+    wxString    valid_msg;
 
-    wxKeyEvent key_event = HK_PROMPT_DIALOG::PromptForKey( GetParent(), name, current_key );
+    bool       valid_key = hkdata->GetChangedHotkey().IsValid( valid_msg );
+    wxKeyEvent key_event =
+            HK_PROMPT_DIALOG::PromptForKey( GetParent(), name, current_key, valid_msg, valid_key );
     long key = MapKeypressToKeycode( key_event );
 
     if( key )
@@ -506,11 +563,29 @@ bool WIDGET_HOTKEY_LIST::ResolveKeyConflicts( long aKey, const wxString& aSectio
 }
 
 
-WIDGET_HOTKEY_LIST::WIDGET_HOTKEY_LIST( wxWindow* aParent, HOTKEY_STORE& aHotkeyStore,
-            bool aReadOnly )
-    :   TWO_COLUMN_TREE_LIST( aParent, wxID_ANY, wxDefaultPosition, wxDefaultSize, wxTL_SINGLE ),
-        m_hk_store( aHotkeyStore ),
-        m_readOnly( aReadOnly )
+WIDGET_HOTKEY_LIST::WIDGET_HOTKEY_LIST(
+        PANEL_HOTKEYS_EDITOR* aParent, HOTKEY_STORE& aHotkeyStore, bool aReadOnly )
+        : TWO_COLUMN_TREE_LIST( aParent, wxID_ANY, wxDefaultPosition, wxDefaultSize, wxTL_SINGLE ),
+          m_hk_store( aHotkeyStore ),
+          m_readOnly( aReadOnly ),
+          m_parentPanel( aParent )
+{
+    initializeElements();
+}
+
+
+WIDGET_HOTKEY_LIST::WIDGET_HOTKEY_LIST(
+        wxWindow* aParent, HOTKEY_STORE& aHotkeyStore, bool aReadOnly )
+        : TWO_COLUMN_TREE_LIST( aParent, wxID_ANY, wxDefaultPosition, wxDefaultSize, wxTL_SINGLE ),
+          m_hk_store( aHotkeyStore ),
+          m_readOnly( aReadOnly ),
+          m_parentPanel( nullptr )
+{
+    initializeElements();
+}
+
+
+void WIDGET_HOTKEY_LIST::initializeElements()
 {
     wxString command_header = _( "Command" );
 
@@ -522,6 +597,11 @@ WIDGET_HOTKEY_LIST::WIDGET_HOTKEY_LIST( wxWindow* aParent, HOTKEY_STORE& aHotkey
     SetRubberBandColumn( 0 );
     SetClampedMinWidth( HOTKEY_MIN_WIDTH );
 
+    // Add the image for invalid hotkey
+    m_imgList = new wxImageList();
+    m_imgList->Add( KiBitmap( cancel_xpm ) );
+    AssignImageList( m_imgList );
+
     if( !m_readOnly )
     {
         // The event only apply if the widget is in editable mode
@@ -531,7 +611,6 @@ WIDGET_HOTKEY_LIST::WIDGET_HOTKEY_LIST( wxWindow* aParent, HOTKEY_STORE& aHotkey
     }
 }
 
-
 void WIDGET_HOTKEY_LIST::ApplyFilterString( const wxString& aFilterStr )
 {
     updateShownItems( aFilterStr );
@@ -556,6 +635,12 @@ void WIDGET_HOTKEY_LIST::ResetAllHotkeys( bool aResetToDefault )
 
     UpdateFromClientData();
     Thaw();
+
+    // Update the panel's error message if it exists
+    // Call here again since the freeze/thaw seems to disrupt the update
+    // inside UpdateFromClientData
+    if( m_parentPanel )
+        m_parentPanel->UpdateErrorMessage();
 }
 
 
@@ -592,6 +677,12 @@ void WIDGET_HOTKEY_LIST::updateShownItems( const wxString& aFilterStr )
 
     UpdateFromClientData();
     Thaw();
+
+    // Update the panel's error message if it exists
+    // Call here again since the freeze/thaw seems to disrupt the update
+    // inside UpdateFromClientData
+    if( m_parentPanel )
+        m_parentPanel->UpdateErrorMessage();
 }
 
 
diff --git a/include/hotkey_store.h b/include/hotkey_store.h
index fd5b5ec90..020fbf0f0 100644
--- a/include/hotkey_store.h
+++ b/include/hotkey_store.h
@@ -43,7 +43,10 @@ public:
         m_orig( aHotkey ),
         m_changed( aHotkey ),
         m_tag( aTag )
-    {}
+    {
+        m_isValid = false;
+        m_validMessage = _( "Hotkey never verified" );
+    }
 
     EDA_HOTKEY& GetCurrentValue()
     {
@@ -88,6 +91,40 @@ public:
         return m_tag;
     }
 
+    /**
+     * Return whether this hotkey has been flagged as invalid
+     *
+     * @param aMessage - If invalid, contains a string giving the reason for being invalid
+     * @return - true if valid, false otherwise
+     */
+    bool IsValid( wxString& aMessage ) const
+    {
+        aMessage = m_validMessage;
+        return m_isValid;
+    }
+
+    /**
+     * Return whether this hotkey has been flagged as invalid
+     *
+     * @return - true if valid, false otherwise
+     */
+    bool IsValid() const
+    {
+        return m_isValid;
+    }
+
+    /**
+     * Set if this hotkey is valid
+     *
+     * @param aValid - Flag giving true if valid, false otherwise
+     * @param amessage - Reason for being invalid (empty if hotkey is valid)
+     */
+    void SetValidity( bool aValid, wxString& aMessage )
+    {
+        m_isValid = aValid;
+        m_validMessage = aMessage;
+    }
+
 private:
     // Reference to an "original" hotkey config
     EDA_HOTKEY&     m_orig;
@@ -98,6 +135,11 @@ private:
     // The hotkey section tag, used to spot conflicts
     const wxString& m_tag;
 
+    // True if the key is a valid hotkey (has no invalid combinations)
+    bool m_isValid;
+
+    // Reason for being invalid
+    wxString m_validMessage;
 };
 
 /**
@@ -162,15 +204,48 @@ public:
     void ResetAllHotkeysToOriginal();
 
     /**
-     * Check whether the given key conflicts with anything in this store.
+     * Check whether the given key conflicts with anything in this store. If a command ID is
+     * specified, then the conflict will only trigger if the conflicting hotkey is for
+     * a different command ID.
      *
      * @param aKey - key to check
      * @param aSectionTag - section tag into which the key is proposed to be installed
      * @param aConfKey - if not NULL, outparam getting the key this one conflicts with
      * @param aConfSect - if not NULL, outparam getting the section this one conflicts with
+     * @param aIdCommand - Optional command ID for the key being tested
+     */
+    bool CheckKeyConflicts( long aKey, const wxString& aSectionTag, EDA_HOTKEY** aConfKey,
+            EDA_HOTKEY_CONFIG** aConfSect, const int aIdCommand = -1 );
+
+    /**
+     * Check if a given key contains only valid key combinations
+     *
+     * @param aKey - The key to check
+     * @param aMessage - If invalid, the outparam containing the message explaining the invalidity
+     * @return - true if valid, false if invalid
+     */
+    static bool CheckKeyValidity( long aKey, wxString& aMessage );
+
+    /**
+     * Test all hotkeys in the hotkey store for validity and conflicts with other keys
      */
-    bool CheckKeyConflicts( long aKey, const wxString& aSectionTag,
-            EDA_HOTKEY** aConfKey, EDA_HOTKEY_CONFIG** aConfSect );
+    bool TestStoreValidity();
+
+    /**
+     * Get a string containing all the errors detected during the validity test
+     * It is formatted as:
+     *    Action name 1: Reason key is invalid
+     *    Action name 2: Reason key is invalid
+     *    ...
+     *
+     * @param aMessage - outparam to store the message in
+     * @return true if no errors detected
+     */
+    bool GetStoreValidityMessage( wxString& aMessage )
+    {
+        aMessage = m_invalidityCauses;
+        return m_isValid;
+    }
 
 private:
 
@@ -182,6 +257,12 @@ private:
 
     // Internal data for every hotkey passed in
     SECTION_LIST m_hk_sections;
+
+    // String containing information on the causes of invalidity for the entire store
+    wxString m_invalidityCauses;
+
+    // Is the store valid
+    bool m_isValid;
 };
 
-#endif // HOTKEY_STORE__H
\ No newline at end of file
+#endif // HOTKEY_STORE__H
diff --git a/include/panel_hotkeys_editor.h b/include/panel_hotkeys_editor.h
index 91e40df90..65e3643d2 100644
--- a/include/panel_hotkeys_editor.h
+++ b/include/panel_hotkeys_editor.h
@@ -34,6 +34,7 @@
 
 class wxPanel;
 class wxSizer;
+class WIDGET_HOTKEY_LIST;
 
 
 class PANEL_HOTKEYS_EDITOR : public wxPanel
@@ -47,6 +48,10 @@ protected:
     HOTKEY_STORE              m_hotkeyStore;
     WIDGET_HOTKEY_LIST*       m_hotkeyListCtrl;
 
+    wxBoxSizer*               m_mainSizer;
+    wxBoxSizer*               m_errorMessageSizer;
+    wxStaticText*             m_errorMessage;
+
 public:
     PANEL_HOTKEYS_EDITOR( EDA_BASE_FRAME* aFrame, wxWindow* aWindow, bool aReadOnly,
                           EDA_HOTKEY_CONFIG* aHotkeys, EDA_HOTKEY_CONFIG* aShowHotkeys,
@@ -55,7 +60,22 @@ public:
     bool TransferDataToWindow() override;
     bool TransferDataFromWindow() override;
 
+    /**
+     * Update the error message display on the panel with the new messages.
+     */
+    void UpdateErrorMessage();
+
 private:
+    /**
+     * Initialize the elements of the panel.
+     */
+    void initializeElements();
+
+    /**
+     * Import hotkey configuration data from a file and verify the validity of the
+     * imported keys. Then prompt the user with the results.
+     */
+    void onImportHotkeyConfigFromFile();
 
     /**
      * Install the button panel (global reset/default, import/export)
diff --git a/include/widgets/widget_hotkey_list.h b/include/widgets/widget_hotkey_list.h
index 6c1065aa9..daacbc9cf 100644
--- a/include/widgets/widget_hotkey_list.h
+++ b/include/widgets/widget_hotkey_list.h
@@ -35,11 +35,13 @@
 #include <wx/treelist.h>
 #include <widgets/two_column_tree_list.h>
 
-#include <hotkeys_basic.h>
 #include <hotkey_store.h>
+#include <hotkeys_basic.h>
+#include <panel_hotkeys_editor.h>
 
 
 class WIDGET_HOTKEY_CLIENT_DATA;
+class PANEL_HOTKEYS_EDITOR;
 
 class WIDGET_HOTKEY_LIST : public TWO_COLUMN_TREE_LIST
 {
@@ -47,6 +49,8 @@ class WIDGET_HOTKEY_LIST : public TWO_COLUMN_TREE_LIST
     bool                        m_readOnly;
 
     wxTreeListItem              m_context_menu_item;
+    wxImageList*                m_imgList;
+    PANEL_HOTKEYS_EDITOR*       m_parentPanel;
 
     /**
      * Method GetHKClientData
@@ -164,9 +168,22 @@ public:
      * @param aParent - parent widget
      * @param aHotkeys - EDA_HOTKEY_CONFIG data - a hotkey store is constructed
      * from this.
+     * @param aReadOnly - true disallows edits of the hotkeys
      */
     WIDGET_HOTKEY_LIST( wxWindow* aParent, HOTKEY_STORE& aHotkeyStore, bool aReadOnly );
 
+    /**
+     * Constructor WIDGET_HOTKEY_LIST
+     * Create a WIDGET_HOTKEY_LIST that will update the panel's error message when
+     * new validity messages are available.
+     *
+     * @param aParent - parent hotkey panel
+     * @param aHotkeys - EDA_HOTKEY_CONFIG data - a hotkey store is constructed
+     * from this.
+     * @param aReadOnly - true disallows edits of the hotkeys
+     */
+    WIDGET_HOTKEY_LIST( PANEL_HOTKEYS_EDITOR* aParent, HOTKEY_STORE& aHotkeyStore, bool aReadOnly );
+
     /**
      * Method ApplyFilterString
      * Apply a filter string to the hotkey list, selecting which hotkeys
@@ -202,6 +219,12 @@ public:
      * Map a keypress event to the correct key code for use as a hotkey.
      */
     static long MapKeypressToKeycode( const wxKeyEvent& aEvent );
+
+private:
+    /**
+     * Initialize the elements of the widget
+     */
+    void initializeElements();
 };
 
 #endif // __widget_hotkey_list__
-- 
2.17.2


Follow ups