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

Textual apps require window content to run #2818

Open
rmartin16 opened this issue Sep 6, 2024 · 8 comments · May be fixed by #2844
Open

Textual apps require window content to run #2818

rmartin16 opened this issue Sep 6, 2024 · 8 comments · May be fixed by #2844
Labels
bug A crash or error in behavior. good first issue Is this your first time contributing? This could be a good place to start!

Comments

@rmartin16
Copy link
Member

Describe the bug

If a Toga textual app does not define any window content, the app will crash.

❯ TOGA_BACKEND=toga_textual \
python -c 'import toga; toga.App(formal_name="MyApp", app_id="MyApp").main_loop()'

╭───────────────────────────────────────────────────────────────── Traceback (most recent call last) ─────────────────────────────────────────────────────────────────╮
│ /home/russell/github/beeware/toga/textual/src/toga_textual/window.py:111 in on_resize                                                                               │
│                                                                                                                                                                     │
│   108 │   │   self.impl = impl                                                                                                                                      │
│   109 │                                                                                                                                                             │
│   110 │   def on_resize(self, event) -> None:                                                                                                                       │
│ ❱ 111 │   │   self.interface.content.refresh()                                                                                                                      │
│   112                                                                                                                                                               │
│   113                                                                                                                                                               │
│   114 class Window:                                                                                                                                                 │
│                                                                                                                                                                     │
│ ╭───────────────────────────────────────── locals ─────────────────────────────────────────╮                                                                        │
│ │ event = Resize(size=Size(width=167, height=43), virtual_size=Size(width=167, height=43)) │                                                                        │
│ │  self = TogaMainWindow()                                                                 │                                                                        │
│ ╰──────────────────────────────────────────────────────────────────────────────────────────╯                                                                        │
╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
AttributeError: 'NoneType' object has no attribute 'refresh'

Steps to reproduce

Run this app:

TOGA_BACKEND=toga_textual \
python -c 'import toga; toga.App(formal_name="MyApp", app_id="MyApp").main_loop()'

Windows:

$env:TOGA_BACKEND="toga_textual"
python -c 'import toga; toga.App(formal_name="MyApp", app_id="MyApp").main_loop()'

Expected behavior

Apps using the textual backend are not required to use window content.

Screenshots

No response

Environment

  • Operating System: all
  • Python version:
  • Software versions:
    • Briefcase: 0.3.20.dev192+g32f529a4
    • Toga: 0.4.7.dev43+g63480d545

Logs

No response

Additional context

No response

@rmartin16 rmartin16 added the bug A crash or error in behavior. label Sep 6, 2024
@freakboy3742 freakboy3742 added the good first issue Is this your first time contributing? This could be a good place to start! label Sep 7, 2024
@freakboy3742
Copy link
Member

This came up recently in the context of #2473, there's some edge cases where it would be helpful to guarantee that window.content is always a valid widget, even if it's just an empty toga.Box(). That's easy enough to guarantee in the constructor, and has no appreciable downside that I can see, so we might as well do it.

@cgkoutzigiannis
Copy link
Contributor

I would like to give this a go! If I understand the codebase correctly the change should be made here, to check if the interface.content is a valid Widget:

class Window:
def __init__(self, interface, title, position, size):
self.interface = interface
self.create()
self.container = Container(self.native)
self.set_title(title)

Sidenote: It's kind of hard to debug the Textual backend. I usually use the pdb debugger, but Textual completely hijacks the terminal window so that doesn't work at all. Any tips to overcome that?

@rmartin16
Copy link
Member Author

Sidenote: It's kind of hard to debug the Textual backend. I usually use the pdb debugger, but Textual completely hijacks the terminal window so that doesn't work at all. Any tips to overcome that?

I've been using two different methods.

Write to a log file on the App object:

class HelloWorld(toga.App):
    file = open(Path.home() / "toga_app_log.txt", "w")

    @classmethod
    def log(cls, msg: str):
        cls.file.write(f"{msg}\n")
        cls.file.flush()

Or add headless=true when running the app in main_loop().

@freakboy3742
Copy link
Member

I would like to give this a go! If I understand the codebase correctly the change should be made here, to check if the interface.content is a valid Widget:

Not quite. The issue is that right now, this is possible:

def startup(self):
    self.my_window = toga.Window("Window Title")
    self.my_window.show()

That's legal, but the Window has no content (i.e., self.my_window.content is None). That's not a huge problem for most backends, but it is for Textual.

What we need to do is ensure that at time of construction, the initial value of self.my_window.content always has a value, even if it's a freshly created instance of toga.Box() (i.e., an empty container). That's effectively what the None case ends up rendering anyway; we could fix this as a specific case for Textual, but we might as well fix it for all platforms, and remove the possibility that content is ever None.

Sidenote: It's kind of hard to debug the Textual backend. I usually use the pdb debugger, but Textual completely hijacks the terminal window so that doesn't work at all. Any tips to overcome that?

Textual has some developer tools that involves running a second terminal that gives you access to the "console" that you'd normally see when running an app; see https://textual.textualize.io/guide/devtools/ for details. Run your Toga app with those tools, and it should be a lot easier to debug.

@cgkoutzigiannis
Copy link
Contributor

So, I should instead make the change on the Window constructor on toga-core, right?

@freakboy3742
Copy link
Member

So, I should instead make the change on the Window constructor on toga-core, right?

Correct - along with updates to the tests in the toga-core package to validate the change (I suspect you'll find there's a test that will fail when you add a default widget).

Also - make sure you have a read of the contribution guide (if you haven't already) - it's got lots of details of how to set up your development environment, and the other details you'll need to have in place in any pull request you submit.

@cgkoutzigiannis
Copy link
Contributor

I can't find a way to provide a default value (an empty toga.Box()) to the content constructor argument, without causing circular import errors. I could leave None as a default value and make sure that we change that to an empty Box, but that doesn't feel right.

@freakboy3742
Copy link
Member

freakboy3742 commented Sep 11, 2024

@cgkoutzigiannis Using None as the default value is unambiguously the right approach in this case. Here's the reason why:

def my_func(value=[]):
    value.append("bing")
    print(value)

my_func()
my_func()

The output of the first call to my_func() is:

['bing']

But the output of the second call to my_func() is:

['bing', 'bing']

And a third call would be

['bing', 'bing', 'bing']

That is - the default value isn't instantiated on each call to the method - it's instantiated once, when the method is parsed. This doesn't matter when the object is a primitive like an integer or None, or an immutable object like a string; but when it's an object that can have state (like a list, dictionary... or a toga.Box) it's a problem.

If you use toga.Box() as the default value, aside from any circular import issues, you'll also be creating a single box instance that Python will try to use on every Window. Which is find as long as you have one window... but as soon as you have 2 empty windows, you have a problem.

So - using None as the default value, and then creating an instance of toga.Box() when a value of None is observed is absolutely the right approach.

It has the additional benefit that toga.Window(content=None) also works - an explicit None will be converted into an actual empty widget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A crash or error in behavior. good first issue Is this your first time contributing? This could be a good place to start!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants