luc-github / ESP3D-WEBUI

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

Implement key map overrides for jogging. Prevent key shortcuts from disrupting typing in Text fields (e.g. Terminal Console) #284

Closed aaronse closed 1 year ago

aaronse commented 1 year ago

This is a rebased version of https://github.com/luc-github/ESP3D-WEBUI/pull/283, see original pull request containing details and discussion with @luc-github and @V1EngineeringInc

Edits:

aaronse commented 1 year ago

Nice project @luc-github, happy to have discovered!

Ended up here after @V1EngineeringInc introduced the V1 Engineering community to your project as part of an effort to enable cost effective Wifi support for SKR Pro based V1E CNCs/Printers/etc...

Here's some responses to https://github.com/luc-github/ESP3D-WEBUI/pull/283#issuecomment-1365550916

Can see example of how to use key remapping at https://www.youtube.com/watch?v=v_rLDywnOp8&t=473s

  • I think the available controls for shortkey should be listed in array because user has no idea of id of different controls , using a search may not work if the panel is not visible and we need to configure key in settings

Agree, with this PR, User can't tell from WebUI which Ids are available to use. Currently, I point people to https://github.com/luc-github/ESP3D-WEBUI/blob/71500118daf84f4ad4d6fff07890b9496c86cbfb/src/components/Panels/Jog.js#L132 they can find Ids there. Not ideal, but better than nothing. Ideally we don't have to manually create a array list of commands that is brittle, and vulnerable to becoming stale/outdated. I didn't come up with anything smart yet, maybe manually creating array list will be needed... Guessing you know best option.

  • Shortkeys should be auto detected (ask user to type them) or listed because user does not know what is the syntax of keys neither

Agree, list of overrides (like macro editing) that people click to add/remove would be easier to not get wrong. Don't know if enough people will use to justify the effort to create?

  • instead of using string which is ok when you know all use a ui like panels: with json format: name (fixed) which could be the id of control but it may be meaning less for user - TBD the id of control to be controled (fixed by above list of bullet 1), and the shortcut
  • Use the json definition from the begining so user can see all keys configured in settings

Thought about using JSON based syntax for the remapping, but figured this comma delimited list of key=action pairs would be easier for non coders to learn and use. Was fast to code as well and fix immediate frustration point we were having.

  • Reflect the help using the json definition would then be very easy even after update
  • doing this will allow to implement it for cnc with more / less than 3 motors

I like the idea of help being dynamically generated, was wondering whether list of elements and tool tip text could be queried at runtime without manually creating (and then having to maintain...) list of commands. Don't know if the html elements exist in the DOM at the time settings are being populated, my gut tells me not.

Has you did the shortcut management already from API point of view it just need an additionnal list type (shortkeys) to be displayed properly and use json and parse them

Let me know what do you think ,

@luc-github thank you for creating this project. Only recently started looking into the project's code, so am not much help at this point.

Would be great to get just enough changes needed to address some immediate usability things we're running into. Maybe more User friendly UI features come later, only if enough people will benefit from them? I have no idea what priority these changes are in your opinion. Again, thanks for being quick to respond. Cheers!

luc-github commented 1 year ago

Hi thank you a lot for your contribution the json is an all in one solution to handle help update, store information, the effort would be actually pretty limited because API already handle similar things (eg: macro, panels order, panels) so add new type should be trivial , and using json allow to generate UI that also is easier for non coder - actually Preact use a lot json object for everything

The issue with current PR is what I have listed and you actually agreed, the combination key talking before is definitly not a priority but introducing proper user configuration is IMHO

I suggest to do by step:

What do you think ?

Of course if you need help to understand the code let me know, also feel free to join discord server to chat on this

aaronse commented 1 year ago
  • Add an array with all controls that need the shortkey - lets do hardcoded first then I will think how to generate it later

Sure thing, where should array of valid Ids for commands go? e.g. "validCommands" : [ "btn-X", "btn+X", etc... ] Is there a single file containing constants/parent-config info, or should a copy of "validCommands" be in each /server/.../preferences.json file? Do we need to edit the following and add the hardcoded list to each of the following files, or somewhere else?

C:\git\ESP3D-WEBUI\server\CNC\GRBL\Flash\preferences.json C:\git\ESP3D-WEBUI\server\CNC\grblHAL\Flash\www\preferences.json C:\git\ESP3D-WEBUI\server\Plotter\HP-GL\Flash\preferences.json C:\git\ESP3D-WEBUI\server\Printer3D\Marlin\Flash\preferences.json C:\git\ESP3D-WEBUI\server\Printer3D\Marlin-embedded\Flash\preferences.json C:\git\ESP3D-WEBUI\server\Printer3D\Repetier\Flash\preferences.json C:\git\ESP3D-WEBUI\server\Printer3D\RepRap\Flash\preferences.json C:\git\ESP3D-WEBUI\server\public\preferences.json

  • Use Json as definition / reference object

Add an array with all controls that need the shortkey - lets do hardcoded first then I will think how to generate it later

Current PR adds keymap to preferences.json :

"keymap": "ArrowLeft=btn-X,ArrowRight=btn+X,ArrowUp=btn+Y,ArrowDown=btn-Y,'=btn+Z,/=btn-Z",

What definition/format are you thinking? Although the above is valid JSON, based on JSON format for panelsorder and extracontents, I'm guessing you're thinking something like...

"keymap": [ { "key" : "ArrowLeft", "cmd" : "btn-X" }, { "key" : "ArrowRight", "cmd" : "btn+X" }, { "key" : "ArrowUp", "cmd" : "btn+Y" }, { "key" : "ArrowDown", "cmd" : "btn-Y" }, { "key" : "btn+Z", "cmd" : "/=btn-Z" } . . . ]

Doesn't feel right asking non/semi technical Users to enter verbose easy to screw up JSON syntax when I don't know if/when UI would happen. Even if UI comes in later, it could detect string instead of object array has been configured, and then parse the 'old' string representation into JSON object graph, should be just a few lines of code. Or, when UI rolls out, we just ask future users to redefine their keymappings using the new UI that uses true JSON graph, similar to extracontents and macros.

  • Use the preference.json as configuration input -> the UI will come after in settings

As much as I like JSON and normally gravitate to it, I'm not sure the upfront verbose representation is worth the labor we're imposing on people until the UI is implemented. Would you expect to get to that soon?

Some additional context about my situation, there's some other pending/backlog tasks I'd like to dig into and contribute to, details here if interested... https://forum.v1engineering.com/t/headless-skr-pro-info/34707/211 So, being open and honest, am keen to do minimal right steps to help make progress on this area. So we can move onto other changes that help User experience. That said, I need to take steps you consider to be in the right direction. Cheers!

luc-github commented 1 year ago

the UI setting will take me 2h I think so it is not a bottle neck - but I won't work upfront until backend work is finalized edit the json is only for initial testing not for end user - and yes the goal is user can define in same way as Macro I would suggest id instead of cmd :

"keymap": [
{
"key" : "ArrowDown",
"id" : "btn-Y"
},...

and will be part of jog entry of preferences.json

for the array I suggest one entry for array in https://github.com/luc-github/ESP3D-WEBUI/blob/3.0/src/targets/Printer3D/Marlin/variablesTable.js it is available in each target

aaronse commented 1 year ago

the UI setting will take me 2h I think so it is not a bottle neck - but I won't work upfront until backend work is finalized edit the json is only for initial testing not for end user - and yes the goal is user can define in same way as Macro

Great, appreciate your support!

I would suggest id instead of cmd :

Sounds good to me.

and will be part of jog entry of preferences.json

I don't understand "jog entry of preferences.json" ? Was expecting new keymap : [] json fragment to be added similar to macros : [...] and extracontents : [...] which are both inside settings : {...}

Reasoning... ESP3D keyboard shortcuts are mostly for jogging related functions. However, it's able to, and currently does already, have mappings for UI controls outside of the job panel, "btnEStop" for example.

for the array I suggest one entry for array in https://github.com/luc-github/ESP3D-WEBUI/blob/3.0/src/targets/Printer3D/Marlin/variablesTable.js it is available in each target

Not clear on this, will look some more. Seems like most, but not all the commands are defined in jog.js

Pasted some examples of list of Ids, got a preference? Unfortunately the jog DOM elements don't exist when Settings page is being displayed. So different approach would be needed if you don't want to hardcode. Anyway, here's my quick attempt...

image

List of valid Ids:

Option 1 - Includes just commands with tooltip text. Seems like more commands should have tooltip text defined?

[...document.getElementsByTagName("*")].filter(e => e.attributes["data-tooltip"] && e.id && e.click).map(e => e.id)

[ "btnEStop", "btn+X", "btnHX", "btn-X", "btn+Y", "btnHY", "btn-Y", "btn+Z", "btnHZ", "btn-Z", "btnHXYZ", "btnMoveXY", "btnMoveZ", "btnMotorOff" ]

Option 2 - Includes any and everything clickable...

[...document.getElementsByTagName("*")].filter(e => e.id && e.click).map(e => e.id)

[ "app", "menu", "dashboardLink", "settingsLink", "gqadhki7e", "1xkmvkgvv", "main", "dashboard", "btnEStop", "jogPanel", "btn+X", "btnHX", "btn-X", "btn+Y", "btnHY", "btn-Y", "btn+Z", "btnHZ", "btn-Z", "move_100", "move_10", "move_1", "move_0_1", "btnHXYZ", "btnMoveXY", "btnMoveZ", "btnMotorOff", "group-extruder-weight-0", "extruder-weight-0", "extruder-weight-0", "group-extruder-weight-1", "extruder-weight-1", "extruder-weight-1", "group-extruder-weight-2", "extruder-weight-2", "extruder-weight-2", "group-extruder-weight-3", "extruder-weight-3", "extruder-weight-3", "group-extruder-weight-4", "extruder-weight-4", "extruder-weight-4", "group-extruder-weight-5", "extruder-weight-5", "extruder-weight-5", "group-input-extruder-mixed", "input-extruder-mixed", "chart1", "chart2", "chart3" ]

Option 3 - Same as option 1 but also includes non localized command description, useful for label/description maybe?

[...document.getElementsByTagName("*")].filter(e => e.attributes["data-tooltip"] && e.id && e.click).map(e => new Object({ "id" : e.id, "data-tooltip" : e.getAttribute("data-tooltip") }))

[ { "id": "btnEStop", "data-tooltip": "Emergency Stop" }, { "id": "btn+X", "data-tooltip": "Jog up" }, { "id": "btnHX", "data-tooltip": "Home X axis" }, { "id": "btn-X", "data-tooltip": "Jog down" }, { "id": "btn+Y", "data-tooltip": "Jog up" }, { "id": "btnHY", "data-tooltip": "Home Y axis" }, { "id": "btn-Y", "data-tooltip": "Jog down" }, { "id": "btn+Z", "data-tooltip": "Jog up" }, { "id": "btnHZ", "data-tooltip": "Home Z axis" }, { "id": "btn-Z", "data-tooltip": "Jog down" }, { "id": "btnHXYZ", "data-tooltip": "Home all axis" }, { "id": "btnMoveXY", "data-tooltip": "Move to: 100,100" }, { "id": "btnMoveZ", "data-tooltip": "Move Z to100" }, { "id": "btnMotorOff", "data-tooltip": "Motors Off" } ]

I don't know enough about preact/react to generate the list at runtime within the Settings panel. Render jog component as non-visible html element maybe. Alternatively, Dashboard generates and stores list of command IDs at runtime, populates preferences with generated list. Setting page looking up cached list, funny and ugly.

luc-github commented 1 year ago

yes preact/React genrate DOM so when you are in settings there is no page dashboard and so the command : document.getElementsByTagName("*") won't work so for starting use hardcoded list is the easiest I was refering to this preferences.json as example https://github.com/luc-github/ESP3D-WEBUI/blob/3.0/src/targets/Printer3D/Marlin/preferences.json#L68 which is not the user one but the reference one: to create https://github.com/luc-github/ESP3D-WEBUI/blob/3.0/server/Printer3D/Marlin/Flash/preferences.json which is indeed flated to use less space as possible on flash

aaronse commented 1 year ago

Will do Option 2 for now. Easy to update anytime...

Thanks for the info, will dig around and try to submit commit with these edits. Cheers!

luc-github commented 1 year ago

Feel free if any question - good luck 😸

aaronse commented 1 year ago

Just submitted edits that use JSON representation of key remapping config.

Unit tested latest edits with remapping config pasted below. I didn't edit files to contain this fragment because I can't check-in edits that will change the default behavior for everyone. Let me know if there's Test Infra where build verification tests should go.

"keymap": [ { "key" : "ArrowLeft", "id" : "btn-X" }, { "key" : "ArrowRight", "id" : "btn+X" }, { "key" : "ArrowUp", "id" : "btn+Y" }, { "key" : "ArrowDown", "id" : "btn-Y" }, { "key" : "'", "id" : "btn+Z" }, { "key" : "/", "id" : "btn-Z" } ],

luc-github commented 1 year ago

thank you I will check this afternoon - I am busy with another topic this morning that I need to finish first

luc-github commented 1 year ago

in preferences.json of the target need to add the entry "sorted": false to avoid the arrows to change order and in the [] need to describe each shortkey 2 selects - one is the list of shortkey possible and the other one is the list of possible control I include a export of json settings for reference - there are 2 macro and one panel defined: preferences.txt

I have created a branch to do tests implementation https://github.com/luc-github/ESP3D-WEBUI/tree/3.0-keyboard-remaping, you can push your PR on it

I will have a look on filling the preferences.json properly - tomorrow it may save me time if you could generate an array with all control id and another one with all keymap accepted , I will use them to create a API that generate array based on not already used control id and keymap to avoid duplicated entries and share here

I still need to finish a task tomorrow morning and I can be full time on keymap UI and API implementation, so hope should be done this week (I am out on staturday)

luc-github commented 1 year ago

Ok I am on it now:

Please do not use / edit / change languages/printerpack/en.json it is temporary working file to generate package, the file to edit for shortcut keymapping is the general one for cnc and 3dprinter: https://github.com/luc-github/ESP3D-WEBUI/blob/3.0/src/targets/translations/en.json

Also it is not necessary to edit/change any preferences.json in https://github.com/luc-github/ESP3D-WEBUI/tree/3.0/server this is the simulator for the webui and the missing entries will be generated from : https://github.com/luc-github/ESP3D-WEBUI/blob/3.0/src/targets/Printer3D/preferences.json for all 3d Printers, https://github.com/luc-github/ESP3D-WEBUI/blob/3.0/src/targets/CNC/preferences.json for all CNC, etc... If specific command is necessary for specific FW then entries can be added to targeted FW, for example Marlin : https://github.com/luc-github/ESP3D-WEBUI/blob/3.0/src/targets/Printer3D/Marlin/preferences.json

When webUI start : 1 - it merge general and specific preferences.json to build the default one 2 - it read the preferences.json from flash if exists and update the default one in memory with user values, the default will be always regenerated at each restart so they cannot be deleted - just modified

so in case of keymap the delete button of each entry in settings wont work so better to remove it for keymap, but need support of NOP to disable keymap , another solution is we do not pre-populate any keymap in general preferences.json and let user populate all keymap from scratch both solution have pro and cons, any suggestion feedback on this ?

Edit: I did a simple POC to show what need to be modified to support basic Setting edition using text input - not selection list - I will do later if we have an agreement on POC and any of above proposal : https://github.com/luc-github/ESP3D-WEBUI/commit/35f1377d992031ff3389b34920a210b87002d59c keymap Be noted the storage format of the information is following current list API, so the keymap parsing need to be updated accordingly

aaronse commented 1 year ago

Thank you for info about which files to edit, and which are generated.

Thank you for putting together and sharing the POC already. Looks good to me. Expected to see Add button and Delete icons, guessing you'll need to make a bunch of edits in src\components\Controls\Fields\ItemsList.js to support. I wouldn't blame you if you prefer to abandon Key mapping UI and go back to simple text field like first pull request. Up to you! I just want to wrap up and submit pull request for another ESP3D-WEBUI feature after this :-)

I don't think User should have to add all Key Mappings from scratch, they won't be happy. Having a default set of Key Mappings is easier for the User.

I originally submitted the pull request with key REmapping functionality for a few keys the User wants to add/change/suppress. However, looks like you're thinking this can grow into being how all keys are mapped (there's no remapping, there is just mapping, so NOP not needed in this case)? I don't mind either way, whatever's fast, and good enough for now. Perfect choice isn't always to make perfect :-)

Be noted the storage format of the information is following current list API, so the keymap parsing need to be updated accordingly.

I appreciate the note and updated code to query key from object graph...

let iterKey = kv.value.filter(el => el.name == "key" )[0].value;
let iterId = kv.id;

Cheers!

luc-github commented 1 year ago

if all buttons/key are populated why do you need add/delete buttons in list?

as you see I did not modified a lot, now just need to populated buttons/key and translations

aaronse commented 1 year ago

Will list have all possible commands?

With subset of commands listed having keys assigned?

Guessing listing all commands would make page UI cluttered/messy? Is there many?

luc-github commented 1 year ago

well all commands listed in current shortkey help if it is in list it is fine IMHO

aaronse commented 1 year ago

If you're okay with item list having all commands. Then would be nice for User to type key into a text field. Then now or later on, things like Alt or Shift or Ctrl combos can be typed by User.

luc-github commented 1 year ago

yes sure or also extend to all ui.... everything is possible

I will work on help page today or tomorrow as well as conflict key management and populate all key/translation for 3dprinter jog , if all ok it can be done on cnc jog and plotter jog

aaronse commented 1 year ago

Sounds good. Thank you for jumping into this, appreciate it. Let me know if my #285 pull request needs edits? Cheers!

luc-github commented 1 year ago

thank you, I will let you know once I have finished the above changes

luc-github commented 1 year ago

Closing in favor of https://github.com/luc-github/ESP3D-WEBUI/pull/285