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

Add "focus" property to widgets / allow setting focus on instantiation #2863

Closed
hyuri opened this issue Sep 21, 2024 · 6 comments
Closed

Add "focus" property to widgets / allow setting focus on instantiation #2863

hyuri opened this issue Sep 21, 2024 · 6 comments
Labels
enhancement New features, or improvements to existing features.

Comments

@hyuri
Copy link

hyuri commented Sep 21, 2024

What is the problem or limitation you are having?

Currently, we cannot set focus on unnamed instances of Widget. Only in named, after instantiation, and apparently after attaching the widget to the visible window.

And toga.Button(...).focus() doesn't work.

But in some cases, we might need to set focus to an unnamed instance, like in the example below:

def render_page(app):
        return toga.Box(
                children = [
                        toga.Box(
                                toga.Label(...)
                                toga.Button(...)
                                toga.Button(...)
                        )
                ]
        )

Or even:

def render_page(app):
        my_page = toga.Box(
                children = [
                        toga.Box(
                                toga.Label(...)
                                toga.Button(...)
                                toga.Button(...)
                        )
                ]
        )

        return my_page

Describe the solution you'd like

Add "focus" property to Widget, and allow us to set focus on instantiation. So that this is possible (notice the second button):

def render_page(app):
        return toga.Box(
                children = [
                        toga.Box(
                                toga.Label(...)
                                toga.Button(...)
                                toga.Button(..., focus = True)
                        )
                ]
        )

Describe alternatives you've considered

Enable us to call .focus() on instantiation:

toga.Button(...).focus()

Additional context

No response

@hyuri hyuri added the enhancement New features, or improvements to existing features. label Sep 21, 2024
@freakboy3742
Copy link
Member

I'm going to close this as "not possible". The problem is that focus is a concept that only makes sense in the context of a widget that is in an active layout; and it's also imperative - it applies right now.

If focus() were to be applied to a widget at time of construction (either by allowing the chained call, or as a focus=True argument), what would prevent multiple widgets from having that property? They can't both be honoured... which one is honoured first?

It might be possible to find a way to implement this by registering the focus request and deferring until the widget was added to a layout - but it would be a complex mechanism, and would result in some odd side effects, so I'm not convinced it would make for intuitive API.

@hyuri
Copy link
Author

hyuri commented Sep 22, 2024

If focus() were to be applied to a widget at time of construction (either by allowing the chained call, or as a focus=True argument), what would prevent multiple widgets from having that property? They can't both be honoured... which one is honoured first?

It would be rare, and human error, but the one to be honoured should be the last widget that sets focus=True.

It might be possible to find a way to implement this by registering the focus request and deferring until the widget was added to a layout - but it would be a complex mechanism, and would result in some odd side effects, so I'm not convinced it would make for intuitive API.

I don't know about Toga's internals, so can't I speak to the complexity, but yes, deferring is what I had in mind. Pseudocode:

on_main_window_content_changed:
    focus_requested_widget = get_last_widget_to_request_focus()
    focus_requested_widget.focus()

@mhsmith
Copy link
Member

mhsmith commented Sep 23, 2024

the one to be honoured should be the last widget that sets focus=True

But what do you mean by "last"? Last one to be constructed? Last one to be added to the layout? Neither of these might be obvious from the code in a non-trivial app, so you end up with "spooky action at a distance". With a method, the ordering is much clearer.

@hyuri
Copy link
Author

hyuri commented Sep 23, 2024

The last one to make the assignment when attaching the widgets to the tree. Basically let Python run its course:
a = None
a = 1
a = 2

get_a() # Returns 2

In other words, what I have in mind is:

main_window.focus = None

# Definition step — main_window.focus is unchanged here
new_content = toga.Box(
    children = [
        toga.Button([...], focus = True), # This defines that main_window.focus should be set to this anonymous widget when it finally gets added to the tree later
        [...]
        toga.Button([...], focus = True) # But now this anonymous widget overwrites the previous anonymous widget's definition to point main_window.focus to itself
)

# Execution step
# At this point point, main_window's content changes, so it triggers the code that adds to the tree the Box and child widgets defined in new_content
main_window.content = new_content

Neither of these might be obvious from the code in a non-trivial app

Collisions should be extremely rare, since there's no intention to set focus on more than one widget. I would hope a non-trivial app is made modular enough to allow finding this issue rather quickly

The way I've been doing it since I made the report is:

  1. In every page (Box containing widgets), assign an ID to the widget I want to get focus
  2. Assign that ID to the app.focus_id variable I defined initially as None in setup()
  3. When I call go_to_page(page), it sets self.main_window.content = page.render() (this returns a Box with widgets), and, following that, sets self.widgets[self.focus_id].focus()

@freakboy3742
Copy link
Member

Neither of these might be obvious from the code in a non-trivial app

Collisions should be extremely rare, since there's no intention to set focus on more than one widget. I would hope a non-trivial app is made modular enough to allow finding this issue rather quickly

They're "extremely rare" until the API actively encourages people to use that pattern. If an API can be used in the wrong way, it will be used in the wrong way. The way you avoid this is by having an API that only allows for correct usage. And that is what an explicit method-based approach provides.

@hyuri
Copy link
Author

hyuri commented Sep 23, 2024

Sure, but offering both a setter method that does things the right way and a property that can be changed directly is fairly common. That's what allows a += 1, for example. The dynamism of Python lets you to do many things wrong, like setting a = 1 followed by a = 2 when you only intended to set it once. But the extra care and attention is a cost you bear for the value you get.

In Toga itself, for example, we can set main_window.content directly, and that can lead to us unintentionally setting it to two different widgets in a row, even though we are only supposed to have one widget in main_window.content at a time.

Edit:
By the way, I managed to achieve the desired effect by using lambda, an app-level focus variable and a focus setting step after assigning the new new content, and it works just the way I described: the the last widget to assign its own id to app.focus_id is the one that gains focus when the page gets rendered and assigned to the main window.

class MyApp(toga.App):
    def setup(self):
        self.focus_id = None
        [...]

    def go_to_page(self, page_id):
        [...a bunch of checks...]
        page = get_page_by_id(page_id)
        self.main_window.content = page.render_page(self)
        self.widgets[self.focus_id].focus()

In the "my_page" file:
def render_page(app):
    return toga.Box(
        children = [
            toga.Button(
                id = (lambda self_id: setattr(app, "focus_id", self_id) or self_id)(my_button_id := "my_button"),
                on_press = partial(go_to_page(app), "my_page"),
            )
        ]
    )

I can live with it, but it obviously makes the app's code a bit more convoluted and less clear than it needs to be. It would nice if we had this built-in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features.
Projects
None yet
Development

No branches or pull requests

3 participants