kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

Warnings are already fixed in an upcoming patch, go ahead and commit and 
I'll send another patch to apply on top (it fixes a couple other minor 
things too).

I'll have a look at the redundant pointer storage, that might be left 
over from some old code. I'll fix that if I can as well.

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