kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

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