kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

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