Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interpretation/handling of abstract commands feels unnecessarily strict #148

Open
ristomatti opened this issue May 18, 2024 · 11 comments
Open

Comments

@ristomatti
Copy link
Contributor

ristomatti commented May 18, 2024

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
@ristomatti ristomatti changed the title Interpretation/handling of abstract commands is too strict Interpretation/handling of abstract commands feels unnecessarily strict May 18, 2024
@houmain
Copy link
Owner

houmain commented May 26, 2024

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
Copy link
Contributor Author

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

@houmain
Copy link
Owner

houmain commented May 28, 2024

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
Copy link
Contributor Author

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

@ristomatti
Copy link
Contributor Author

ristomatti commented Jun 3, 2024

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
Copy link
Owner

houmain commented Jun 6, 2024

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
Copy link
Contributor Author

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:

To simplify mapping of one input expression to different output expressions, it can be mapped to an abstract command first. The command name can be chosen arbitrarily but must not be a key name. The configuration is case sensitive and all key names start with a capital letter, so it is advisable to begin command names with a lowercase letter:

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:

  • reduce the potential for confusion around the difference between macros and abstract actions
  • simplify implementing editor syntax highlighting rules

@ristomatti
Copy link
Contributor Author

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. 👍

@ristomatti
Copy link
Contributor Author

@houmain Ok to close this?

@houmain
Copy link
Owner

houmain commented Jun 17, 2024

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
Copy link
Contributor Author

Great idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants