kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

HI Wayne,

Yes, here is a separate patch for the 5.0 series.

Cheers,

John
On Fri, Sep 28, 2018 at 3:11 PM Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
>
> Hey John,
>
> Nice work!  I merged your patch into the development branch.  I do have
> one favor to ask if you have the time.  Patch 6 does not apply to the
> 5.0 branch which also suffers from the crash bug.  If you could merge it
> into the 5.0 branch and send me a separate patch I would appreciate it.
> If you don't have the time please let me know because this needs to be
> fixed for the 5.0.1 release.
>
> Cheers,
>
> Wayne
>
> On 09/28/2018 07:53 AM, John Beard wrote:
> > Hi,
> >
> > Here is a patch set for adding a filter control to the hotkey editor
> > dialog. Preview video: https://sendvid.com/je0cyg87
> >
> > Most of the work in the first commit is separating out the hotkey data
> > from the UI widget code.
> >
> > This also fixes a couple of other bugs (one crashing, and one able to
> > get the HK configs into a mess with conflicts) in that dialog, and
> > adds some mock objects to the common QA tests, which will make it
> > easier to get more of libcommon under test.
> >
> > A 5.0 series fix for the crashing bug will follow.
> >
> > This is also useful as groundwork for
> > https://bugs.launchpad.net/kicad/+bug/1778374 (a 5.1 milestone), but
> > that will need a further decision to choose between:
> >
> > * Use a read-only variant of the hotkey editor TreeListView for the
> > hotkey list dialog (needs more code: a new dialog and adding a
> > read-only mode to the current widget), or
> > * Remove the hotkey list dialog altogether and bind the "show hotkey
> > list" command to simply opening the prefs dialog to the hotkey panel
> >
> > Cheers,
> >
> > John
> >
> >
> >
> > _______________________________________________
> > 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 7b4692552f6bc03dff62d036ad0543a76dd11309 Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Thu, 27 Sep 2018 14:44:03 +0100
Subject: [PATCH] Prevent segfault when undoing or resetting non-hotkey rows

This is caused by:

* Not checking the hotkey data is not null when performing a
  hotkey action
* Allowing hotkey actions on non-hotkey rows.

This fixes both of these, and adds an assert to warn if someone
does manage to fire a hotkey action on a non-hotkey row (but it
won't crash).

This is a ported version of commit 704615721 on the master branch.

Fixes: lp:1794756
* https://bugs.launchpad.net/kicad/+bug/1794756
---
 common/widgets/widget_hotkey_list.cpp | 47 ++++++++++++++++++++-------
 include/widgets/widget_hotkey_list.h  |  7 ++++
 2 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/common/widgets/widget_hotkey_list.cpp b/common/widgets/widget_hotkey_list.cpp
index 66b38ad04..74c3f59b7 100644
--- a/common/widgets/widget_hotkey_list.cpp
+++ b/common/widgets/widget_hotkey_list.cpp
@@ -253,6 +253,18 @@ WIDGET_HOTKEY_CLIENT_DATA* WIDGET_HOTKEY_LIST::GetSelHKClientData()
 }
 
 
+WIDGET_HOTKEY_CLIENT_DATA* WIDGET_HOTKEY_LIST::getExpectedHkClientData( wxTreeListItem aItem )
+{
+    const auto hkdata = GetHKClientData( aItem );
+
+    // This probably means a hotkey-only action is being attempted on
+    // a row that is not a hotkey (like a section heading)
+    wxASSERT_MSG( hkdata != nullptr, "No hotkey data found for list item" );
+
+    return hkdata;
+}
+
+
 void WIDGET_HOTKEY_LIST::UpdateFromClientData()
 {
     for( wxTreeListItem i = GetFirstItem(); i.IsOk(); i = GetNextItem( i ) )
@@ -285,13 +297,10 @@ void WIDGET_HOTKEY_LIST::LoadSection( EDA_HOTKEY_CONFIG* aSection )
 
 void WIDGET_HOTKEY_LIST::EditItem( wxTreeListItem aItem )
 {
-    WIDGET_HOTKEY_CLIENT_DATA* hkdata = GetHKClientData( aItem );
+    WIDGET_HOTKEY_CLIENT_DATA* hkdata = getExpectedHkClientData( aItem );
 
     if( !hkdata )
-    {
-        // Activated item was not a hotkey row
         return;
-    }
 
     wxString    name = GetItemText( aItem, 0 );
     wxString    current_key = GetItemText( aItem, 1 );
@@ -299,7 +308,7 @@ void WIDGET_HOTKEY_LIST::EditItem( wxTreeListItem aItem )
     wxKeyEvent key_event = HK_PROMPT_DIALOG::PromptForKey( GetParent(), name, current_key );
     long key = MapKeypressToKeycode( key_event );
 
-    if( hkdata && key )
+    if( key )
     {
         // See if this key code is handled in hotkeys names list
         bool exists;
@@ -327,7 +336,11 @@ void WIDGET_HOTKEY_LIST::EditItem( wxTreeListItem aItem )
 
 void WIDGET_HOTKEY_LIST::ResetItem( wxTreeListItem aItem )
 {
-    WIDGET_HOTKEY_CLIENT_DATA* hkdata = GetHKClientData( aItem );
+    WIDGET_HOTKEY_CLIENT_DATA* hkdata = getExpectedHkClientData( aItem );
+
+    if( !hkdata )
+        return;
+
     EDA_HOTKEY* hk = &hkdata->GetHotkey();
 
     for( size_t sec_index = 0; sec_index < m_sections.size(); ++sec_index )
@@ -356,9 +369,14 @@ void WIDGET_HOTKEY_LIST::ResetItem( wxTreeListItem aItem )
 
 void WIDGET_HOTKEY_LIST::ResetItemToDefault( wxTreeListItem aItem )
 {
-    WIDGET_HOTKEY_CLIENT_DATA* hkdata = GetHKClientData( aItem );
+    WIDGET_HOTKEY_CLIENT_DATA* hkdata = getExpectedHkClientData( aItem );
+
+    if( !hkdata )
+        return;
+
     EDA_HOTKEY* hk = &hkdata->GetHotkey();
     hk->ResetKeyCodeToDefault();
+
     UpdateFromClientData();
 }
 
@@ -376,10 +394,17 @@ void WIDGET_HOTKEY_LIST::OnContextMenu( wxTreeListEvent& aEvent )
 
     wxMenu menu;
 
-    menu.Append( ID_EDIT, _( "Edit..." ) );
-    menu.Append( ID_RESET, _( "Undo Changes" ) );
-    menu.Append( ID_DEFAULT, _( "Restore Default" ) );
-    menu.Append( wxID_SEPARATOR );
+    WIDGET_HOTKEY_CLIENT_DATA* hkdata = GetHKClientData( m_context_menu_item );
+
+    // Some actions only apply if the row is hotkey data
+    if( hkdata )
+    {
+        menu.Append( ID_EDIT, _( "Edit..." ) );
+        menu.Append( ID_RESET, _( "Undo Changes" ) );
+        menu.Append( ID_DEFAULT, _( "Restore Default" ) );
+        menu.Append( wxID_SEPARATOR );
+    }
+
     menu.Append( ID_RESET_ALL, _( "Undo All Changes" ) );
     menu.Append( ID_DEFAULT_ALL, _( "Restore All to Default" ) );
 
diff --git a/include/widgets/widget_hotkey_list.h b/include/widgets/widget_hotkey_list.h
index dff73f03e..fe8eb087f 100644
--- a/include/widgets/widget_hotkey_list.h
+++ b/include/widgets/widget_hotkey_list.h
@@ -72,6 +72,13 @@ class WIDGET_HOTKEY_LIST : public TWO_COLUMN_TREE_LIST
      */
     WIDGET_HOTKEY_CLIENT_DATA* GetSelHKClientData();
 
+    /**
+     * Get the WIDGET_HOTKEY_CLIENT_DATA form an item and assert if it isn't
+     * found. This is for use when the data not being present indicates an
+     * error.
+     */
+    WIDGET_HOTKEY_CLIENT_DATA* getExpectedHkClientData( wxTreeListItem aItem );
+
     /**
      * Method UpdateFromClientData
      * Refresh the visible text on the widget from the rows' client data objects.
-- 
2.19.0