kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #22337
Re: [PATCH] Refactor hotkeys editor dialog more
Oh, crap. Made a mistake in that one (added one GetParent() line but
forgot to remove the m_parent line it replaced). Here, this one is a
fixed version of patch 4.
On Tue, Jan 05, 2016 at 02:28:39PM -0500, Chris Pavlina wrote:
> 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 );
>
commit 913c1c2e81b03963f9126664593909843f1fd439
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..28c715e 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,19 @@ 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 );
- static_cast<SCH_EDIT_FRAME*>( m_parent )->Process_Config( aEvent );
+ GetParent()->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 );
References
