houmain / keymapper

A cross-platform context-aware key remapper.
GNU General Public License v3.0
257 stars 21 forks source link

Interpretation/handling of abstract commands feels unnecessarily strict #148

Open ristomatti opened 1 month ago

ristomatti commented 1 month ago

I believe the current handling of abstract commands is too strict and has some limitations that I've found a bit unexpected. This is about two separate issues, but I still decided to create only a single issue. I can split these into separate issues if that's preferred.

1. Unused abstract command makes the config invalid

In some situations this behavior is useful, but it can also create a jarring user experience. I think a warning would be more appropriate. This feels also inconsistent as an unused alias doesn't make the config invalid, even though they can sometimes be used for the same purpose.

There's two situations when I find this especially jarring. First example: when I start remapping a new app, I'd like to first write all the abstract commands I'm expecting to remap. Coming up with intuitive remappings some times takes some time so I might not do them all at once. I then have to comment out the commands and uncomment the commands, while I'd prefer them to be there when I need them.

The other type of situation I find this jarring is when some remap I've done turns out interfering with some other functionality while I'm working. I then usually just comment it out to be fixed later, but I then end up with an invalid config and have to go back and also comment out the abstract command.

2. Abstract commands cannot be combined with other output mappings

A couple of examples which illustrate how this can feel inconsistent or confusing.

Example 1:

✅ Base config

[class = "app1"]
AltLeft{F} >> Control{F}

[class = "app2"]
AltLeft{F} >> !AltLeft Control{F}

❌ Naive attempt to use abstract command, invalid

[class = "app1"]
AltLeft{F} >> find

[class = "app2"]
AltLeft{F} >> !AltLeft find

[default]
find >> Control{F}

✅ The above would be valid using an alias

find = Control{F}

[class = "app1"]
AltLeft{F} >> find

[class = "app2"]
AltLeft{F} >> !AltLeft find

Example 2:

✅ Debugging a config

log = $(echo "$1" >> ~/tmp/keymapper.log)

[class = "app1"]
AltLeft{T} >> log["app1 new tab"] Control{T}
AltLeft{F} >> find

[class = "app2"]
AltLeft{F} >> find

[default]
find >> Control{F}

❌ Further debugging, invalid config

log = $(echo "$1" >> ~/tmp/keymapper.log)

[class = "app1"]
AltLeft{T} >> log["app1 new tab"] Control{T}
AltLeft{F} >> log["app1 find"] find

[class = "app2"]
AltLeft{F} >> find

[default]
find >> Control{F}

✅ Same with an alias, valid

log = $(echo "$1" >> ~/tmp/keymapper.log)
find = Control{F}

[class = "app1"]
AltLeft{T} >> log["app1 new tab"] Control{T}
AltLeft{F} >> log["app1 find"] find

[class = "app2"]
AltLeft{F} >> find
houmain commented 1 month ago

Thanks for the suggestions.

  1. I think it is save to allow abstract commands without mappings. It will behave just as if all mappings were in non-active contexts. I will change that.

  2. Is not easy to change. While aliases are simply substituted when the configuration is loaded, abstract commands are mappings, which are resolved at runtime. I likely could allow some prefix to abstract command mappings, which would make your examples work, but I do not know when I am going to attempt that.

ristomatti commented 1 month ago

Number 2 is now crossed over. It sounds like it would have potential for breaking things. 🙂

houmain commented 1 month ago

I am also not entirely sure yet about 1. Not allowing unmapped abstract commands helps to spot typos like the following:

CapsLock >> BackSpace

The user likely wants to map the Backspace key instead of creating an unmapped command BackSpace.

ristomatti commented 1 month ago

Ah but I meant the other way around. To have abstract actions defined but not used in any mappings. 🙂

ristomatti commented 1 month ago

For instance, my use case is that I might have a set of abstract actions defined like below. Notice I have join_lines commented out as otherwise the config would not be valid. I removed the binding using it as I changed it to another action. I've still went through the trouble of adding the abstract action, so I want to keep it there for the time I come up with another binding.

[class = JetBrainsIDE]
  close_tab                 >> Alt{W}
  close_tool_window         >> Shift{Escape}

  find                      >> Control{F}
  find_in_files             >> (Control Shift){F}
  replace                   >> Control{R}
  replace_in_files          >> (Control Shift){R}

  show_actions              >> _Alt{A}
  show_context_actions      >> _Alt{Enter}
  show_hover_info           >> Control{F1}
  show_quick_documentation  >> Control{Q}

  toggle_terminal           >> ^ Alt{T}
  toggle_problems           >> ^ Alt{8}

  goto_file                 >> Alt{P}
  goto_symbol               >> Alt{S}

  goto_declaration          >> Control{B}
  goto_type_declaration     >> (Control Shift){B}
  goto_implementation       >> Control{Alt{B}}

  jump_to_text_start        >> Control{Home}
  jump_to_text_end          >> Control{End}
  jump_to_top               >> Control{PageUp}
  jump_to_bottom            >> Control{PageDown}
  jump_to_line              >> Control{G}
  jump_to_character         >> Alt{Comma}
  jump_to_word              >> Control{Comma}
  jump_to_matching_brace    >> Control{Shift{M}}
  jump_to_last_edit_loc     >> Alt{Shift{E}}

  next_error                >> !Escape _Alt{Escape}
  prev_error                >> !Escape _Alt{Shift{Escape}}

  next_function             >> _Alt{ArrowDown}
  prev_function             >> _Alt{ArrowUp}

  rollback_change           >> (Control Alt){Z}
  next_change               >> (Control Alt){ArrowDown}
  prev_change               >> (Control Alt){ArrowUp}
  next_difference           >> (Control Alt){ArrowDown}
  prev_difference           >> (Control Alt){ArrowUp}
  compare_next_file         >> (Alt Shift){N}
  compare_prev_file         >> (Alt Shift){J}

  extend_selection          >> _Alt{W}
  shrink_selection          >> _Alt{Shift{W}}

  cut_to_line_end           >> !AltGr (Shift Control Alt){Delete}
  delete_to_line_end        >> !AltGr (Control Alt){Delete}
  # join_lines                >> _Control{J}
  toggle_comment            >> !Escape _Control{7}
  toggle_block_comment      >> !Escape _Control{Shift{7}}
  accept_completion         >> ^ Tab
houmain commented 4 weeks ago

The other way round it would also have drawbacks. Allowing join_lines >> _Control{J} when join_lines was not defined would also make it impossible to detect typing errors like Caplock >> Backspace. There currently are no other warnings, only errors, which are reported as notifications, which have a very limited length.

One could use something like the following to temporarily disable a command:

OFF = Virtual99

OFF Meta{J} >> join_lines
join_lines >> _Control{J}
ristomatti commented 4 weeks ago

I see the issue now and agree it'd just make the situation worse. One way around this would be to enforce this recommendation from the documentation:

https://github.com/houmain/keymapper/blob/6b842481c936df8fef5a1b49f919c530f96e0d01/README.md?plain=1#L148

This could be behind a "strict mode" startup flag or introduced as a breaking change. What do you think? Personally, I doubt the convenience would be worth the trouble for this alone. However, enforcing lower case initial letter for abstract actions sounds like an improvement in itself. It would for example:

ristomatti commented 4 weeks ago

One could use something like the following to temporarily disable a command:

OFF = Virtual99

OFF Meta{J} >> join_lines
join_lines >> _Control{J}

Neat workaround. I personally would find that a bit cluttering as often the case has been that I want to use the keybinding for some other action. But using the same idea, I will definitely start doing this:

None = Virtual99

None >> join_lines
None >> toggle_comment

It can be a useful indicator of "TODO" items also. :+1:

ristomatti commented 2 weeks ago

@houmain Ok to close this?

houmain commented 2 weeks ago

I am thinking about adding directives some time. Something like:

@skip-devices /.*/
@grab-devices /Some Keyboard/
@include "filename.conf"

Then I can also add a directive for allowing not mapped identifiers or enforcing some naming conventions.

ristomatti commented 2 weeks ago

Great idea!