-
-
Notifications
You must be signed in to change notification settings - Fork 712
Create proxy fix #3345
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
base: main
Are you sure you want to change the base?
Create proxy fix #3345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix you've provided here looks like it resolves the issue; however, I'm not completely convinced it's the right approach.
Taking the solution "as is" - I'm not wild about the name change. You've taken a method called addEventListener
and rebound it to the name create_proxy
- which is a method name that also exists in the pyscript.ffi
namespace. That means create_proxy()
is (a) no longer doing what the built-in create_proxy
method does, and (b) doesn't accurately describe what it does do, either. If we were going down this path, I'd be inclined to make a utility add_event_listener()
method (possibly as member method on the widget class).
However, I'm not completely convinced this is needed. The ffi
methods you're using here are low level pyodide methods; and one of the goals of PyScript is to provide a higher-level API that abstracts those methods and details behind a more Pythonic API.
To that end - Pyscript even provides a wrapper around create_proxy
- but immediately advises that it shouldn't ever be needed if you're using the pyscript API as intended.
In particiular, the pyscript.web.*
APIs (additional user guide here) provide a way to create any DOM element, and presumably it also "does the right thing" when it comes to attaching handlers - that would seem to be the intention of the when()
method/decorator.
So - can we avoid the need to call pyodide
internal APIs entirely by using the pyscript API exclusively? Can we replace Widget._create_dom_element
with a native Pyscript call?
Briefcase is currently using PyScript 2024.11.1; if we need to switch to a newer version of PyScript to get bug fixes and updates, then that's absolutely something we can and should do. This is also something that would fit into the bigger piece of work of making Briefcase indepdendent of the specific needs of Toga - that is, the Pyscript and Shoelace version dependencies are dependencies of Toga, and should be defined there, not defined as part of Briefcase.
One other detail - when you're proposing a change that is purely code, and doesn't contain a change to tests, it's worth flagging how you've tested this yourself. If the code has tests (or a pre-existing, but failing test), then the "yes, this works" criteria is obvious; but when it's just code change, it's not necessarily obvious how you've established that code is working.
In this case, my go-to was the passwordinput
example in the examples folder... and it won't work because (a) it's currently missing a travertino
dependency, and (b) the dom_onchange
implementation on PasswordInput is currently passing in a None
argument whenever it is fired. You might not have hit the former problem (addressed in #3346) if you rolled your own test case - but I'm not sure how you could have tested any change event handling on PasswordInput and not hit the second problem.
Grabbing changes from upstream
updating branch
How would you suggest going about this. I'm assuming it would require coordinating PRs across both Toga and Briefcase to achieve this. Just want to make sure that I've understood this correctly. Also any tips or good to know things before i start would be greatly appreciated |
When Toga uses Briefcase, it's always using the "development" version; so as soon as any changes are made upstream, those changes will be used by Toga. And since the testing story for the web backend is weak, there's no chicken-and-egg coordination problem - there's very few automated tests that need to pass before we can merge changes to the web template, so we don't need to worry about fixing Toga so that the template change will pass automated testing so that we can merge a PR so that Toga can use new features :-) The two possible places a change might be required are in Briefcase itself, and in https://github.com/beeware/briefcase-web-static-template - the template that is used to generate Web code. The minimalist fix will be to update (2) to point at the newly required version. Assuming there's no other large configuration changes, this may be all you need to do. If you want to test things out, you can clone the template repository, then add The intermediate fix would be to decouple the PyScript version from Briefcase entirely. To do this, you'd modify the web template to make the Pyscript version a template variable, and use that template variable in the template (with a value of The comprehensive fix would be to resurrect the parts of #1945 and beeware/briefcase#1285 that would allow Toga to specify the version of Pyscript (and any other dependencies, such as Shoelace) that it requires. Those PRs allow a wheel to specify "inserts" which will be processed by Briefcase, and inserted into In terms of a pragmatic approach, I'd be entirely comfortable with the minimalist approach right now, as that gets you unstuck on the immediate issue of updating Pyscript API usage. However, I'd like to think you and the team would be able to take a swing at the intermediate or comprehensive fix as part of the work that you're doing improving BeeWare's web tooling. |
Improve event binding across web back-end by replacing the old create_proxy() with the pyodide.ffi.wrappers.add_event_listener(), this has been done in the libs module.
This affects any web widgets using DOM events such as blur, focus, change etc
Changes has been made to the passwordinput widget as well as the window module to make use of this change
New usage is now
create_proxy(dom_element,"event-type",python_handler)
e.g.
create_proxy(self.native,"sl-blur",self.dom_onblur)
Resolves issues related to garbage-collected borrowed proxies that show in the console.
Background
Brought to my attention in my previous pull request
PR Checklist: