luc-github / ESP3D-WEBUI

A Web UI for ESP8266 or ESP32 based boards connected to 3D printers / CNC
GNU General Public License v3.0
734 stars 303 forks source link

3.0 keyboard shortcut mapping #288

Closed luc-github closed 1 year ago

luc-github commented 1 year ago

per @aaronse initial PR for shortcut remapping : https://github.com/luc-github/ESP3D-WEBUI/pull/285 Also now allows keys configuration for keyboard shortcut in settings Also allows shortcuts on macro buttons Also have better control on auto repeat keys

luc-github commented 1 year ago

@aaronse please give a try to this PR I have tried to cover most of I can I will merge it end of this week if no issue / comment for change

aaronse commented 1 year ago

Nice! Looks great to me. Great to see key shortcuts enabled for macros now too, that enables a bunch of scenarios...

Not important, but any thoughts on adding an undo button next to the delete button? I guess the user can always refresh page and start over with the key mapping changes they were making. Without an Undo button, they will learn to save regularly to avoid redoing a bunch of edits before they want to undo something. Again, not important, but maybe implementing undo for input fields is useful for other situations too.

Sent small edits in https://github.com/luc-github/ESP3D-WEBUI/pull/289 to enable more compact rendering of Key map items. Thoughts?

Thanks again @luc-github. Great to see UI support for keyboard shortcuts for jogging, macros and some other features. Also nice to see Terminal improvements with key shortcuts not disrupting User typing.

luc-github commented 1 year ago

the reset feature was present in previous UI and actually I removed it because no one seems using it, and it maked UI simpler and code also simpler and lighter.

luc-github commented 1 year ago

I will rebuild packages end of this week then

aaronse commented 1 year ago

@luc-github encountered some bugs/issues while testing on my actual CNC (SKR Pro) that did not show up during unit testing using Dev box local hosting. Will share more details, still testing, but think some edits and/or release notes might be needed...

aaronse commented 1 year ago

problem: Single letter key shortcuts are not working. For example expected pressing 'Z' to home Z axis, actual behavior is nothing happens.

cause: 1) Preferences.json files were updated with Uppercase character key. But e.key will be lowercase if Shift key is not pressed.

                    {
                        "id": "btnHZ",
                        "name": "btnHZ",
                        "key": "Z"      <-- 'Z' Should be lowercase, and/or, both settings and onkeypress code should be insensitive.
                    },

Some fix ideas :
a) Either, edit all Preferences.json files to instead have lowercase single letter key shortcuts. This fixes the problem today, however, tomorrow many others manually editing preferences.json will run into this and get frustrated too... b) Or, edit index.js line ~66 to be case insensitive for single key press. So change

keyval += e.key  

to instead be...

keyval += e.key.toUpperCase()  

This will work for most people, maybe Turkish 'i' or some other locales will be impacted IF they specified wrong casing in the preferences.json.

Thinking 'b' is better, other ideas/suggestions?

Adding console log in on key press handling code helps to debug this :-)

    console.log("KeyMap override match, key = " + e.key + ", cmd= " + cmdMatch + ", alt= " + e.altKey);
luc-github commented 1 year ago

I was in caps lock that may be why all are upper case I will move back to lower case, now user can change keys, upper or lower should be user choice no need to upper case

Help reflect the keys so user should be aware of necessary keys, even more if he remap the keys

aaronse commented 1 year ago

2) After upgrading to latest build and deploying to my SKR the key mapping icons we blank and keys values were unexpected. Sorry, I didn't capture screenshot. I fixed by deleting preferences.js, don't know if this is expected when switching between versions or not? Probably just an artifact of my preferences having older schema for keymap, this shouldn't be problem for other users since they never had keymap settings until now...

3) Use keyboard moved to unexpected location, was in jogging panel dropdown, now is in panels list drop down. I spent way too long trying to find. I noticed Settings panel has a Key Settings section containing Auto Repeat setting. Consider moving Use Keyboard to settings panel instead. Do you think that will be easier for people to find? Personally, I plan to always use the keyboard shortcuts when possible.

aaronse commented 1 year ago

upper or lower should be user choice no need to upper case

User will mess this up if they manually edit Preference.json. They will type accidentally type "X" when they should type "x". They will accidentally type "Shift+x' when they should type "Shift+X". What's the best option to minimize User frustration?

Edit: I guess Users will be OK if they only use the new UI for key shortcut settings.

luc-github commented 1 year ago

point 1 - I have checked vs code key binding and indeed it upper case - so I have followed your suggestion point 2- this is normal - I do not plan to handle any upgrade change in alpha - only for 3.0-> 3.X or it will be never ending so deleting preferences.json is the thing to be done if testing new alpha version - there is no release in alpha point 3 - I have put general configuration in panel general and shorcuts entry in macro list / jog panel and jog shortcut in jog panel - currently use a dedicated panel it is not possible if macros / jog - are different list , and will be a challenge to update shortcut list if macro change / added / is deleted , same for others panel if there is shortcut request for each panel - so let says it is feature

Edit : user should not edit manually Preference.json this is not good recommendation to give, I do not plan to deliver a Monkey tested fullproof solution sorry

aaronse commented 1 year ago

point 2- this is normal

Good to know. So users upgrading to new release will get the new keymap settings merged into their preferences, they don't need to delete their preferences file as part of the upgrade?

Edit : user should not edit manually Preference.json this is not good recommendation to give, I do not plan to deliver a Monkey tested fullproof solution sorry

LOL, makes sense.

Pulling and trying out latest version. Hope I'm helping more than I'm pestering :-)

luc-github commented 1 year ago

1 So users upgrading to new release will get the new keymap settings merged into their preferences, they don't need to delete their preferences file as part of the upgrade? yes backward compatibility is something important to me as well as breaking changes between version all 3.X but alpha / beta should be backward compatible with seamless upgrade, from another point of view upgrade from 2.X->3.X is out of scope for backward compatibility

3 - Hope I'm helping more than I'm pestering :-)

I am always open to discuss and listen constructive comments - as you saw after discussing I may also change my mind if I am convinced - and it also good for me to explain my dev choice - challenging them is ok, it make me reviewing my decision - some decision can be technical - architechtural - historical - but also related to the fact I am lazy (orz) or I have limited bandwidth to do everything and they are just decisions based on priorites list