kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

Okay, here's the fourth and hopefully final patch.

- Fix wxTreeListCtrl column sizing (had to override the one provided by 
  wx, it's buggy)

- Fix signed/unsigned comparison

- Remove unnecessary m_parent direct access and redundant storage

On Tue, Jan 05, 2016 at 01:00:21PM -0500, Wayne Stambaugh wrote:
> This patch looks fine.  I do like the tree view better than the tabbed
> view.  I got two build warnings using GCC 5.3.0 which probably should be
> fixed.  If it will prevent your third patch from applying cleanly, I can
> commit it as is and you can fix them later.  Here are the build warnings:
> 
> [ 41%] Building CXX object
> common/CMakeFiles/common.dir/dialogs/dialog_hotkeys_editor_base.cpp.obj
> In file included from C:/msys64/mingw32/include/wx-3.0/wx/defs.h:695:0,
>                  from C:/msys64/mingw32/include/wx-3.0/wx/wx.h:14,
>                  from
> C:/msys64/home/wstambaugh/src/kicad/product/include/fctsys.h:28,
>                  from
> C:/msys64/home/wstambaugh/src/kicad/product/common/dialogs/dialog_hotkeys_editor.cpp:26:
> C:/msys64/home/wstambaugh/src/kicad/product/common/dialogs/dialog_hotkeys_editor.cpp:
> In member function 'void HOTKEY_LIST_CTRL::DeselectRow(int)':
> C:/msys64/home/wstambaugh/src/kicad/product/common/dialogs/dialog_hotkeys_editor.cpp:63:33:
> warning: comparison between signed and unsigned integer expressions
> [-Wsign-compare]
>      wxASSERT( aRow >= 0 && aRow < m_items.size() );
>                                  ^
> C:/msys64/home/wstambaugh/src/kicad/product/common/dialogs/dialog_hotkeys_editor.cpp:
> In member function 'void HOTKEY_LIST_CTRL::OnChar(wxKeyEvent&)':
> C:/msys64/home/wstambaugh/src/kicad/product/common/dialogs/dialog_hotkeys_editor.cpp:115:40:
> warning: unused variable 'parent' [-Wunused-variable]
>                  HOTKEYS_EDITOR_DIALOG* parent =
> static_cast<HOTKEYS_EDITOR_DIALOG*>( m_parent );
> 
> I also noticed that this dialog is storing the pointer to the parent
> window in the m_parent member variable.  This is redundant since the
> wxDialog base class saves the pointer to the parent window and can be
> accessed using wxWindow::GetParent().  GetParent() is virtual in case
> you need to overload it to return a specific type of parent window.
> This seems to be a pattern in a lot of our dialogs which I would like to
> see go away.
> 
> Let me know if you want me to commit it as is or wait for a patch to fix
> the build warnings.
> 
> Thanks,
> 
> Wayne
> 
> On 1/4/2016 4:28 PM, Chris Pavlina wrote:
> > Second step is to remove the pages from the hotkeys dialog and make it a 
> > single-page control, so we don't have nested tabs when it's brought into 
> > the already tabbed options dialog. This patch removes the pages, in 
> > favor of categories in the wxTreeListCtrl. It also makes the final few 
> > steps towards making the HOTKEY_LIST_CTRL fully embeddable.
> > 
> > This patch applies on top of the previous one. I recommend keeping them 
> > as separate commits, just in case I've created any bugs (makes them 
> > easier to track down).
> > 
> > There are a couple known GUI "quirks" - the columns are sized slightly 
> > wrong when there is a vectical scrollbar, and the column sizing changes 
> > a bit erratically when resizing the dialog. These are wx bugs. I'm going 
> > to address them after the logical bits are complete.
> > 
> > On Mon, Jan 04, 2016 at 03:05:33PM -0500, Chris Pavlina wrote:
> >> Hi,
> >>
> >> First step in getting the hotkey configuration inside the main 
> >> preferences dialog is to refactor it a bit. This patch:
> >>
> >> - Replaces the wxListCtrl with a wxTreeListCtrl, allowing expandable 
> >>   categories in a future change.
> >>
> >> - Cleans up the code to make HOTKEY_LIST_CTRL function a bit better on 
> >>   its own.
> >>
> >> - Migrates this dialog as well to TransferData{To,From}Window, use 
> >>   matching TransferData{To,From}Control methods on HOTKEY_LIST_CTRL so 
> >>   it is easy to embed.
> >>
> >> Despite being replaced by a tree-type control, none of the behavior has 
> >> been changed yet. The appearance changes slightly due to wxTreeListCtrl 
> >> looking a bit different.
> >>
> >> --
> >> Chris
> >>
> >>
> >>
> >> _______________________________________________
> >> 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

commit 3766dd9b1b92fd3a450cccba9614c048a88bfe10
Author: Chris Pavlina <cpavlin1@xxxxxxxxxxxxxx>
Date:   Mon Jan 4 23:26:11 2016 -0500

    Eeschema options+hotkeys fixes
    
    Fix wxTreeListCtrl column sizing
    
    Minor: fix signed/unsigned comparison
    
    Remove unnecessary m_parent direct access

diff --git a/common/dialogs/dialog_hotkeys_editor.cpp b/common/dialogs/dialog_hotkeys_editor.cpp
index 2a387e9..ac50980 100644
--- a/common/dialogs/dialog_hotkeys_editor.cpp
+++ b/common/dialogs/dialog_hotkeys_editor.cpp
@@ -27,6 +27,7 @@
 #include <pgm_base.h>
 #include <common.h>
 #include <confirm.h>
+#include <wx/dataview.h>
 
 #include <dialog_hotkeys_editor.h>
 
@@ -52,9 +53,8 @@ HOTKEY_LIST_CTRL::HOTKEY_LIST_CTRL( wxWindow *aParent, const HOTKEYS_SECTIONS& a
     AppendColumn( _( "Command" ) );
     AppendColumn( _( "Hotkey" ) );
 
-    SetColumnWidth( 1, 100 );
-
     Bind( wxEVT_CHAR, &HOTKEY_LIST_CTRL::OnChar, this );
+    Bind( wxEVT_SIZE, &HOTKEY_LIST_CTRL::OnSize, this );
 }
 
 
@@ -70,9 +70,34 @@ HOTKEYS_SECTIONS HOTKEY_LIST_CTRL::Sections( EDA_HOTKEY_CONFIG* aHotkeys )
 }
 
 
+void HOTKEY_LIST_CTRL::OnSize( wxSizeEvent& aEvent )
+{
+    // Handle this manually - wxTreeListCtrl screws up the width of the first column
+    wxDataViewCtrl* view = GetDataView();
+
+    if( !view )
+        return;
+
+    const wxRect rect = GetClientRect();
+    view->SetSize( rect );
+
+#ifdef wxHAS_GENERIC_DATAVIEWCTRL
+    {
+        wxWindow* const view = GetView();
+        view->Refresh();
+        view->Update();
+    }
+#endif
+
+    SetColumnWidth( 1, 100 );
+    SetColumnWidth( 0, rect.width - 120 );
+}
+
+
 void HOTKEY_LIST_CTRL::DeselectRow( int aRow )
 {
-    wxASSERT( aRow >= 0 && aRow < m_items.size() );
+    wxASSERT( aRow >= 0 );
+    wxASSERT( (size_t)( aRow ) < m_items.size() );
     Unselect( m_items[aRow] );
 }
 
@@ -277,7 +302,7 @@ bool HOTKEY_LIST_CTRL::ResolveKeyConflicts( long aKey, const wxString& aSectionT
             KeyNameFromKeyCode( aKey ), GetChars( info ),
             *(conflictingSection->m_Title) );
 
-        wxMessageDialog dlg( m_parent, msg, _( "Confirm change" ), wxYES_NO | wxNO_DEFAULT );
+        wxMessageDialog dlg( GetParent(), msg, _( "Confirm change" ), wxYES_NO | wxNO_DEFAULT );
 
         if( dlg.ShowModal() == wxID_YES )
         {
@@ -358,7 +383,6 @@ void InstallHotkeyFrame( EDA_BASE_FRAME* aParent, EDA_HOTKEY_CONFIG* aHotkeys )
 HOTKEYS_EDITOR_DIALOG::HOTKEYS_EDITOR_DIALOG( EDA_BASE_FRAME*    aParent,
                                               EDA_HOTKEY_CONFIG* aHotkeys ) :
     HOTKEYS_EDITOR_DIALOG_BASE( aParent ),
-    m_parent( aParent ),
     m_hotkeys( aHotkeys )
 {
     m_hotkeyListCtrl = new HOTKEY_LIST_CTRL( this, HOTKEY_LIST_CTRL::Sections( aHotkeys ) );
@@ -391,7 +415,7 @@ bool HOTKEYS_EDITOR_DIALOG::TransferDataFromWindow()
         return false;
 
     // save the hotkeys
-    m_parent->WriteHotkeyConfig( m_hotkeys );
+    GetParent()->WriteHotkeyConfig( m_hotkeys );
 
     return true;
 }
diff --git a/eeschema/dialogs/dialog_eeschema_options.cpp b/eeschema/dialogs/dialog_eeschema_options.cpp
index 35c68c1..8535750 100644
--- a/eeschema/dialogs/dialog_eeschema_options.cpp
+++ b/eeschema/dialogs/dialog_eeschema_options.cpp
@@ -47,7 +47,7 @@ enum IMP_EXP_MENU_IDS
     ID_EXPORT_HOTKEYS
 };
 
-DIALOG_EESCHEMA_OPTIONS::DIALOG_EESCHEMA_OPTIONS( wxWindow* parent ) :
+DIALOG_EESCHEMA_OPTIONS::DIALOG_EESCHEMA_OPTIONS( SCH_EDIT_FRAME* parent ) :
     DIALOG_EESCHEMA_OPTIONS_BASE( parent )
 {
     m_choiceUnits->SetFocus();
@@ -80,6 +80,12 @@ DIALOG_EESCHEMA_OPTIONS::DIALOG_EESCHEMA_OPTIONS( wxWindow* parent ) :
 }
 
 
+SCH_EDIT_FRAME* DIALOG_EESCHEMA_OPTIONS::GetParent()
+{
+    return static_cast<SCH_EDIT_FRAME*>( DIALOG_EESCHEMA_OPTIONS_BASE::GetParent() );
+}
+
+
 void DIALOG_EESCHEMA_OPTIONS::OnImpExpClick( wxCommandEvent& aEvent )
 {
     wxMenu menu;
@@ -103,19 +109,20 @@ void DIALOG_EESCHEMA_OPTIONS::OnMenu( wxCommandEvent& aEvent )
     {
     case ID_IMPORT_PREFS:
         aEvent.SetId( ID_CONFIG_READ );
-        static_cast<SCH_EDIT_FRAME*>( m_parent )->Process_Config( aEvent );
+        GetParent()->Process_Config( aEvent );
         break;
     case ID_EXPORT_PREFS:
         aEvent.SetId( ID_CONFIG_SAVE );
+        GetParent()->Process_Config( aEvent );
         static_cast<SCH_EDIT_FRAME*>( m_parent )->Process_Config( aEvent );
         break;
     case ID_IMPORT_HOTKEYS:
         aEvent.SetId( ID_PREFERENCES_HOTKEY_IMPORT_CONFIG );
-        static_cast<SCH_EDIT_FRAME*>( m_parent )->Process_Config( aEvent );
+        GetParent()->Process_Config( aEvent );
         break;
     case ID_EXPORT_HOTKEYS:
         aEvent.SetId( ID_PREFERENCES_HOTKEY_EXPORT_CONFIG );
-        static_cast<SCH_EDIT_FRAME*>( m_parent )->Process_Config( aEvent );
+        GetParent()->Process_Config( aEvent );
         break;
     default:
         wxFAIL_MSG("Unexpected menu ID");
diff --git a/eeschema/dialogs/dialog_eeschema_options.h b/eeschema/dialogs/dialog_eeschema_options.h
index 2235b0e..2afc07a 100644
--- a/eeschema/dialogs/dialog_eeschema_options.h
+++ b/eeschema/dialogs/dialog_eeschema_options.h
@@ -35,6 +35,7 @@
 #include <template_fieldnames.h>
 
 class HOTKEY_LIST_CTRL;
+class SCH_EDIT_FRAME;
 
 class DIALOG_EESCHEMA_OPTIONS : public DIALOG_EESCHEMA_OPTIONS_BASE
 {
@@ -96,7 +97,9 @@ public:
      *
      * @param parent The dialog's parent
      */
-    DIALOG_EESCHEMA_OPTIONS( wxWindow* parent );
+    DIALOG_EESCHEMA_OPTIONS( SCH_EDIT_FRAME* parent );
+
+    virtual SCH_EDIT_FRAME* GetParent();
 
     /**
      * Function GetUnitsSelection
diff --git a/include/dialog_hotkeys_editor.h b/include/dialog_hotkeys_editor.h
index 072a747..ea7b3d3 100644
--- a/include/dialog_hotkeys_editor.h
+++ b/include/dialog_hotkeys_editor.h
@@ -173,6 +173,12 @@ protected:
      * @param aEvent is the key press event, the keycode is retrieved from it
      */
     void OnChar( wxKeyEvent& aEvent );
+
+    /**
+     * Function OnSize
+     * Handle resizing of the control. Overrides the buggy wxTreeListCtrl::OnSize.
+     */
+    void OnSize( wxSizeEvent& aEvent );
 };
 
 
@@ -184,7 +190,6 @@ protected:
 class HOTKEYS_EDITOR_DIALOG : public HOTKEYS_EDITOR_DIALOG_BASE
 {
 protected:
-    EDA_BASE_FRAME* m_parent;
     struct EDA_HOTKEY_CONFIG* m_hotkeys;
 
     HOTKEY_LIST_CTRL* m_hotkeyListCtrl;
@@ -192,6 +197,11 @@ protected:
     bool TransferDataToWindow();
     bool TransferDataFromWindow();
 
+    virtual EDA_BASE_FRAME* GetParent()
+    {
+        return static_cast<EDA_BASE_FRAME*>( HOTKEYS_EDITOR_DIALOG_BASE::GetParent() );
+    }
+
 public:
     HOTKEYS_EDITOR_DIALOG( EDA_BASE_FRAME* aParent, EDA_HOTKEY_CONFIG* aHotkeys );