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

exclude removed reactables #1750

Merged
merged 8 commits into from
Feb 9, 2023
Merged

exclude removed reactables #1750

merged 8 commits into from
Feb 9, 2023

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Feb 9, 2023

Fixes #1714

When calling switch_screen a screen is removed, but the watchers in the header were called after the DOM had been deleted.

  • Deprecates reactive.watch in favor of DOMNode.watch
  • Removes watch callbacks for detached nodes

@davep
Copy link
Contributor

davep commented Feb 9, 2023

Running the example code in the related issue, with this PR, causes this error when I follow the instructions in that issue:

╭──────────────────────────────────────────────────────────── Traceback (most recent call last) ─────────────────────────────────────────────────────────────╮
│ /Users/davep/temp/textual/src/textual/widgets/_header.py:136 in on_mount                                                                                   │
│                                                                                                                                                            │
│   133 │   │   def set_sub_title(sub_title: str) -> None:                                                                                                   │
│   134 │   │   │   self.query_one(HeaderTitle).sub_text = sub_title                                                                                         │
│   135 │   │                                                                                                                                                │
│ ❱ 136 │   │   watch(self.app, "title", set_title)                                                                                                          │
│   137 │   │   watch(self.app, "sub_title", set_sub_title)                                                                                                  │
│   138                                                                                                                                                      │
│                                                                                                                                                            │
│ ╭───────────────────────────────────── locals ─────────────────────────────────────╮                                                                       │
│ │          self = Header()                                                         │                                                                       │
│ │ set_sub_title = <function Header.on_mount.<locals>.set_sub_title at 0x103fbf2e0> │                                                                       │
│ │     set_title = <function Header.on_mount.<locals>.set_title at 0x103fbf060>     │                                                                       │
│ ╰──────────────────────────────────────────────────────────────────────────────────╯                                                                       │
│                                                                                                                                                            │
│ /Users/davep/temp/textual/src/textual/reactive.py:352 in watch                                                                                             │
│                                                                                                                                                            │
│   349 │   watcher_list.append((obj, callback))                                                                                                             │
│   350 │   if init:                                                                                                                                         │
│   351 │   │   current_value = getattr(obj, attribute_name, None)                                                                                           │
│ ❱ 352 │   │   Reactive._check_watchers(obj, attribute_name, current_value)                                                                                 │
│   353                                                                                                                                                      │
│                                                                                                                                                            │
│ ╭───────────────────────────────────────────────────────────────────── locals ─────────────────────────────────────────────────────────────────────╮       │
│ │ attribute_name = 'title'                                                                                                                         │       │
│ │       callback = <function Header.on_mount.<locals>.set_title at 0x103fbf060>                                                                    │       │
│ │  current_value = 'ModalApp'                                                                                                                      │       │
│ │           init = True                                                                                                                            │       │
│ │            obj = ModalApp(title='ModalApp', classes={'-dark-mode'})                                                                              │       │
│ │   watcher_list = [                                                                                                                               │       │
│ │                  │   (ModalApp(title='ModalApp', classes={'-dark-mode'}), <function Header.on_mount.<locals>.set_title at 0x103f8af20>),         │       │
│ │                  │   (ModalApp(title='ModalApp', classes={'-dark-mode'}), <function Header.on_mount.<locals>.set_title at 0x103fbca40>),         │       │
│ │                  │   (ModalApp(title='ModalApp', classes={'-dark-mode'}), <function Header.on_mount.<locals>.set_title at 0x103fbf060>)          │       │
│ │                  ]                                                                                                                               │       │
│ │       watchers = {                                                                                                                               │       │
│ │                  │   'title': [                                                                                                                  │       │
│ │                  │   │   (ModalApp(title='ModalApp', classes={'-dark-mode'}), <function Header.on_mount.<locals>.set_title at 0x103f8af20>),     │       │
│ │                  │   │   (ModalApp(title='ModalApp', classes={'-dark-mode'}), <function Header.on_mount.<locals>.set_title at 0x103fbca40>),     │       │
│ │                  │   │   (ModalApp(title='ModalApp', classes={'-dark-mode'}), <function Header.on_mount.<locals>.set_title at 0x103fbf060>)      │       │
│ │                  │   ],                                                                                                                          │       │
│ │                  │   'sub_title': [                                                                                                              │       │
│ │                  │   │   (                                                                                                                       │       │
│ │                  │   │   │   ModalApp(title='ModalApp', classes={'-dark-mode'}),                                                                 │       │
│ │                  │   │   │   <function Header.on_mount.<locals>.set_sub_title at 0x103f58e00>                                                    │       │
│ │                  │   │   ),                                                                                                                      │       │
│ │                  │   │   (                                                                                                                       │       │
│ │                  │   │   │   ModalApp(title='ModalApp', classes={'-dark-mode'}),                                                                 │       │
│ │                  │   │   │   <function Header.on_mount.<locals>.set_sub_title at 0x103fbcf40>                                                    │       │
│ │                  │   │   )                                                                                                                       │       │
│ │                  │   ]                                                                                                                           │       │
│ │                  }                                                                                                                               │       │
│ ╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯       │
│                                                                                                                                                            │
│ /Users/davep/temp/textual/src/textual/widgets/_header.py:131 in set_title                                                                                  │
│                                                                                                                                                            │
│   128 │                                                                                        ╭────── locals ──────╮                                      │
│   129 │   def on_mount(self) -> None:                                                          │  self = Header()   │                                      │
│   130 │   │   def set_title(title: str) -> None:                                               │ title = 'ModalApp' │                                      │
│ ❱ 131 │   │   │   self.query_one(HeaderTitle).text = title                                     ╰────────────────────╯                                      │
│   132 │   │                                                                                                                                                │
│   133 │   │   def set_sub_title(sub_title: str) -> None:                                                                                                   │
│   134 │   │   │   self.query_one(HeaderTitle).sub_text = sub_title                                                                                         │
│                                                                                                                                                            │
│ /Users/davep/temp/textual/src/textual/dom.py:818 in query_one                                                                                              │
│                                                                                                                                                            │
│   815 │   │   │   query_selector = selector.__name__                                                                                                       │
│   816 │   │   query: DOMQuery[Widget] = DOMQuery(self, filter=query_selector)                                                                              │
│   817 │   │                                                                                                                                                │
│ ❱ 818 │   │   return query.only_one() if expect_type is None else query.only_one(expect_type)                                                              │
│   819 │                                                                                                                                                    │
│   820 │   def set_styles(self, css: str | None = None, **update_styles) -> None:                                                                           │
│   821 │   │   """Set custom styles on this object."""                                                                                                      │
│                                                                                                                                                            │
│ ╭──────────────────────────── locals ────────────────────────────╮                                                                                         │
│ │       DOMQuery = <class 'textual.css.query.DOMQuery'>          │                                                                                         │
│ │    expect_type = None                                          │                                                                                         │
│ │          query = <DOMQuery Header() filter='HeaderTitle'>      │                                                                                         │
│ │ query_selector = 'HeaderTitle'                                 │                                                                                         │
│ │       selector = <class 'textual.widgets._header.HeaderTitle'> │                                                                                         │
│ │           self = Header()                                      │                                                                                         │
│ ╰────────────────────────────────────────────────────────────────╯                                                                                         │
│                                                                                                                                                            │
│ /Users/davep/temp/textual/src/textual/css/query.py:243 in only_one                                                                                         │
│                                                                                                                                                            │
│   240 │   │   """                                                                              ╭──────────────────────── locals ────────────────────────╮  │
│   241 │   │   # Call on first to get the first item. Here we'll use all of the                 │ expect_type = None                                     │  │
│   242 │   │   # testing and checking it provides.                                              │        self = <DOMQuery Header() filter='HeaderTitle'> │  │
│ ❱ 243 │   │   the_one = self.first(expect_type) if expect_type is not None else self.first()   ╰────────────────────────────────────────────────────────╯  │
│   244 │   │   try:                                                                                                                                         │
│   245 │   │   │   # Now see if we can access a subsequent item in the nodes. There                                                                         │
│   246 │   │   │   # should *not* be anything there, so we *should* get an                                                                                  │
│                                                                                                                                                            │
│ /Users/davep/temp/textual/src/textual/css/query.py:214 in first                                                                                            │
│                                                                                                                                                            │
│   211 │   │   │   │   │   )                                                                    ╭──────────────────────── locals ────────────────────────╮  │
│   212 │   │   │   return first                                                                 │ expect_type = None                                     │  │
│   213 │   │   else:                                                                            │        self = <DOMQuery Header() filter='HeaderTitle'> │  │
│ ❱ 214 │   │   │   raise NoMatches(f"No nodes match {self!r}")                                  ╰────────────────────────────────────────────────────────╯  │
│   215 │                                                                                                                                                    │
│   216 │   @overload                                                                                                                                        │
│   217 │   def only_one(self) -> Widget:                                                                                                                    │
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
NoMatches: No nodes match <DOMQuery Header() filter='HeaderTitle'>

This looks to be the same result as when I tested with main a couple of days ago.

@willmcgugan
Copy link
Collaborator Author

Bit premature @davep

@davep
Copy link
Contributor

davep commented Feb 9, 2023

Bit premature @davep

D'oh! Could have sworn I'd seen a review request come in on this one. Apologies, getting stuff mixed up!

@willmcgugan
Copy link
Collaborator Author

No worries. Ready now.

@davep
Copy link
Contributor

davep commented Feb 9, 2023

Hey look, it works now! ;-)

@@ -123,6 +123,20 @@ def log(self) -> Logger:
"""
return self.app._logger

@property
def is_attached(self) -> bool:
"""Check the node is attached to the app via the DOM."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a property, perhaps this would be better worded more like "Is the node attached to the app via the DOM?".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the type of thing I would say :')

@willmcgugan willmcgugan merged commit 67c2127 into main Feb 9, 2023
@willmcgugan willmcgugan deleted the fix-screen-switch branch February 9, 2023 12:08
@willmcgugan willmcgugan restored the fix-screen-switch branch February 9, 2023 12:19
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.

switch_screen() hangs
3 participants