As of 0.6.0, bindings on (at least) left, right, up and down no longer appear to work
#1343
Comments
|
This seems to be related to the
That is, I get no Now, if I mark the arrow keys as
That is: they do fire via |
|
In fact, if I make all of the keys in the example code I'm unsure at the moment if this is expected behaviour, and I'm unsure what would have happened here with 0.5.0 (I'll check), but there's a difference in how the arrow keys (at least, I've not found any other affected keys) work between 0.5.0 and 0.6.0. The only (documented) change to gets between 0.5.0 and 0.6.0 is how |
|
Looks like I've found the culprit. It does seem to be an unintended consequence of the change to how bindings are inherited. Consider this version of the test code: from textual.app import App, ComposeResult
from textual.widgets import TextLog, Header, Footer
from textual.binding import Binding
from textual.events import Key
class Binder( App[ None ] ):
CSS = """
TextLog {
background: #222200;
color: #BBBB00;
text-style: bold;
}
"""
BINDINGS = [
Binding( "up", "log( 'up' )", "Up" ),
Binding( "down", "log( 'down' )", "Down" ),
Binding( "left", "log( 'left' )", "Left" ),
Binding( "right", "log( 'right' )", "Right" ),
Binding( "home", "log( 'home' )", "Home"),
Binding( "end", "log( 'end' )", "End"),
Binding( "pageup", "log( 'page_up' )", "Page Up"),
Binding( "pagedown", "log( 'page_down' )", "Page Down"),
Binding( "a", "log( 'a' )", "a" ),
Binding( "s", "log( 's' )", "s" ),
Binding( "d", "log( 'd' )", "d" ),
Binding( "f", "log( 'f' )", "f" ),
Binding( "comma", "log( 'comma' )", "," ),
Binding( "f1", "log( 'f1' )", "F1" ),
]
def compose( self ) -> ComposeResult:
yield Header()
yield TextLog()
yield Footer()
def on_mount( self ) -> None:
self.query_one( TextLog ).write( "Ready..." )
self.query_one( TextLog ).write( "Key bindings being looked for:" )
self.query_one( TextLog ).write( ", ".join(
[ binding.key for binding in self.BINDINGS ]
) )
def action_log( self, name: str ) -> None:
self.query_one( TextLog ).write( f"Via binding: {name}" )
def on_key( self, event: Key ) -> None:
self.query_one( TextLog ).write( f"Via event: {event!r}" )
if __name__ == "__main__":
Binder().run()Now consider this code in Moreover, I would have expected a child class (in this case my own More digging needed. |
|
Just to confirm that this is a difference from 0.5.0, here's the output of the above code, pressing the arrow keys (note the keys being looked for aren't printed here as I commented that out for a moment):
|
|
So it looks like it's one or more changes in #1170 (the code that adds the new |
|
Had a wee chat with @willmcgugan to confirm the intentions of the change in #1170 and was happy that the intended working made sense. So to dive a bit deeper I made the following change to Textual itself: diff --git a/src/textual/app.py b/src/textual/app.py
index 19e7b4c1..efa5fd70 100644
--- a/src/textual/app.py
+++ b/src/textual/app.py
@@ -1741,8 +1741,10 @@ class App(Generic[ReturnType], DOMNode):
"""
for namespace, bindings in self._binding_chain:
+ self.log.debug( f"{'' if universal else 'NON-'}UNIVERSAL BINDING CHECK - {key} - {namespace} - {bindings}" )
binding = bindings.keys.get(key)
if binding is not None and binding.universal == universal:
+ self.log.debug( f"HIT {binding}" )
await self.action(binding.action, default_namespace=namespace)
return True
return FalseRunning with In this instance at least, it would appear that the default |
|
Actually, that may not be it and there may be something else afoot. Having created what looked like a failure mode above, I then moved to attempting to have the problem happen when it's all happening on a widget (as that's the case in my own app that first showed this problem on upgrade to 0.6.0): from textual.app import App, ComposeResult
from textual.widgets import TextLog, Header, Footer
from textual.binding import Binding
from textual.events import Key
class MyLog( TextLog ):
BINDINGS = [
Binding( "up", "log( 'up' )", "Up" ),
Binding( "down", "log( 'down' )", "Down" ),
Binding( "left", "log( 'left' )", "Left" ),
Binding( "right", "log( 'right' )", "Right" ),
Binding( "home", "log( 'home' )", "Home"),
Binding( "end", "log( 'end' )", "End"),
Binding( "pageup", "log( 'page_up' )", "Page Up"),
Binding( "pagedown", "log( 'page_down' )", "Page Down")
]
def on_mount( self ) -> None:
self.write( "Ready..." )
def action_log( self, name: str ) -> None:
self.write( f"Via binding: {name}" )
def on_key( self, event: Key ) -> None:
self.write( f"Via event: {event!r}" )
class Binder( App[ None ] ):
CSS = """
MyLog {
background: #222200;
color: #BBBB00;
text-style: bold;
}
"""
def compose( self ) -> ComposeResult:
yield Header()
yield MyLog()
yield Footer()
def on_mount( self ) -> None:
self.query_one( MyLog ).focus()
if __name__ == "__main__":
Binder().run()but... that works fine!
My suspicion now is that widgets might be affected if there's a non-default screen at play (which is the case in my app). So that's the next test. |
|
Nope. That works fine too: from textual.app import App, ComposeResult
from textual.screen import Screen
from textual.widgets import TextLog, Header, Footer
from textual.binding import Binding
from textual.events import Key
class MyLog( TextLog ):
BINDINGS = [
Binding( "up", "log( 'up' )", "Up" ),
Binding( "down", "log( 'down' )", "Down" ),
Binding( "left", "log( 'left' )", "Left" ),
Binding( "right", "log( 'right' )", "Right" ),
Binding( "home", "log( 'home' )", "Home"),
Binding( "end", "log( 'end' )", "End"),
Binding( "pageup", "log( 'page_up' )", "Page Up"),
Binding( "pagedown", "log( 'page_down' )", "Page Down")
]
def on_mount( self ) -> None:
self.write( "Ready..." )
def action_log( self, name: str ) -> None:
self.write( f"Via binding: {name}" )
def on_key( self, event: Key ) -> None:
self.write( f"Via event: {event!r}" )
class BinderScreen( Screen ):
def compose( self ) -> ComposeResult:
yield Header()
yield MyLog()
yield Footer()
def on_mount( self ) -> None:
self.query_one( MyLog ).focus()
class Binder( App[ None ] ):
CSS = """
MyLog {
background: #222200;
color: #BBBB00;
text-style: bold;
}
"""
SCREENS = {
"main": BinderScreen
}
def on_mount( self ) -> None:
self.push_screen( "main" )
if __name__ == "__main__":
Binder().run()Time to go back to my app and see what I'm doing that's different, that worked fine with 0.5.0 but stopped working with 0.6.0. |
|
What I'm doing different.... the bindings and actions are on the screen. |
|
Bingo! from textual.app import App, ComposeResult
from textual.screen import Screen
from textual.widgets import TextLog, Header, Footer
from textual.binding import Binding
from textual.events import Key
class MyLog( TextLog ):
pass
class BinderScreen( Screen ):
BINDINGS = [
Binding( "up", "log( 'up' )", "Up" ),
Binding( "down", "log( 'down' )", "Down" ),
Binding( "left", "log( 'left' )", "Left" ),
Binding( "right", "log( 'right' )", "Right" ),
Binding( "home", "log( 'home' )", "Home"),
Binding( "end", "log( 'end' )", "End"),
Binding( "pageup", "log( 'page_up' )", "Page Up"),
Binding( "pagedown", "log( 'page_down' )", "Page Down")
]
def action_log( self, name: str ) -> None:
self.query_one( MyLog ).write( f"Via binding: {name}" )
def on_key( self, event: Key ) -> None:
self.query_one( MyLog ).write( f"Via event: {event!r}" )
def compose( self ) -> ComposeResult:
yield Header()
yield MyLog()
yield Footer()
def on_mount( self ) -> None:
self.query_one( MyLog ).write( "Ready..." )
self.query_one( MyLog ).focus()
class Binder( App[ None ] ):
CSS = """
MyLog {
background: #222200;
color: #BBBB00;
text-style: bold;
}
"""
SCREENS = {
"main": BinderScreen
}
def on_mount( self ) -> None:
self.push_screen( "main" )
if __name__ == "__main__":
Binder().run()
So... it looks like the new key binding inheritance approach has an unintended consequence when it comes to movement key bindings on classes that inherit from |
|
Okay, narrowing in more, take this code for example (similar to the above, just sans the inherited from textual.app import App, ComposeResult
from textual.screen import Screen
from textual.widgets import TextLog, Header, Footer
from textual.binding import Binding
from textual.events import Key
class BinderScreen( Screen ):
BINDINGS = [
Binding( "up", "log( 'up' )", "Up" ),
Binding( "down", "log( 'down' )", "Down" ),
Binding( "left", "log( 'left' )", "Left" ),
Binding( "right", "log( 'right' )", "Right" ),
Binding( "home", "log( 'home' )", "Home"),
Binding( "end", "log( 'end' )", "End"),
Binding( "pageup", "log( 'page_up' )", "Page Up"),
Binding( "pagedown", "log( 'page_down' )", "Page Down")
]
def action_log( self, name: str ) -> None:
self.query_one( TextLog ).write( f"Via binding: {name}" )
def on_key( self, event: Key ) -> None:
self.query_one( TextLog ).write( f"Via event: {event!r}" )
def compose( self ) -> ComposeResult:
yield Header()
yield TextLog()
yield Footer()
def on_mount( self ) -> None:
self.query_one( TextLog ).write( "Ready..." )
self.query_one( TextLog ).focus()
class Binder( App[ None ] ):
CSS = """
TextLog {
background: #222200;
color: #BBBB00;
text-style: bold;
}
"""
SCREENS = {
"main": BinderScreen
}
def on_mount( self ) -> None:
self.push_screen( "main" )
if __name__ == "__main__":
Binder().run()This still has the same issue. I think it boils down to this: there is a [16:35:36] DEBUG app.py:1744
NON-UNIVERSAL BINDING CHECK - down - TextLog(pseudo_classes={'focus-within', 'focus'}) - Bindings({'up': Binding(key='up', action='scroll_up',
description='Scroll Up', show=False, key_display=None, universal=False), 'down': Binding(key='down', action='scroll_down', description='Scroll Down',
show=False, key_display=None, universal=False), 'left': Binding(key='left', action='scroll_left', description='Scroll Up', show=False, key_display=None,
universal=False), 'right': Binding(key='right', action='scroll_right', description='Scroll Right', show=False, key_display=None, universal=False), 'home':
Binding(key='home', action='scroll_home', description='Scroll Home', show=False, key_display=None, universal=False), 'end': Binding(key='end',
action='scroll_end', description='Scroll End', show=False, key_display=None, universal=False), 'pageup': Binding(key='pageup', action='page_up',
description='Page Up', show=False, key_display=None, universal=False), 'pagedown': Binding(key='pagedown', action='page_down', description='Page Down',
show=False, key_display=None, universal=False)})
[16:35:36] DEBUG app.py:1747
HIT Binding(key='down', action='scroll_down', description='Scroll Down', show=False, key_display=None, universal=False)the binding handling code is finding a hit for that key in |
|
Taking it back to my own app. There's a widget called Bindings(
{
'up': Binding(key='up', action='scroll_up', description='Scroll Up', show=False, key_display=None, universal=False),
'down': Binding(key='down', action='scroll_down', description='Scroll Down', show=False, key_display=None, universal=False),
'left': Binding(key='left', action='scroll_left', description='Scroll Up', show=False, key_display=None, universal=False),
'right': Binding(key='right', action='scroll_right', description='Scroll Right', show=False, key_display=None, universal=False),
'home': Binding(key='home', action='scroll_home', description='Scroll Home', show=False, key_display=None, universal=False),
'end': Binding(key='end', action='scroll_end', description='Scroll End', show=False, key_display=None, universal=False),
'pageup': Binding(key='pageup', action='page_up', description='Page Up', show=False, key_display=None, universal=False),
'pagedown': Binding(key='pagedown', action='page_down', description='Page Down', show=False, key_display=None, universal=False),
'minus': Binding(key='minus', action='done( -1 )', description='Less Done', show=True, key_display='-', universal=False),
'backspace': Binding(key='backspace', action='done( -1 )', description='Less Done', show=True, key_display='-', universal=False),
'equals_sign': Binding(key='equals_sign', action='done( 1 )', description='More Done', show=True, key_display='=', universal=False),
'space': Binding(key='space', action='done( 1 )', description='More Done', show=True, key_display='=', universal=False)
}
)In other words we can see that it has inherited a whole bunch of bindings that we didn't intend for it to inherit, which would potentially rob our main screen of hits on the related actions. So... using the diff --git a/oidia/widgets/streakline.py b/oidia/widgets/streakline.py
index 3611ca6..bcd1bcb 100644
--- a/oidia/widgets/streakline.py
+++ b/oidia/widgets/streakline.py
@@ -23,7 +23,7 @@ from .timeline import TimelineTitle, TimelineDay, Timeline
from .title_input import TitleInput
##############################################################################
-class StreakDay( TimelineDay, can_focus=True ):
+class StreakDay( TimelineDay, can_focus=True, inherit_bindings=False ):
"""Widget for tracking if a day is done or not."""
DEFAULT_CSS = """
@@ -77,6 +77,7 @@ class StreakDay( TimelineDay, can_focus=True ):
def on_mount( self ) -> None:
"""Force an initial refresh on mount."""
self.action_done( 0 )
+ self.log.debug( self._bindings )
def watch_done( self, new_done: int ) -> None:
"""React to changes in the done count.we then see this: Bindings(
{
'minus': Binding(key='minus', action='done( -1 )', description='Less Done', show=True, key_display='-', universal=False),
'backspace': Binding(key='backspace', action='done( -1 )', description='Less Done', show=True, key_display='-', universal=False),
'equals_sign': Binding(key='equals_sign', action='done( 1 )', description='More Done', show=True, key_display='=', universal=False),
'space': Binding(key='space', action='done( 1 )', description='More Done', show=True, key_display='=', universal=False)
}
)this shows that setting To make this work I'd have to set As such, and as agreed in conversation with @willmcgugan, I think a bit of a rethink of how the movement bindings are done is in order. Having them at play from |
Up until now there doesn't seem to have been any unit tests aimed squarely at setting up bindings, as part of a running application, which are only about testing the bindings. As such there was no way of slotting in tests for how inheritance of bindings works. This starts that process with a view to testing how inheriting likely *should* work. See Textualize#1343 for some background to this.
|
Started adding some unit tests to reproduce the issue in a controlled environment, along with some control tests that are fine at the moment and should remain fine throughout any changes I make (more tests to come). Having done so, having a quick look at the code, especially @property
def is_container(self) -> bool:
"""Check if this widget is a container (contains other widgets).
Returns:
bool: True if this widget is a container.
"""
return self.styles.layout is not None or bool(self.children)
@property
def is_scrollable(self) -> bool:
"""Check if this Widget may be scrolled.
Returns:
bool: True if this widget may be scrolled.
"""
return self.styles.layout is not None or bool(self.children)The latter looks to be somewhat important to decisions made elsewhere in the code, and could possibly be something that can deliver a more sensitive approach to providing these bindings, but unless I'm missing something obvious it seems that |
|
They are the same thing for the base class, and most widgets. But it is possible for something to be scrollable that is not a container. I think the only one we have is |
…able The code is exactly the same between is_container and is_scrollable, and the intent (confirmed in Textualize#1343 (comment)) is that the latter is intended to be overridden in some circumstances (so far only in `ScrollView`). As such I feel it better conveys intent and reduces the changes of mismatched future changes if is_scrollable is defined in respect to is_container. The only possible reason I can think of is if there's a measurable performance win here. Applying Knuth for the moment, at least for the scope of this PR. I strongly suspect this is one of the 97% rather than one of the 3% and for the purposes of moving stuff around (which I may be doing as I explore this) I believe this makes it easier to follow and to think about.
This is the heart of the issue introduced by Textualize@b48a140 and which is being investigated in Textualize#1343 -- the child widget can be focused, but (as far as the author of the code is concerned) it has no bindings. Bindings for movement-oriented keys exist on the screen which composes up the widget into it. Up until 0.5.0 this worked just fine. As of 0.6.0, because binding inheritance was introduced, the bindings for movement that live at the `Widget` level cause the widget that has no bindings to appear to have bindings. While this can potentially be worked around with the use of inherit_bindings, this isn't a very satisfying solution and also breaks the rule of least astonishment. This test is going to be key to all of this. This is the test that should be made to work without breaking any of the other currently-passing tests.
|
While building up a collection of unit tests to explore all the issues caused by this change, I've found another that, while heavily overlapping with this one, I feel deserves an issue of its own: #1351 |
Testing the overlap between Textualize#1343 and Textualize#1351.
|
I think I've got a pretty comprehensive collection of tests in #1346 now -- I suspect there's one more to add, which will be the situation where a screen wants to capture movement bindings, contains a container1 that won't ever scroll, which in turn contains widgets that won't ever scroll. Footnotes
|
|
Tidying up the current set of tests, with a view to making them a PR in their own right (0.6.x needs these standalone of any changes anyway, either to describe the current problem, or to test changes made to fix it), and suddenly this:
11 instances of these tests passed before this, and looking at the issue I suspect the problem may be simply a timing issue with faking key presses and subsequently detecting them. Perhaps all of the fake presses are going to require a pause along the way? |
|
Had a good long chat with @willmcgugan about this this morning, and the options we have available to resolving this. After going over the causes and symptoms we've arrived at this:
In other words, mostly, this is working as advertised and intended, it's just that binding inheritance has had the unintended consequence of elevating the movement keys to a sort of special "system" status. Apps that make use of them in the way I have been will need to bind them as The next step is to modify OIDIA and 5x5 in this way and be confidant that it fixes the problem. If so... The naming of
|
Due to binding inheritance being introduced in 0.6.0, the cursor keys stopped working in the app. The issue is that they're getting consumed by widgets *below* the screen now. This makes the screen as having priority for those keys. See Textualize/textual#1343 for a bunch of background on this, and note that this will change some more in the future once the fix we're aiming for goes into place.
The cursor keys stopped working in 5x5 once binding inheritance was introduced in 0.6.0 (see Textualize#1343). Making them `universal` keys here fixes the issue. This won't be the final form of this change, but this fixes this example so that it works with 0.6.0 (so anyone cloning down the code and running with an installed 0.6.0 will get the full effect). Once the final work resulting from Textualize#1343 takes place this will need a final update (and should be a good test example for the changes).
|
Both #1360 and davep/oidia@a3dbb19 nicely demonstrate that this ultimately does come down to the use of |
This commit changes things slightly so that the priority of a binding is an three-state: True, False or None. True and False are firm choices that nothing else should override. None says "fall back to whatever default is up for grabs". The commit also then adds support for a default priority and, when building a binding, it uses that if the binding has a priority of None. See Textualize#1343.
This works in conjunction with BINDINGS. If a widget has BINDINGS, and if any of those bindings have a priority that isn't True or False, the value of PRIORITY_BINDINGS will be used (or the value from one of the parent classes, if there are any, will be used if it isn't set on the current class). See Textualize#1343.
As requested by @willmcgugan while discussing Textualize#1343.
As requested by @willmcgugan while discussing Textualize#1343.
I feel some more will be needed, but this is all of the basics, hitting all of the important points that relate to Textualize#1343. More importantly all of the xfails are now removed.
|
Something I should add to #1346
No matter what the final decision is on how these situations work out, the tests will be there to ensure the code works as desired and that any future changes take this into account. |
|
Don't forget to star the repository! Follow @textualizeio for Textual updates. |
With Textual 0.5.0 I had bindings on the arrow keys that worked fine. Having upgraded to 0.6.0 those bindings no longer seem to work. Isolating the issue I can recreate with this code (some other keys thrown in as control tests)
Running the above, pressing the arrow keys vs some of the other keys...
The text was updated successfully, but these errors were encountered: