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 : [email protected] > >>>>> Unsubscribe : https://launchpad.net/~kicad-developers > >>>>> More help : https://help.launchpad.net/ListHelp > >>>>> > >>>> > >>>> _______________________________________________ > >>>> Mailing list: https://launchpad.net/~kicad-developers > >>>> Post to : [email protected] > >>>> Unsubscribe : https://launchpad.net/~kicad-developers > >>>> More help : https://help.launchpad.net/ListHelp _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
