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

Add option action_alias #4260

Closed
wants to merge 4 commits into from

Conversation

page-down
Copy link
Contributor

New option action_alias is added to define common actions.
Also restrict kitten_alias to apply only to kitten actions.


I found a type mismatch issue. The KeyAction object is added to the args during the parsing of the combine action.

class KeyAction(NamedTuple):
    func: str
    args: Tuple[Union[str, float, bool, int, None], ...] = ()

# options/utils.py
@func_with_args('combine')
def combine_parse(func: str, rest: str) -> FuncArgsType:
    # ...
    args = tuple(map(parse_key_action, filter(None, parts))) # parse_key_action() -> KeyAction
    return func, args

Should KeyAction be added to the Union type of KeyAction args?

@page-down
Copy link
Contributor Author

I left the kitten_alias implementation untouched because I need to replace the alias before the string is converted to a KeyAction, and parse the entire string with the alias replaced. Otherwise it would be discarded when converting to KeyAction because the parser detects insufficient parameters or a problem with the position.

There is a problem with aliases not taking effect within the combine action. I tried to resolve it in combine_parse (parse_key_action(resolve_action_alias(...))), but the global action aliases are not easy to pass into these parser functions and I need help. Thank you.

@kovidgoyal
Copy link
Owner

Make a separate PR for your conf formatting changes please.

@page-down page-down deleted the feat-action-alias branch November 22, 2021 14:44
@page-down
Copy link
Contributor Author

Thank you. I found that there seems to be a problem in validating the action.

kitty --config=NONE -o 'action_alias ac1 launch --type=tab' -o 'map f1 ac1 sh -c read' -o 'map f2 ac1 --title=debug2'
Ignoring unknown map action: ac1 sh -c read
Ignoring unknown map action: ac1 --title=debug2

@kovidgoyal
Copy link
Owner

should be fine now

@page-down
Copy link
Contributor Author

How to configure with the new action_alias for setting default parameters for kittens?
(This is one reason why I left kitten_alias untouched before.)
This feature is very convenient.

The following only works for the shortcuts configured afterwards, and not for the default ctrl+shift+e etc.

kitty --config=NONE -o 'kitten_alias hints hints --hints-offset=3' -o 'map f1 kitten hints'

Not related to action_alias:

kitty --config=NONE -o 'kitten_alias hints --invalid' -o 'map f1 kitten hints'
ModuleNotFoundError: No module named 'kittens.__invalid'

It would be more convenient to be able to configure the following as well.
Action with fixed position parameters. Validate when finalizing the alias resolution.

kitty --config=NONE -o 'action_alias ac1 change_font_size current' -o 'map f1 ac1 +2' -o 'map f2 ac1 -2'
TypeError: change_font_size() missing 1 required positional argument: 'amt'

Is it possible to do parameter validation when finalizing the alias resolution? Instead of throwing an exception on execution.
Users can also know that there is a configuration problem that needs to be fixed.

kitty --config=NONE -o 'action_alias ac1 toggle_marker --invalid' -o 'map f1 ac1'
TypeError: toggle_marker() missing 2 required positional arguments: 'spec' and 'flags'

Also I remember that if the option is configured incorrectly, an error message is displayed in the foreground (overlay), when the kitty config is finished parsing.

@kovidgoyal
Copy link
Owner

action_alias mykitten kitten hints --whatever
map f1 mykitten --extra

@page-down
Copy link
Contributor Author

page-down commented Nov 22, 2021

action_alias mykitten kitten hints --whatever
map f1 mykitten --extra

Yes, this is obviously working. So that means that kitten_alias hints hints --hints-offset=0 in previous versions modified all kitten hints parameters by default, including the default shortcuts. However, with the new action_alias, all kitten hints parameters will no longer be changed. This should be considered a breaking change?

kitty --config=NONE -o 'action_alias hints kitten hints --hints-offset=3'
# ctrl+shift+e

If this change has to be introduced, does the default value for kitten_alias in definition.py need to be removed? Is it possible to not include deprecated configuration items in the commented configuration files generated for new users?

I like the new action_alias approach, just need to reconfigure all kitten default shortcuts.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Nov 22, 2021 via email

@page-down
Copy link
Contributor Author

The same remains true if you use kitten_alias now.

The following issues may need to be addressed, latest master branch.

kitty --config=NONE -o 'kitten_alias hints hints --hints-offset=3'
RecursionError: maximum recursion depth exceeded while calling a Python object

If you use action_alias, yes you have to re-write.

I understand that action_alias myhints hints needs to be called with myhints as the action.

However, in the definition.py documentation, you mention that

Similarly, to alias kitten invocation::

action_alias hints kitten hints --hints-offset=0

How is this interpreted? Is it the same as kitten hints hints? According to the literal meaning, a new alias named hints is defined and called with map f1 hints.

What change? kitten_alias should continue to work as before

I fully understand your want backward compatibility, so the issues raised so far are to ensure that the current implementation is consistent with the previous behavior.

Since it is worthwhile to keep the previous features, it would be nice to have new ways of defining the previous behavior (set default parameters for kitten) without the need for the previous kitten_alias.

@page-down
Copy link
Contributor Author

page-down commented Nov 22, 2021

The action alias combine does not seem to be working properly.

kitty --config=NONE -o 'action_alias ac1 combine : send_text normal 1 : send_text normal 2' -o 'map f1 ac1'

combine : send_text normal 1 : send_text normal 2
combine() takes from 2 to 4 positional arguments but 9 were given

And this one below.

kitty --config=NONE -o 'action_alias ac1 change_font_size current' -o 'map f1 ac1 +2' -o 'map f2 ac1 -2'

change_font_size current +2
change_font_size() missing 1 required positional argument: 'amt'

Other than the above mentioned, everything seems to be working fine for now.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Nov 23, 2021 via email

@kovidgoyal
Copy link
Owner

kovidgoyal commented Nov 23, 2021 via email

@page-down
Copy link
Contributor Author

So combine and other actions are not included, at least for now.

For users with such niche needs, it is recommended to use templates or external tools such as sed to generate configuration files with string replacement.

I found that in class MultiOption: as_conf(); as_rst();, the long_text of the option is not affected by documented. This causes the long_text to be output alone.

It might be worthwhile to keep information about the deprecated option in the documentation so that users can be informed, yet not make this option available to new users (generated config).

Thank you for implementing and making this feature available.

@page-down
Copy link
Contributor Author

In the latest change, kitten_alias fails to apply to the default configured shortcuts.

kitty --config=NONE -o 'kitten_alias hints hints --hints-offset=3'

press ctrl+shift+p>w

It worked fine in earlier revisions.

Also error is reported when a non-existent consecutive shortcut key is pressed.

press ctrl+shift+p>p

"kitty/boss.py", line 1137, in combine
    if self.dispatch_action(actions[0], window_for_dispatch, dispatch_type):
IndexError: tuple index out of range

During handling of the above exception, another exception occurred:
"kitty/boss.py", line 1142, in combine
    self.show_error('Key action failed', f'{actions[0].pretty()}\n{e}')
IndexError: tuple index out of range

@kovidgoyal
Copy link
Owner

kovidgoyal commented Nov 23, 2021 via email

@page-down
Copy link
Contributor Author

... kitten_alias fails to apply to the default ...

Yes, that's by design. It's a performance optimization, ... I suppose I could reparse them only if aliases are defined.

I appreciate the good design. As long as the documentation is consistent with the behavior, it should be fine.

... Also error is reported when a non-existent consecutive shortcut key is pressed.

Fixed.

It works fine now.

Regarding the empty alias, is it possible to reset (remove the definition)?

kitty --config=NONE -o 'action_alias change_font_size change_font_size current' -o 'action_alias change_font_size' -o 'map f1 change_font_size all +2'

Currently it needs to be defined again using action_alias change_font_size change_font_size.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Nov 23, 2021 via email

@page-down
Copy link
Contributor Author

What would be the point of removing an alias?

Since alias will not take effect for the mapping configured before then, resetting to the original definition is still somewhat usable.

But now that re-parse has been implemented, it makes no sense at all.

Thank you for the improvements to action_alias, it now behaves the same as the previous kitten_alias.

@page-down
Copy link
Contributor Author

After the recent Parse actions on demand [0c274a9] update, I noticed that one of my mouse_map was acting abnormally.

This can be reproduced with the following command, related to combine, launch.

kitty --config=NONE -o 'map f1 combine : launch sh -c read : send_text normal : no_op'

Before the recent changes, everything was fine and one kitty window would be created. Now it opens three windows (with the wrong text if stdin-source + pager is used).

@kovidgoyal
Copy link
Owner

af7a104

@page-down
Copy link
Contributor Author

I found a very common scenario in daily use.
Both launch and program have the need to specify their parameters.

action_alias mypager launch --type os-window --stdin-source={action_args} less {program_args:-default_value_here}

I am not sure if there is a good solution. Can slightly cover this simple and efficient use case and is easy to maintain.
Allow users to define such action aliases to reduce configuration duplication.

action_alias should not be overly complex, and any additional parsing needs should be written in their own kitten.

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

Successfully merging this pull request may close these issues.

None yet

2 participants