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

New Feature: Get a reference to a widget by id #930

Closed
Bobw147 opened this issue May 31, 2020 · 20 comments · Fixed by #1599
Closed

New Feature: Get a reference to a widget by id #930

Bobw147 opened this issue May 31, 2020 · 20 comments · Fixed by #1599
Labels
enhancement New features, or improvements to existing features. not quite right The idea or PR has been reviewed, but more work is needed.

Comments

@Bobw147
Copy link
Contributor

Bobw147 commented May 31, 2020

It is perfectly valid Python to define either a full or partial GUI widget hierarchy in the following way.

    self.content = Box(
        style=Pack(flex=1, direction=COLUMN),
        children=[
            TextInput(
                id='node_name_input',
                placeholder='Enter topic name'
            ),
            Box(
                style=Pack(flex=1),
                children=[
                    Button(
                        'Ok',
                        on_press=self.on_accept
                    ),
                    Button(
                        'Cancel',
                        on_press=self.on_close
                    )
                ]
            )
        ]
    )

In this example I subsequently want to get the value of the TextInput box when the 'Ok' button is clicked but because of the way it has been declared I have no immediate reference to it. It would be useful to have a method in the base Window class that mimics the HTML DOM getElementById() that returns a reference to the widget given its 'id'.

@freakboy3742
Copy link
Member

Definite +1 to the idea of being able to retrieve widgets by ID.

My question is about how that API is exposed. One of the underlying principles of Toga's design is to present a Pythonic API - not just copy the APIs exposed by underlying systems. Directly emulating the DOM API has some merit in that it's a "known quantity"; but it's not especially Pythonic - the existence of getter and setter methods in a Python API is usually a bit of a code smell.

A couple of possible alternatives:

  • Use the Pythonic idiom for lookup-by-attribute: __getitem__. This could be on the window itself (window['my_node_id']), or made explicit that it's a widget lookup using a descriptor (window.widgets['my_node_id'])
  • Put the 'DOM-analog' API behind a DOM namespace, so it's obvious that you're moving out of a "Pythonic" context: window.dom.get_widget_by_id('my_node_id')

By way of defensive API design, I'd also like to take a moment to consider what other DOM APIs we might want to import here. It's easy to imagine that lookup by "class" and/or selector could be useful (if only for applying a CSS stylesheet) - are there any other DOM-like APIs that we might want to add to Toga? The first API implementation doesn't necessarily need to implement any or all of these other APIs - but we should make sure we don't box ourselves into a corner by adding one method in a way that precludes adding others.

@Bobw147
Copy link
Contributor Author

Bobw147 commented Jun 1, 2020

Thanks for the feedback and certainly some things to think about there. I am not particularly attached to the current implementation, it was put together to get me out of a hole using the simplest available solution. That said the typical documentation for the 'id' field is currently along the lines of 'The DOM identifier for the window. This id can be used to target CSS directives' (taken from the window.py) implying that its intended usage is analogous to the HTML id attribute. Consequently I felt in general that your intention was to mimic the HTML DOM/CSS approach.

'Use the Pythonic idiom for lookup-by-attribute: getitem. This could be on the window itself (window['my_node_id']), or made explicit that it's a widget lookup using a descriptor (window.widgets['my_node_id'])'

I did briefly think about a dictionary/attribute approach but discounted it as I felt it is, to some extent, duplicating information that is already available via the node tree. As such it would require extra maintenance and consideration in light of any future structural changes/updates. TBH having said that it would still be my preferred solution if you want a more pythonic approach.

'Put the 'DOM-analog' API behind a DOM namespace, so it's obvious that you're moving out of a "Pythonic" context: window.dom.get_widget_by_id('my_node_id')'.

I am not so keen on this approach personally as it introduces another level of complexity alongside native, _impl, interface etc. Also widgets are not added via 'window.dom.content' or 'widget.dom.add_children' so I would find having to use a 'dom' namespace to recover references to them somewhat idiosyncratic.

As for the wider consideration of other DOM API calls, I am happy to have a look through and see what may be useful.

@saroad2
Copy link
Member

saroad2 commented Jun 1, 2020

I think that another approach we should consider is using a DOM singleton as our widget source.
Here some code example of what I mean:

from toga import DOM
from travertino.color import BLUE

# Get element by ID
my_box = DOM.get_element_by_id(id="my_box")
my_box.style.backgorund_color = BLUE

# Get elements by name
elements_list = DOM.get_elements_by_name(name="push_buttons")
for element in elements_list:
   element.label = "Push me"

I know it very HTML-driven, but I think it makes some sense even for Windows, Mac, and Linux.
What do you think?

@Bobw147
Copy link
Contributor Author

Bobw147 commented Jun 1, 2020

I would find it inconsistent and incongruous to have get the instance via a DOM namespace when I didn't have to use one to add the widgets in the first place. What would represent the Document? The app? The main window? A non-main window? I can see the appeal of having something HTML-like but would it be right?

@saroad2
Copy link
Member

saroad2 commented Jun 1, 2020

I don't think that it should be added to the MainWindow class because not all widgets are showed in the main window. As I see it, there are 2 options:

  1. Add it to the Window class where the ids are window specific, ie. ids can be duplicated between windows
  2. Add it to the App class where the ids are not window specific, ie. ids cannot be duplicated between windows.

Between the two, I prefer the second one.

@Bobw147
Copy link
Contributor Author

Bobw147 commented Jun 1, 2020

Thanks for the input.
I totally agree that it shouldn't be added to the main window class but I prefer the first option of the two by quite a long way :-)

My use case was to recover the value of a text input field from a form on a non-modal pop-up window in lieu of modal windows being implemented. The widget only exists for the short lifetime of the window and the widget tree is unique to that window. The window specific id is perfect for this because I don't have to worry about name duplication across all possible displayable windows in an app. In fact it allows me to be consistent with my naming convention if I so desire. Also, once again, since I have to add my main widget (usually a container) to a window and other widgets are added to the tree under that main widget, I wouldn't expect to have to go to the app instance in order to obtain the instance ref I am after.

@freakboy3742
Copy link
Member

the typical documentation for the 'id' field is currently along the lines of 'The DOM identifier for the window. This id can be used to target CSS directives' (taken from the window.py) implying that its intended usage is analogous to the HTML id attribute. Consequently I felt in general that your intention was to mimic the HTML DOM/CSS approach.

You're completely correct in saying that the functional role of the ID is identical to that of the HTML DOM/CSS ID. However, that doesn't mean the API we expose must to be named the same as the DOM API.

'Use the Pythonic idiom for lookup-by-attribute: getitem. This could be on the window itself (window['my_node_id']), or made explicit that it's a widget lookup using a descriptor (window.widgets['my_node_id'])'

I did briefly think about a dictionary/attribute approach but discounted it as I felt it is, to some extent, duplicating information that is already available via the node tree. As such it would require extra maintenance and consideration in light of any future structural changes/updates. TBH having said that it would still be my preferred solution if you want a more pythonic approach.

I think we may have some crossed wires here. I'm not proposing a change in functionality - I'm talking purely about the name of the function that is used to access this capability. You're suggesting get_widget_by_id(); I'm suggesting __getitem__() (which means that in practice, you use [] to invoke the function). The implementation of the method would be otherwise identical.

I think there is some value in there actually being a lookup map behind the scenes, but that's an implementation issue (that I brought up on the related), not an API design issue. The real test of a good API is whether you can replace the implementation without changing the API at all - and in this case, "get the widget that has ID X" betrays nothing about the implementation, no matter how you spell it.

'Put the 'DOM-analog' API behind a DOM namespace, so it's obvious that you're moving out of a "Pythonic" context: window.dom.get_widget_by_id('my_node_id')'.

I am not so keen on this approach personally as it introduces another level of complexity alongside native, _impl, interface etc. Also widgets are not added via 'window.dom.content' or 'widget.dom.add_children' so I would find having to use a 'dom' namespace to recover references to them somewhat idiosyncratic.

I completely agree that this would be idiosyncratic :-) My question is whether it would be too idiosyncratic to make sense. The purpose behind the .dom "spacer" is purely to provide a mental "past this point the functions won't have Pythonic names" barrier.

That said: I'd rather avoid non-pythonic names altogether; and looking at this with fresh eyes, it also occurs to me that get_widget_by_id() isn't the DOM name anyway. The DOM name would be get_element_by_id(), which doesn't make any sense in the Toga context. Leaning on the DOM analogy too hard (either by having a DOM namespace, or using a DOM-like function name) probably isn't a good idea.

As for the wider consideration of other DOM API calls, I am happy to have a look through and see what may be useful.

I think it's worth a cursory survey - it provides useful guidance over whether we're talking about one or two methods, which could be added to the App/Window namespace without much problem; or if we're talking about dozens, in which case a widget-search/manipulation namespace may be called for - with the specific name being a separate matter.

@freakboy3742
Copy link
Member

As I see it, there are 2 options:

1. Add it to the `Window` class where the ids are **window specific**, ie. ids can be duplicated between windows

2. Add it to the `App` class where the ids are **not window specific**, ie. ids cannot be duplicated between windows.

Between the two, I prefer the second one.

The scope of IDs is (or should be) global; so I can see the instinct to make this an App API. There's also the fact that Windows have IDs.

However, I can also see @Bobw147's point; from the perspective of the app builder, the app has to exist, but it's the Window that owns widgets.

Maybe we actually need "get by ID" APIs on both App and Window? Other than the fact that we'd be duplicating an API, is there any reason having a consistent API in both locations would be bad? Presumably, the Window version would be scoped so that it would only return widgets that belonged to the window, whereas the App lookup would be global.

@saroad2
Copy link
Member

saroad2 commented Jun 2, 2020

Maybe we actually need "get by ID" APIs on both App and Window? Other than the fact that we'd be duplicating an API, is there any reason having a consistent API in both locations would be bad? Presumably, the Window version would be scoped so that it would only return widgets that belonged to the window, whereas the App lookup would be global.

I don't think it's a good idea. It adds more points of failure and will be hard to debug in the future if bugs will occur. It is also very un-SOLID (violates single responsibility).

I still stick to to my opinion that the API should be only for the App class if we choose IDs to be located globally without duplications. If you want to enable duplications, stick to Window API. Don't do both.

Maybe it is worth raising this up in the Gitter chat and see what other people thinks about it.

@Bobw147
Copy link
Contributor Author

Bobw147 commented Jun 2, 2020

'I still stick to to my opinion that the API should be only for the App class if we choose IDs to be located globally without duplications. If you want to enable duplications, stick to Window API. Don't do both.'

I agree with this, I don't like the idea of the dual approach and the complexity it introduces. Regardless of whether id's should be unique or not a widget id will only exist for the lifetime of the window it is contained in. Since windows will be created and released dynamically throughout the lifetime of the app I don't see any value in a solution based on the latter. The need to access a widget only exists for the duration that its containing window is in scope (displayed or hidden) and a reference to that widget typically would be required to either set/get a value or possibly modify a style element. There may be other use cases (these are the ones that immediately spring to mind) but do any them violate the above principle?

On this basis my vote still goes firmly for a Window implementation rather than App regardless of the method of implementation.

I agree that it may be time to get a wider consensus

@freakboy3742 freakboy3742 added enhancement New features, or improvements to existing features. not quite right The idea or PR has been reviewed, but more work is needed. labels Jun 3, 2020
@saroad2
Copy link
Member

saroad2 commented Oct 1, 2022

Hey everyone. It's been a while since we disucessed this issue :)

My opinions have changed since we last spoke about this PR two years ago. Now, I do think that getting a widget by ID should be implemented in both Window and App. It is true that the App is where all widgets are stored, but we can't ignore the fact that in the end every widget belongs to a specific window, and we should be able to search for a widget in a window.

Using the __getitem__ function to get the widgets seems like the right pythonic approach. My only question is, should the API be app["widget_id"] or app.widgets["widget_id"]? I think the latter is more expressive than the first.

If we agree on this, I'll go ahead and implement it.
It is way overdue 😅.

@freakboy3742
Copy link
Member

I'd say app.widgets[]/window.widgets[] is the right API here. If you're up for implementing this, then go ahead!

@saroad2 saroad2 mentioned this issue Oct 2, 2022
4 tasks
@Bobw147
Copy link
Contributor Author

Bobw147 commented Oct 2, 2022

My preference would be for window.widgets[]. I personally feel that app.widgets[] is too broad a context and somewhat tenuous.

@mhsmith
Copy link
Member

mhsmith commented Oct 2, 2022

I agree: I'm not seeing any clear use case where you'd know the ID but not the window it's in, so what's the benefit in looking them up on the app? And if this requires widget IDs to be unique across the entire app, which I guess #1599 does, this will force multiple developers working on the same app to coordinate with each other to avoid conflicts, and probably end up using longer IDs to keep them unique, making their code much more verbose.

@freakboy3742
Copy link
Member

One reason for having a "global" ID space that I can see is consistency with the platform that already provides a directly analogous API - the web (provided as Document.getelementbyid()).

Another reason is the general motivation for having a "lookup by ID" in the first place - only needing one piece of identifying information to find a widget. If you're looking up widgets by ID, then you're already pretty far down the rabbit hole of not knowing which window a widget is in - after all, if you're keeping a close handle on widgets, you can store them as attributes on the app/window that you're building.

@saroad2
Copy link
Member

saroad2 commented Oct 3, 2022

In my opinion, there is no question about the need to get a widget via App.widgets. This is the most reasonable place most people will use this feature in. Some may use Window.widgets, but again: in order to know in which window the widget is, one must have a prior knowledge about the widget, which is not always there.

@mhsmith
Copy link
Member

mhsmith commented Oct 3, 2022

One reason for having a "global" ID space that I can see is consistency with the platform that already provides a directly analogous API - the web (provided as Document.getelementbyid()).

That depends on whether you consider the "document" to be analagous to the App or the Window. When a website opens multiple windows or tabs, isn't each of them a separate DOM Document?

If you're looking up widgets by ID, then you're already pretty far down the rabbit hole of not knowing which window a widget is in - after all, if you're keeping a close handle on widgets, you can store them as attributes on the app/window that you're building.

If you do that, then you can't define the layout using a nested, HTML-like expression as shown in the original post. There are obvious advantages to having the structure of the code reflect the structure of the layout, and the developer shouldn't be forced to pull a widget out of that structure just to assign it to an attribute. Unfortunately the walrus operator doesn't solve this, because it can't assign attributes, only simple variables.

In my opinion, there is no question about the need to get a widget via App.widgets. This is the most reasonable place most people will use this feature in.

Why do you think so? The only notational advantage I can see to the App is that the current app template generates a class for the app but not for the window. In that context, it's obviously much easier to type self.widgets["id"] rather than self.main_window.widgets["id"]. But this could be dealt with by making the template generate a separate window subclass, within which the developer can define the layout with self as the window.

in order to know in which window the widget is, one must have a prior knowledge about the widget, which is not always there.

Can you give an example where you would know the ID but not the window?

And what do you think of my point above about conflicts between multiple windows? Inevitably people would solve this by prefixing the ID with the window name. So where self is a Window, looking up globally-unique IDs on the App would give you this:

self.app.widgets["this_text_input"]         # This window
self.app.widgets["other_text_input"]        # Other window

While looking them up on the Window with window-scoped IDs would give you this:

self.widgets["text_input"]                  # 2 words shorter
self.other_window.widgets["text_input"]     # No change

Since accessing widgets within the same window will obviously be more common, the Window option will make the code shorter.

Finally, consider the case of multiple instances of the same window, e.g. one window per document. If IDs have to be unique across the app, then that means these windows can't use IDs at all.

@freakboy3742
Copy link
Member

One reason for having a "global" ID space that I can see is consistency with the platform that already provides a directly analogous API - the web (provided as Document.getelementbyid()).

That depends on whether you consider the "document" to be analagous to the App or the Window. When a website opens multiple windows or tabs, isn't each of them a separate DOM Document?

I guess the real question is "Is a web Document a Toga App, or a Toga Window"?

Given that there isn't an existing implementation of Window on the web backend (at least, not one that lets you open other windows), it's difficult to say - but having a separate window (and thus DOM document) per window would definitely be a likely outcome.

It's also worth noting that the actual implementation won't be using getelementbyid() - comparisons with the DOM are really only useful as an analogous APIs that can be used for design inspiration. You'll also still want to be able to reference widgets by ID from other windows - regardless of whether that be via app.widgets or other_window.widgets.

If you're looking up widgets by ID, then you're already pretty far down the rabbit hole of not knowing which window a widget is in - after all, if you're keeping a close handle on widgets, you can store them as attributes on the app/window that you're building.

If you do that, then you can't define the layout using a nested, HTML-like expression as shown in the original post. There are obvious advantages to having the structure of the code reflect the structure of the layout, and the developer shouldn't be forced to pull a widget out of that structure just to assign it to an attribute. Unfortunately the walrus operator doesn't solve this, because it can't assign attributes, only simple variables.

True, but I'd argue that's really a reflection of 2 different way of building an app - declarative vs imperative. There's clearly a lot of interest in the declarative approach - requests for a UI markup language are a regular theme on Discord etc - but all the existing code examples are imperative, because that's what the APIs are suited to.

In my opinion, there is no question about the need to get a widget via App.widgets. This is the most reasonable place most people will use this feature in.

Why do you think so? The only notational advantage I can see to the App is that the current app template generates a class for the app but not for the window. In that context, it's obviously much easier to type self.widgets["id"] rather than self.main_window.widgets["id"]. But this could be dealt with by making the template generate a separate window subclass, within which the developer can define the layout with self as the window.

To clarify - are you suggesting here that the end user would define class MyApp and class MyMainWindow, and MyApp.startup() would instantiate MyMainWindow? While I can definitely see how this could be a useful abstraction in some situations (and one that we should conceptually support... if we don't already), I'm a little wary about adopting it as a "default" approach in the template - needing to define 2 classes to get "Hello World" going is a little more complex that I'm comfortable with.

in order to know in which window the widget is, one must have a prior knowledge about the widget, which is not always there.

Can you give an example where you would know the ID but not the window?

I guess I'd flip this around and ask why would you know the window? You might have an ID for the window... but why would you necessarily know which window instance

For example - a GUI where you have a button in one window, and a chart widget in another. When the user presses the button, the chart is updated. In order for the handler for the button looks up the chart by ID, it needs to know which window the chart is in. To be sure - it's not that hard to store references to the two windows on the app (or store a reference to the chart's window on the button's window) - but at that point, why not store a reference to the the chart widget as well?

The other likely use case I can think of is in any API that accepts a widget as an argument. If we have a globally unique ID namespace, modifying that API to accept either widget or a string, with the string being an implied app.widgets[] lookup is trivial. If widget IDs are only unique to a window, this isn't possible (or, is only possible with very specific caveats of which IDs are allowed).

And what do you think of my point above about conflicts between multiple windows? Inevitably people would solve this by prefixing the ID with the window name. So where self is a Window, looking up globally-unique IDs on the App would give you this:

self.app.widgets["this_text_input"]         # This window
self.app.widgets["other_text_input"]        # Other window

Yes - but this is also a pattern that is very common in other ID-using contexts. I've used prefix-based ID lookup on more than one web project in my time.

While looking them up on the Window with window-scoped IDs would give you this:

self.widgets["text_input"]                  # 2 words shorter
self.other_window.widgets["text_input"]     # No change

Since accessing widgets within the same window will obviously be more common, the Window option will make the code shorter.

It will certainly be common - if only because a single window app is going to be the most common case. However, that's also the case where there's no ambiguity between per-app and per-window lookup.

While code length is definitely a concern, I'm not convinced it's something that should be the primary optimisation concern for an identifier.

Finally, consider the case of multiple instances of the same window, e.g. one window per document. If IDs have to be unique across the app, then that means these windows can't use IDs at all.

That's not true - it just means that the construction of widgets needs to incorporate a window disambiguating component in their ID scheme.

And this is something that will likely be needed in other contexts anyway. Consider the case of a complex widget composed of multiple subwidgets (say, 2 buttons and a canvas), and a GUI that deploys multiple versions of this complex widget in a single window. The constructor for the complex widget will need to incorporate a disambiguating naming scheme to separate the 2 buttons in a single widget from each other, and button 1 in widget 1 from button 1 in widget 2 - adding a layer disambiguating the window is just one more layer to this.

Which I guess is the root of my argument. Having a single ID namespace in an app means your IDs need to be unique across your app - but then, even in your proposal, inside a window, your IDs will need to be unique inside a window. Uniqueness is a requirement of identifiers. All we're discussing is how many root namespaces we're going to have.

Lookup by ID is, by definition, a little inefficient - if you want things to be fast and optimised, you should be using an imperative approach and retain direct links to the objects you're interested in. Micro-optimizing the length of identifiers because of one axis of uniqueness when there are multiple other axes of uniqueness that will need to be accommodated seems like an odd optimisation choice. This is especially true when the cost of this accommodation is that you lose the one benefit of having a single root namespace - unambiguous app-wide lookup by a single factor.

@mhsmith
Copy link
Member

mhsmith commented Oct 4, 2022

Thanks, I'll have a think about this and get back to you tomorrow.

@mhsmith
Copy link
Member

mhsmith commented Oct 5, 2022

OK, these are good points, so let's continue with the approach of #1599.

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. not quite right The idea or PR has been reviewed, but more work is needed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants