kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

Thanks for looking it over. :)

On Thu, Jan 07, 2016 at 04:22:15PM -0500, Wayne Stambaugh wrote:
> A few quick comments on your patch:
> 
> 1) It would be nice if the hotkey being changed wouldn't loose it's
> selection status after the key press.  If you decide you don't like your
> choice, you have to select it again with the mouse cursor.

Agreed, I've actually fixed that now. That was the original behavior, 
god knows why.

> 
> 2) I saw a few coding policy issues: some missing spaces between
> parentheses, a switch statement indentation is incorrect (not sure that
> ones yours), and there should be two empty lines between functions in
> source files instead of one empty line.

Okay, I'll run through the patch and see if I can find them.

Question about coding style: AFAIK it's supposed to be two lines between 
functions, but we seem to use a single line between method declarations 
in header files. Is this proper, or just old cruft?

> 
> 3) I noticed you are using "" in some of your include statements instead
> of <> which is fine but be careful.  As of right know, kicad's build
> config adds all of the source paths to the list of includes so #include
> "hotkeys.h" is not an issue but it really should be #include
> "../../include/hotkeys.h".  The reason is if "hotkeys.h" is not found in
> the source path it falls back to searching the system include paths
> which will add some overhead to the compile time.  I'm not sure why we
> decided to use <> everywhere.  There was a reason we went this way but I
> honestly don't remember what it was.  I've always preferred <> for
> system headers and "" for headers in the project source.  If you are
> going to use "", please use the fully qualified path to prevent
> confusion.  If I was looking at this code and I didn't know the source
> layout, I would assume hotkeys.h was in the same path as the dialog code.

Okay, will change that.

> 
> On 1/7/2016 12:15 PM, Chris Pavlina wrote:
> > I suppose I should report a few known bugs so far to save your time - 
> > this wasn't meant to be a completed patch, I just wanted help testing 
> > the primary logic.
> > 
> > 1) Minor imbalance of button size between old buttons and ones I have 
> > added
> > 
> > 2) Export Hotkeys saves the wrong data set (the set from global memory 
> > that would be overwritten by pressing OK, not the set from the GUI 
> > widget)
> > 
> > 3) Column sizing is fixed, and may look bad with grotesquely unusual 
> > fonts or themes ;)
> > 
> > Plus one known bug of the hotkeys system itself that may be exposed by 
> > testing my code:
> > 
> > 4) Some keys with international symbols cannot be used as hotkeys
> > 
> > I will have a final version fixing 1, 2, and 3 by tomorrow. Fixing 4 
> > would take some reworking of the hotkey system, so I think I'm going to 
> > leave that one as is for now (and perhaps file a report so we have it on 
> > record).
> > 
> > On Thu, Jan 07, 2016 at 10:52:52AM -0500, Wayne Stambaugh wrote:
> >> On 1/7/2016 9:59 AM, Chris Pavlina wrote:
> >>> Really? It applies just fine here with:
> >>>
> >>>     patch -Np1 -i options.patch
> >>>
> >>> In any case, here's a patch generated with the more usual mechanism:
> >>>
> >>>     https://misc.c4757p.com/hotkeys.patch
> >>
> >> This one does apply cleanly.  I'll let you know when I finish building
> >> an testing it.
> >>
> >> Thanks,
> >>
> >> Wayne
> >>
> >>>
> >>> Could also download the whole branch with:
> >>>
> >>>     git clone -b options https://github.com/cpavlina/kicad
> >>>
> >>>
> >>> On Thu, Jan 07, 2016 at 09:53:58AM -0500, Wayne Stambaugh wrote:
> >>>> Chris,
> >>>>
> >>>> This patch does not apply cleanly against the tip of the product branch
> >>>> so I cannot test it.
> >>>>
> >>>> Cheers,
> >>>>
> >>>> Wayne
> >>>>
> >>>> On 1/7/2016 9:28 AM, Chris Pavlina wrote:
> >>>>> Hey, can I get a couple more people to test the latest version of my 
> >>>>> hotkeys patch? Particularly someone on OS X, as I've not run into the 
> >>>>> guy who usually guineapigs my stuff on OS X since finishing it... :) 
> >>>>> Though testing on ALL platforms is welcome.
> >>>>>
> >>>>> https://github.com/cpavlina/kicad/compare/options.patch
> >>>>>
> >>>>> Make sure setting hotkeys works for all keys, that everything looks 
> >>>>> halfway decent, no unexpected behavior, etc... Hotkeys can be reset by 
> >>>>> right-clicking them in the list and choosing "Reset".
> >>>>>
> >>>>> The 'helper' dialog I used seems to be a fairly popular approach for 
> >>>>> this - it's fairly difficult to catch *any* keypress on a control 
> >>>>> embedded in a complex window, there's too much trying to steal 
> >>>>> navigation keys and whatnot. In my testing (on wxGTK and wxMSW Win10) it 
> >>>>> seems quite robust.
> >>>>>
> >>>>> Thank you all in advance. :)
> >>>>>
> >>>>> --
> >>>>> 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