Skip to content
This repository has been archived by the owner on May 28, 2022. It is now read-only.
This repository has been archived by the owner on May 28, 2022. It is now read-only.

basic_keys.keymap list fails to set in context #192

Open
dwighthouse opened this issue Oct 9, 2019 · 4 comments
Open

basic_keys.keymap list fails to set in context #192

dwighthouse opened this issue Oct 9, 2019 · 4 comments

Comments

@dwighthouse
Copy link

I'm working my way through the scripts here, learning what I've missed for the last few months since being away.

In misc/basic_keys.py, for reasons I don't fully understand, the context is loaded with the list of keys from the keymap dictionary, which contains all the other defined dictionary keys save modifier_keys.

https://github.com/dwiel/talon_community/blob/master/misc/basic_keys.py#L140

This results in a failure messages in the console:

2019-10-08 17:51:39    IO cmd {'success': False, 'error': 'error setting list', 'cmd': {'name': 'talon', 'list': 'basic_keys.keymap', 'items': 100, 'cmd': 'g.listset'}, 'ts': 64395175723, 'nonce': 250}

This message seems to repeat every time the file is touched or sometimes just when switching applications (even if neither application is the code editor).

This is curious, as the code of basic_keys.py already has lists for every other meaningful key, as the keymap list is simply the combination of the others. The only use case for keymap as a context list seems to be as one of the selection groups in the get_keys method (where it is redundant).


It appears that the problem has something to do with the fact that context's keymap keys do not have a single instance where the basic_keys.keymap list is used.

Adding the following entry will fix the problem:

{
    ...
    "{basic_keys.modifiers}* {basic_keys.keymap}+": press_keys,
    ...
}

Or it can be fixed by removing the references to the basic_keys.keymap list on lines 94 and 140.

Since the basic_keys.keymap list is just the combination of other lists, it might even be better to remove their lists, and instead use the above entry in place of lines 126-127. This would result in almost identical functionality, except that it would allow the arrow key words (left, right, up, and down) to be used for the arrow keys without any modifiers.

Please explain the reasoning I may have missed behind this decision, and suggest which if any of these possible solutions should be pursued? I can make a PR with the chosen solution if desired.

@dwighthouse
Copy link
Author

Meant to check the Slack before posting, but it looks like Jim on Slack found this issue back on September 20th ( #help room ). It doesn't appear to be a Talon-specific issue, and it certainly doesn't harm recognition of other things (only things of that particular list, which in this case is redundant anyway). So, I think any method to reduce unnecessary warnings in the console would be good.

@dwiel
Copy link
Owner

dwiel commented Oct 29, 2019

I apologize for only getting to this now. Is this still an issue? I am not sure i understand what the problem is though. If the issue is that arrow keywords should be available without any modifiers, I think we should make this an option. It's likely that it set up the way it is purposefully. Having the word up in the global context can cause lots of false positives. Maybe we can set it up to be optionally available in the global context?

@dwighthouse
Copy link
Author

@dwiel I agree that it's appropriate not to have standard arrow direction words being usable with no modifiers. I would prefer to remove the references to basic_keys.keymap on lines 94 and 140. This would also solve the issue and keep functionality the same. I just wasn't sure the intent. Sounds like your intent is the same as mine, so the removal option is best.

@dwiel
Copy link
Owner

dwiel commented Oct 30, 2019

Cool, sounds good to me.

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

No branches or pull requests

2 participants