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.
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. 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. 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
