[key bindings] Add dynamic "$event.[something]" mini-language to our key bindings management #543

olivierphi · 2022-05-26T10:49:32Z

With this PR we can now use a "mini-language" to inject event-related data into the key bindings' handlers
e.g. self.bind("1,2,3", "digit_pressed('$event.key')")

My first implementation was using the Python "Format Specification Mini-Language", so the syntax was the following:
```
 self.bind("1,2,3", "digit_pressed('{event.key}')") # <-- uses curly brackets for interpolation
```
But as I was writing some integration tests with various kinds of Python primitives passed to the key bindings handlers I realised that using this syntax was causing an issue: as it's based on curly brackets we could not use Python dicts or sets in the parameters we inject.
This is why I ended up opting for a dedicated SDL that doesn't conflict with Python syntax for dicts or set literals. As I don't want to create fancy SDLs out of thin air that users have to learn, I simply based it on the Python Template strings' one.
So our previous expression becomes this:
```
 self.bind("1,2,3", "digit_pressed('$event.key')") # <-- uses a "dollar" syntax for interpolation

 # ...which enables this kind of things:
 self.bind("1,2,3", "digit_pressed('$event.key', {'sender_name': 'sidebar'})") # <-- injects a Python dict
```

Limits of this "action binding mini-language"

As explained in one of my comments on the GitHub diff, I also realised that this "action binding mini-language" has some strong limits, which can be a good thing for security but also has limits.
We can pass $event.key to the action method for example, but we cannot pass the instance of the Event itself, or the sender instance - because this SDL is based on ast.literal_eval, which intentionally doesn't allow using custom objects.

I guess that time and dog-fooding will tell us if expressions such as $event.sender (which is the string representation of the sender, rather than the sender instance) is enough, or if we should rather start thinking of something else? 🙂

Potential future improvements: dependency injection?

One potential way to solve this would be to use "a la Pytest" / "a la FastAPI" dependency injection...
So if the action method declares a param named event for example we detect that and inject the Event instance for this parameter. Same with a param named sender, etc. 🙂

Dependency injection would also allow users to not use error-prone "Python-syntax-in-a-string expressions" - i.e. it's easy to miss the syntax error in something like this:

self.bind("1,2,3", "digit_pressed('$event.key', {'sender_name: 'sidebar'})")

(a closing single quote is missing)

closes #496

olivierphi · 2022-05-26T10:50:39Z

sandbox/scroll_to_widget.py

+        scroll_to_placeholders_keys_to_bind = ",".join(
+            [str(i) for i in range(placeholders_count)]
+        )
+        self.bind(


Practical use of this new feature: rather than calling self.bind() 10 times, we can now call it once and have the $event.key injected into the handler

olivierphi · 2022-05-26T10:54:01Z

src/textual/_callback.py

-    """Count the number of parameters in a callable"""
-    return len(signature(func).parameters)
+def count_parameters(func: Callable) -> tuple[int, bool]:
+    """Count the number of parameters in a callable, and checks if the last positional one is a "capture all"


I made a slight change to this function because of an edge case: if the parameters are (1, 'a', True) for example, and the function uses *arg to capture all the positional arguments, we were seeing only one arg and injected only the first one (1 is this example).

With this update we use the already instantiated Signature object to also determine if the callable is using such a "capture all" argument, which will allow callers of this function to inject 1, 'a' and True to the callable if that's the case.

olivierphi · 2022-05-26T11:00:59Z

src/textual/actions.py

@@ -32,6 +42,21 @@ def parse(action: str) -> tuple[str, tuple[Any, ...]]:
    )


+def _process_dynamic_event_params(action_params_str: str, event: Event) -> str:


It works but it is definitely a bit hacky to be honest - and also limited, as the only things we can inject with this mini language are scalar values.

On one hand it certainly limits the potential security issues of this feature (if the keybinding is taken from a database for example, and contains sequences such as $event.__class__.os.environ to expose the environment variables the app is running with )

But on the other hand it prevents passing the Event instance itself for example, which I guess could be handy if we wanted to do checks such as this in the event handler:

def action_on_digit(self, key:str, sender): if sender is left_panel_widget: # <-- not possible, since we cannot pass objects with this syntax do_something()

We can always inject is the string representation of the sender's __class__ with this mini-language, but it's more limited.

Ah, the toughness of trade-offs... 😅 ¯_(ツ)_/¯

I am fine with it being limited to scalars. For precisely the reason you gave. These bindings could come from external sources, and we don't want to expose or run any code.

olivierphi · 2022-05-26T11:02:10Z

src/textual/app.py

@@ -931,21 +931,20 @@ def bell(self) -> None:
        """Play the console 'bell'."""
        self.console.bell()

-    async def press(self, key: str) -> bool:
+    async def press(self, event: events.Key) -> bool:


the action method now needs to be able to optionally receive the event, so we can process the $event.[something] placeholders of the binding expression

Not keen on this. The name no longer makes sense. You press a key, but you don't press an event.

Could you construct the events.Key within this method?

olivierphi · 2022-05-26T11:03:29Z

tests/utilities/test_app.py

@@ -25,13 +25,15 @@

 _SYNC_START_SEQUENCE = TERMINAL_MODES_ANSI_SEQUENCES["sync_start"]

+_DEFAULT_SIZE = Size(20, 10)


for some tests, such as this key bindings new one, we really don't care about the size of the app 🙂 - let's make this size optional!

olivierphi · 2022-05-26T11:05:42Z

tests/test_integration_key_binding.py

+            await wait_for_key_processing()
+            assert app.key_binding_result == (
+                (
+                    "Ellipsis",


All the $event.[something] dynamic parameters are scalar-ified, so for example,:

the Python ellipsis becomes "Ellipsis"

$event.__class__.__mro__ becomes the string representation of the event class' MRO

willmcgugan

A few changes as discussed:

White list the allows substitutions within the event object. Attributes must be simple atomic types (which can be parsed with literal_eval), raise an error if not.
Do the substitution with the repr of the object. This will limit what could be done, but reduce the possibility of breaking the action

willmcgugan · 2022-05-26T12:14:16Z

src/textual/_callback.py

-def count_parameters(func: Callable) -> int:
-    """Count the number of parameters in a callable"""
-    return len(signature(func).parameters)
+def count_parameters(func: Callable) -> tuple[int, bool]:


The change to the functionality as rendered the name inaccurate, since its does more than count. Is there a better name?

willmcgugan · 2022-05-26T12:19:36Z

src/textual/actions.py


 class ActionError(Exception):
    pass


 re_action_params = re.compile(r"([\w\.]+)(\(.*?\))")
+# We use our own "mini language" for dynamic event properties injections:
+# e.g. `'$event.key'` will inject the pressed key, `$event.sender` the string representation of the sender, etc.
+re_injected_event = re.compile(r"\$(event[\w.]+)")


We probably shouldn't restrict ourselves to just event. Maybe that all we ever user, but it seems an arbitrary restriction.

willmcgugan · 2022-05-26T12:27:54Z

src/textual/actions.py

@@ -32,6 +42,21 @@ def parse(action: str) -> tuple[str, tuple[Any, ...]]:
    )


+def _process_dynamic_event_params(action_params_str: str, event: Event) -> str:


I am fine with it being limited to scalars. For precisely the reason you gave. These bindings could come from external sources, and we don't want to expose or run any code.

willmcgugan · 2022-05-26T12:31:15Z

src/textual/app.py

@@ -931,21 +931,20 @@ def bell(self) -> None:
        """Play the console 'bell'."""
        self.console.bell()

-    async def press(self, key: str) -> bool:
+    async def press(self, event: events.Key) -> bool:


Not keen on this. The name no longer makes sense. You press a key, but you don't press an event.

Could you construct the events.Key within this method?

willmcgugan · 2022-05-26T12:59:15Z

src/textual/actions.py

@@ -32,6 +42,21 @@ def parse(action: str) -> tuple[str, tuple[Any, ...]]:
    )


+def _process_dynamic_event_params(action_params_str: str, event: Event) -> str:
+    for dynamic_event_match in re.finditer(re_injected_event, action_params_str):


I have a feeling this can be done with a single re.sub and without using format.

olivierphi · 2022-05-26T16:22:17Z

@willmcgugan As discussed I updated the code for a new "action bindings mini-language" machinery: 9b12010 🙂

Wrapping dynamic expressions in quotes or not wrapping them? 🤔

My first shot didn't require the use of quotes around dynamic parameters, as we said, so one could simply use an expression such as toggle($event.key).
However, I realised that doing so makes it very difficult to also support compound Python data structures into these expressions.
e.g. if we wanted to support expressions such as toggle($event.key, ["sidebar", "topnav"]) we would then have to implement our own params parser, that would run before ast.literal_eval.
And naively splitting the string with commas to get a list of params wouldn't do the job, because of potential commas in the param values themselves - ["sidebar", "topnav"] is a single parameter for example.

But if instead of this we still wrap dynamic expressions in quotes we can then let ast.literal_eval do this params parsing for us, and then simply post-process params that are strings which start with $.
This makes our own code very readable, simple to maintain, and doesn't need the summoning of regexes.

It's a trade-off, for sure, and I would have preferred to be able to use $event.key without quotes around it.
But to me the benefit of having this quotes wrapping and be able to off-load the hard parsing to ast.literal_eval is worth the annoyance of quotes 🙂 (which we have to use for scalar string values in these expressions anyway)

Classes can opt-in to allow their attributes use in dynamic action expressions

Regarding the explicit "opt-in" of classes to their attributes in such expressions...

In order to avoid potentially offensive terminologies I opted for __allowed_attributes_for_actions__ (verbose but explicit ^^) rather than "whitelist". Too verbose maybe? 😅

I added (quite a big) docstring to the actions.parse function, which hopefully will ease its maintenance in the long term 🙂

textual/src/textual/actions.py

Lines 37 to 73 in 9b12010

Parse an action string into a tuple of the action name and the action parameters.

Any parameter that is a string starting with a dollar sign is considered a dynamic placeholder,

and will be replaced with the data source value for that key.

For example, the parameter string `"$event.key"` will be parsed into `"c"` if the dynamic data sources

dict has an "event" key, for which the value is a class that explicitly allows the usage of its "key"

attribute by actions parameters - and the value of its "key" attribute is the string "c".

Such explicit attribute access is needed for security reasons, to avoid exposing sensitive data in the

action.

To enable such explicit attribute access, you can add a `__allowed_attributes_for_actions__` attribute

on the object.

For example, an Event class with the following line in its class definition:

```__allowed_attributes_for_actions__ = ("key", "sender)```

...explicitly allows the use of its "key" and "sender" attributes by actions parameters.

Only one level of dynamic attribute access is allowed, so `"$event.sender.is_system"` is never allowed

even if the "sender" explicitly allow the usage of its "is_system" attribute.

In order to allow direct access to a direct value of dynamic data sources dict, we can add the

string "Self" to the list of allowed attributes.

For example, an Event class with the following line in its class definition:

```__allowed_attributes_for_actions__ = ("Self", "key", "sender)```

...explicitly allows the use of the expression `$event` actions parameters if an instance of this class

is passed as the "event" key of the dynamic data sources dict.

Args:

action (str): an action expression - e.g. `on_mount("login", "$event.key")`

dynamic_data_sources (dict | None): a mapping of dynamic data sources to use in the action expression.

Returns:

tuple[str, tuple[Any, ...]]: the action name and the action parameters, processed according to the

dynamic data sources and dynamic parameters pattern.

Raises:

ActionError: if the action expression is invalid

NotAllowedActionExpression:, if dynamic attributes resolution rules prevent access to an attribute

I also chose to allow classes to opt-in for their direct own instances use in action expressions, by adding the string "Self" in this __allowed_attributes_for_actions__ sequence (inspired by the new typing.Self in Python 3.11).
So if the Param class defines only key in this sequence, $param.key is allowed but $param is not. But if this sequence contains "Self" we can then use $param, and the instance of the Param class will be injected into the handler.
I've added this "Self" value in the __allowed_attributes_for_actions__ sequence of our event.Events class (and all its subclasses), so people can use $event in their key bindings expressions.
But if you think we'd better not too, I'll remove "Self" from there 🙂

olivierphi · 2022-05-27T14:39:13Z

@willmcgugan if you want to take a look at my last iteration on this PR, so I have a chance to address your comments before I'm off for the next 2 weeks... 🙂 🏖️ --> #543 (comment)

willmcgugan · 2022-05-27T14:43:34Z

@drbenton I think I might have to defer this one for later. Needs a bit of a deep dive. No worries though, it can wait until after your break!

…key bindings management

olivierphi · 2022-06-14T08:35:56Z

@willmcgugan branch is now rebased, ready for your input regarding #543 (comment) 🙂

willmcgugan · 2022-06-14T09:40:27Z

Going to put this one on hold for now.

There probably is a requirement for this, but the current solution of substitution and then parsing with ast.parse_literal could introduce new edge cases and might require escaping. If we do it, we will probably have to implement our own literal parser that understands the variable substitutions more explicitly.

willmcgugan · 2022-06-14T09:43:51Z

It's not as hard as it may seem to implement our own parse_literal. Should be doable with the pyparsing lib.

olivierphi commented May 26, 2022

View reviewed changes

olivierphi marked this pull request as ready for review May 26, 2022 11:28

olivierphi requested a review from willmcgugan May 26, 2022 11:28

willmcgugan requested changes May 26, 2022

View reviewed changes

olivierphi force-pushed the events-can-be-passed-to-bindings branch from 1ee1b53 to 9b12010 Compare May 26, 2022 16:05

darrenburns mentioned this pull request May 27, 2022

Widget focus keybindings #546

Merged

olivierphi requested a review from willmcgugan May 27, 2022 14:25

Olivier Philippon added 2 commits June 14, 2022 09:30

[key bindings] Add dynamic "$event.[something]" mini-language to our …

d82d1b3

…key bindings management

[key bindings] Address "dynamic action params mini-language" PR feedback

a9691e4

olivierphi force-pushed the events-can-be-passed-to-bindings branch from 9b12010 to a9691e4 Compare June 14, 2022 08:30

willmcgugan closed this Jun 14, 2022

willmcgugan deleted the events-can-be-passed-to-bindings branch February 6, 2023 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[key bindings] Add dynamic "$event.[something]" mini-language to our key bindings management #543

[key bindings] Add dynamic "$event.[something]" mini-language to our key bindings management #543

olivierphi commented May 26, 2022 •

edited

Loading

olivierphi May 26, 2022

olivierphi May 26, 2022

olivierphi May 26, 2022

willmcgugan May 26, 2022

olivierphi May 26, 2022

willmcgugan May 26, 2022

olivierphi May 26, 2022

olivierphi May 26, 2022

willmcgugan left a comment

willmcgugan May 26, 2022

willmcgugan May 26, 2022

willmcgugan May 26, 2022

willmcgugan May 26, 2022

willmcgugan May 26, 2022

olivierphi commented May 26, 2022 •

edited

Loading

olivierphi commented May 27, 2022

willmcgugan commented May 27, 2022

olivierphi commented Jun 14, 2022

willmcgugan commented Jun 14, 2022

willmcgugan commented Jun 14, 2022

		@@ -32,6 +42,21 @@ def parse(action: str) -> tuple[str, tuple[Any, ...]]:
		)


		def _process_dynamic_event_params(action_params_str: str, event: Event) -> str:

		@@ -25,13 +25,15 @@

		_SYNC_START_SEQUENCE = TERMINAL_MODES_ANSI_SEQUENCES["sync_start"]

		_DEFAULT_SIZE = Size(20, 10)

[key bindings] Add dynamic "$event.[something]" mini-language to our key bindings management #543

[key bindings] Add dynamic "$event.[something]" mini-language to our key bindings management #543

Conversation

olivierphi commented May 26, 2022 • edited Loading

Limits of this "action binding mini-language"

Potential future improvements: dependency injection?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willmcgugan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivierphi commented May 26, 2022 • edited Loading

Wrapping dynamic expressions in quotes or not wrapping them? 🤔

Classes can opt-in to allow their attributes use in dynamic action expressions

olivierphi commented May 27, 2022

willmcgugan commented May 27, 2022

olivierphi commented Jun 14, 2022

willmcgugan commented Jun 14, 2022

willmcgugan commented Jun 14, 2022

olivierphi commented May 26, 2022 •

edited

Loading

olivierphi commented May 26, 2022 •

edited

Loading