On 10/2/2017 2:49 PM, Dan Green wrote: > > > On Friday, September 29, 2017 at 11:32 PM, jp charras wrote: > >> Le 29/09/2017 à 03:51, Dan Green a écrit : >>> Odd, on macOS the menu displays properly as the option symbol+F8. Yes I can >>> look into tomorrow, I >>> have kicad building on a linux machine at work. >>> >>> Looks like AddModifierToKey() in hotkeys_basic.cpp is adding an extra “Alt” >>> to any accelerator key >>> that’s not between ‘A’ - ‘Z’. >>> Anyone know why this is? I’ll try to figure it out tomorrow… >>> >>> Dan >> >> AddHotkeyName automatically adds “Alt” or "Ctrl" to a key if IS_ACCELERATOR >> option is used. >> >> IS_ACCELERATOR option modify the key code and is useful *only* if the "same" >> key (like hotkeys ZOOM >> F1 and F2 and a few others) is used *both* as hotkey and accelerator, and if >> the action is nearly >> the same but differs by using the mouse position. >> (For instance in zoom: the zoom center is the mouse position if the hotkey >> is pressed, and the >> screen center if the menu or accelerator key is pressed). >> >> This is due to the fact I never found a easy way (common to the 3 platforms) >> to know if a menuitem >> was activated by a click on it or by its accelerator key to use or not the >> mouse position.
The easiest way I found to do this is to convert the hotkey to a context menu command event. Context menu events typically use the right click mouse position to begin an operation. A hotkey is a shortcut for context menu actions. Most, if not all of the context menu entries that have equivalent hotkeys are already defined. >> >> If it is not the case (if the key is not also used as hotkey or if the mouse >> position is not used), >> do not use IS_ACCELERATOR option that adds a SHIFT or ALT to the base key. >> Just set the key code: in this case ALT+F8 (or perhaps CTRL+F8) >> > Thanks jp, I see what IS_ACCELERATOR is for now. > The easy solution would be to patch AddHotkeyName() and/or AddModifierToKey() > to disallow adding an Alt if an Alt > already exists in the hotkey (in which case, try adding Ctrl, etc). > > But thinking about this more, it seems that there are really two distinct > actions we want to do. For example: > Begin Wire (W = hotkey) vs. switch to Wire Tool (Shift+W = accelerator/menu) > > Perhaps each action needs its own entry in the *_Hotkey_List, and its own > command event ID (e.g. there would be > ID_BEGIN_WIRE in addition to ID_WIRE_BUTT) > > From the user’s perspective, this would match the menu bar displaying > Shift+W, vs. the WIDGET_HOTKEY_LIST > displaying just W. An added benefit would be that it won’t be confusing to a > user who tries to change a new hotkey > in the preferences, and sees it showing up as having an extra Shift or Alt > added to it in the menu bar. > > Is this worthwhile to change, or is there some other layer of complexity I’m > missing that requires us to tie the two > actions together? I can volunteer to tackle this unless you or someone else > would prefer. See my comments above. Over time, I have slowly converted the hotkeys over to context menu command events where applicable to reduce duplicate code. As to whether or not this makes sense somewhat depends on how long before the schematic editor is converted over to use GAL and the new tool framework which handles hotkeys differently. > > Also on my short to-do list is to bring the hotkeys up to compliance with Mac > OS keyboards, allowing for use of (raw) Ctrl > for hotkeys, and calling Alt by it’s Mac name “Opt” I cannot comment on this since I'm not a mac user but there are some rather interesting bits of code that attempt to normalize key modifiers between platforms in both wxWidgets and KiCad which could cause issues. I would like to avoid ugly #ifdef OSX/#endif code as much as possible. > > thanks, > Dan >> >>> >>> >>>> On Sep 28, 2017, at 9:47 AM, Wayne Stambaugh <[email protected] >>>> <mailto:[email protected]>> >>>> wrote: >>>> >>>> Dan, >>>> >>>> I found an issue but I don't think it's your issue but rather a bug in >>>> the AddHotKeyName() function. The menu accelerator for the "Update PCB >>>> from Schematic" menu entry ends up being "Alt + Alt + F8" instead of >>>> "Alt + F8" (see attached image). Do you have time to look at this and >>>> see if you can fix it? The broken code is in common/hotkeys.cpp. This >>>> will have to be fixed to include your patch. >>>> >>>> Wayne >>>> >>>> On 9/26/2017 6:35 PM, Dan Green wrote: >>>>> Ah yes, of course, all that makes sense. Thanks for explaining it. >>>>> Alt+F8 is available, so attached is a patch using that as the default >>>>> (and with IS_ACCELERATOR). >>>>> thanks, >>>>> Dan >>>>> >>>>> >>>>> On Tuesday, September 26, 2017 at 6:57 AM, Wayne Stambaugh wrote: >>>>> >>>>>> Hey Dan, >>>>>> >>>>>> I have a few comments on your patches. >>>>>> >>>>>> The "Update PCB from Schematic" patch uses a duplicate hotkey. F8 is >>>>>> already assigned to the HK_SWITCH_LAYER_TO_INNER4 command ID. Check the >>>>>> pcbnew/hotkeys.cpp file for the list of assigned hotkey. Also, you are >>>>>> using F8 as a menu accelerator not a hotkey which requires you to add >>>>>> the IS_ACCEL HOTKEY_ACTION_TYPE to the AddHotkeyName() call. You are >>>>>> going to have to choose a different hotkey and resubmit your patch. >>>>>> >>>>>> Technically the file name fix patch works but you should use the >>>>>> wxFileName( path, name, ext) ctor to create the full file name. I've >>>>>> been trying to weed out this particular issue in KiCad but I must have >>>>>> missed this one. Also, there is no need to wrap "%s.%s" in with the >>>>>> internationalization macro _(). There is no text to translate in the >>>>>> string. I will fix this one now that I know where the issue exists. >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Wayne >> >> >> -- >> Jean-Pierre CHARRAS >> >> _______________________________________________ >> Mailing list: https://launchpad.net/~kicad-developers >> Post to : [email protected] >> (mailto:[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
