From 7b1688c2462d7e01c4ed83cd3d2c96f90c0fabff Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 18 Oct 2022 13:04:12 +0100 Subject: [PATCH 1/7] Don't remove a widget from display before removal This change ensures that, for example, the widget remains in the Screen.focus_chain so that it's easier to handle passing off focus on removal. To help address #939. --- src/textual/widget.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/textual/widget.py b/src/textual/widget.py index dab9e06d14..cb152bdd6f 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -1775,7 +1775,6 @@ def refresh( def remove(self) -> None: """Remove the Widget from the DOM (effectively deleting it)""" - self.display = False self.app.post_message_no_wait(events.Remove(self, widget=self)) def render(self) -> RenderableType: From 750b7ca52d123065027ab9d8f8e674bfe312b889 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 18 Oct 2022 13:17:06 +0100 Subject: [PATCH 2/7] Resettle focus during removal using the focus chain Rather than call on Screen._reset_focus, which seems to only really consider siblings, instead call on the being-removed widget's screen to find the previous widget that can receive focus. Addresses #939. --- src/textual/app.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 20ae53db96..b39539273b 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1419,12 +1419,12 @@ async def _on_remove(self, event: events.Remove) -> None: widget = event.widget parent = widget.parent - widget.reset_focus() - remove_widgets = widget.walk_children( Widget, with_self=True, method="depth", reverse=True ) for child in remove_widgets: + if self.focused is child: + child.screen.focus_previous() await child._close_messages() self._unregister(child) if parent is not None: From d40fd245257e28cddc58918e39df2721ab22b561 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 18 Oct 2022 16:01:49 +0100 Subject: [PATCH 3/7] Add Screen.focus_from Added to allow checking where focus can and should go without actually causing focus to move. Initially added to aid resolve #943. Also mote that Screen._move_focus has been rewritten to use Screen.focus_from (really the bulk of Screen.focus_from was taken from Screen._move_focus). --- src/textual/screen.py | 45 ++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/src/textual/screen.py b/src/textual/screen.py index d32b2dda53..5af6f44992 100644 --- a/src/textual/screen.py +++ b/src/textual/screen.py @@ -175,37 +175,52 @@ def focus_chain(self) -> list[Widget]: return widgets - def _move_focus(self, direction: int = 0) -> Widget | None: - """Move the focus in the given direction. + def focus_from(self, widget: Widget | None, direction: int = 0) -> Widget | None: + """Locate a candidate widget given a focus direction. Args: - direction (int, optional): 1 to move forward, -1 to move backward, or + direction (int, optional): 1 to locate forward, -1 to locate backward, or 0 to keep the current focus. Returns: - Widget | None: Newly focused widget, or None for no focus. + Widget | None: The candidate widget, or None for no focus. """ focusable_widgets = self.focus_chain if not focusable_widgets: # Nothing focusable, so nothing to do - return self.focused - if self.focused is None: - # Nothing currently focused, so focus the first one - self.set_focus(focusable_widgets[0]) + return None + if widget is None: + # No widget given so the first in the chain is the best + # candidate from here. + return focusable_widgets[0] else: try: - # Find the index of the currently focused widget - current_index = focusable_widgets.index(self.focused) + # Find the index of the widget to seek from. + current_index = focusable_widgets.index(widget) except ValueError: - # Focused widget was removed in the interim, start again - self.set_focus(focusable_widgets[0]) + # That widget was removed in the interim, start again + return focusable_widgets[0] else: - # Only move the focus if we are currently showing the focus + # Only seek a new widget if we've been given a direction. if direction: - current_index = (current_index + direction) % len(focusable_widgets) - self.set_focus(focusable_widgets[current_index]) + return focusable_widgets[ + (current_index + direction) % len(focusable_widgets) + ] + + return None + + def _move_focus(self, direction: int = 0) -> Widget | None: + """Move the focus in the given direction. + + Args: + direction (int, optional): 1 to move forward, -1 to move backward, or + 0 to keep the current focus. + Returns: + Widget | None: Newly focused widget, or None for no focus. + """ + self.set_focus(self.focus_from(self.focused, direction)) return self.focused def focus_next(self) -> Widget | None: From 6c121e266a118cb225a9681f9a39c71f31243ed9 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 18 Oct 2022 16:04:57 +0100 Subject: [PATCH 4/7] Rework the settling of focus to guard against extra messages In aid of resolving #943. --- src/textual/app.py | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index b39539273b..110c9f7caf 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1419,14 +1419,36 @@ async def _on_remove(self, event: events.Remove) -> None: widget = event.widget parent = widget.parent - remove_widgets = widget.walk_children( - Widget, with_self=True, method="depth", reverse=True + # Get the list of all the widgets that are about to be removed. + remove_widgets = list( + widget.walk_children(Widget, with_self=True, method="depth", reverse=True) ) + + # Of the list of widgets to be removed, let's great a set of those + # that can receive focus -- we're about to go on a focus hunt and + # this can be a subset of the above. + remove_focusable = {widget for widget in remove_widgets if widget.can_focus} + + # Assume that we aren't going to move focus at all. + settle_focus = self.screen.focused + + # If there are more items in the focus chain than there are items to + # be removed that can receive focus... + if len(widget.screen.focus_chain) >= len(remove_focusable): + # ...go and seek the previous widget in the focus chain that + # isn't going to be removed. + while settle_focus is not None and settle_focus in remove_widgets: + settle_focus = self.screen.focus_from(settle_focus, -1) + else: + # It seems we can't settle on anywhere to focus. + settle_focus = None + for child in remove_widgets: - if self.focused is child: - child.screen.focus_previous() await child._close_messages() self._unregister(child) + + self.screen.set_focus(settle_focus) + if parent is not None: parent.refresh(layout=True) From 71b3c3b555aedcf01457715134cb57724cedb918 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 18 Oct 2022 16:42:04 +0100 Subject: [PATCH 5/7] Fix hang in case of single focused widget left standing --- src/textual/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/app.py b/src/textual/app.py index 110c9f7caf..09b1dfb1ce 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1434,7 +1434,7 @@ async def _on_remove(self, event: events.Remove) -> None: # If there are more items in the focus chain than there are items to # be removed that can receive focus... - if len(widget.screen.focus_chain) >= len(remove_focusable): + if len(widget.screen.focus_chain) > len(remove_focusable): # ...go and seek the previous widget in the focus chain that # isn't going to be removed. while settle_focus is not None and settle_focus in remove_widgets: From b36f1dc6e57a7a8f6c0ba0bd104fb26a2a100092 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 18 Oct 2022 20:16:18 +0100 Subject: [PATCH 6/7] Add a personal sandbox widget removal tester --- sandbox/davep/single_widget.py | 35 ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 sandbox/davep/single_widget.py diff --git a/sandbox/davep/single_widget.py b/sandbox/davep/single_widget.py new file mode 100644 index 0000000000..5ac58813ed --- /dev/null +++ b/sandbox/davep/single_widget.py @@ -0,0 +1,35 @@ +"""Sandbox app to test widget removal and focus-handing. + +Written to help test changes made to solve: + + https://github.com/Textualize/textual/issues/939 +""" + +from textual.app import App +from textual.containers import Container +from textual.widgets import Static + + +class NonFocusParent(Static): + def compose(self): + yield Static("Test") + + +class SingleWidgetTester(App[None]): + + BINDINGS = [("a", "add_widget", "Add Widget"), ("d", "del_widget", "Delete Widget")] + + def compose(self): + yield Container() + + def action_add_widget(self): + self.query_one(Container).mount(NonFocusParent()) + + def action_del_widget(self): + candidates = self.query(NonFocusParent) + if candidates: + candidates.last().remove() + + +if __name__ == "__main__": + SingleWidgetTester().run() From 3f6f295c4c717468d6189f2d286eca242282b1a7 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 18 Oct 2022 21:39:34 +0100 Subject: [PATCH 7/7] Fix a comment typo and also improve the explanation --- src/textual/app.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 09b1dfb1ce..a11927e523 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1424,9 +1424,10 @@ async def _on_remove(self, event: events.Remove) -> None: widget.walk_children(Widget, with_self=True, method="depth", reverse=True) ) - # Of the list of widgets to be removed, let's great a set of those + # Of the list of widgets to be removed, let's get a set of those # that can receive focus -- we're about to go on a focus hunt and - # this can be a subset of the above. + # this can be a subset of the above (that is: it's possible that not + # every widget we're about to remove is one that can hold focus). remove_focusable = {widget for widget in remove_widgets if widget.can_focus} # Assume that we aren't going to move focus at all.