On 1/7/2016 4:25 PM, Chris Pavlina wrote: > 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?
This is correct. One space in headers, two in source files. > >> >> 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
