-
-
Notifications
You must be signed in to change notification settings - Fork 685
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
MicroPython compatibility #2976
base: main
Are you sure you want to change the base?
Conversation
OK, MicroPython is now able to import all the Toga modules that would be required to support Invent's current features. Actually testing those modules on MicroPython can wait until a future PR. Main changes:
|
I've tried various search terms, to no avail. What's Invent? |
https://github.com/invent-framework/invent It's a PyScript-based GUI framework created by one of our colleagues at Anaconda. How exactly we'll be working with them is TBD, but getting at least a subset of Toga to run on MicroPython is a prerequisite. |
Huh, interesting. Seems like it's currently an orthogonal alternative to what Toga does. I guess it might potentially use the Toga web backend instead of its current internal web implementation? Or Toga's web backend might use Invent? |
There may still be two separate backends, since toga-web provides a more traditional app structure with menus, toolbars and dialogs, while Invent is focused on more web-based idioms. But hopefully at least the widget and layout APIs can be shared between them. |
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.
A couple of Qs inline, but largely looks good.
My biggest concern is the extent to which we're going to degrade the non-MicroPython developer experience to satisfy the constraints of MicroPython - I've flagged a couple of them; the /
usage in the protocol definitions is another.
import sys | ||
|
||
if sys.implementation.name != "cpython": # pragma: no cover | ||
from . import compat # noqa: F401 |
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.
Idle thought - should the compat module name be based on sys.implementation.name
in some way? I don't have any idea which other implementations we'd be concerned about, but at the very least it would be clear from the namespace that toga.compat.micropython
is a micropython compatibility shim.
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.
Since there won't be any other implementations in the foreseeable future, let's cross that bridge when we come to it. I see this compat
package as a temporary stopgap anyway – virtually all of it should eventually be upstreamed to micropython-lib.
# internal use, but those methods shouldn't be used by end-users. | ||
|
||
def __init__(self, *args: Any, **kwargs: Any): | ||
self._registry = WeakValueDictionary(*args, **kwargs) | ||
self._registry = dict(*args, **kwargs) |
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.
We need to be careful that this doesn't introduce leaks elsewhere. This was a WeakValueDict historically to ensure that there wasn't a reference loop between the ObjC and Python objects; that might not be an issue any more given the work done on the window/widget lifecycle, plus the in-flight Rubicon reference handling changes - but we should confirm this is true.
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 registry was made a WeakValueDictionary in #2066 because at that time it stored references to all widgets which existed. But then #2320 and #2517 tightened that up to include only widgets which are attached to a window which has not yet been closed.
That means that if a widget is in the registry, it must also be reachable via a chain of strong references from App.windows
. So I don't believe the weakref functionality is being used anymore – widgets are being removed from the registry before their weakrefs expire.
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.
I suspect you're correct here - and the memory leak tests in the testbed should confirm this.
self, | ||
types=[] if document_types is None else document_types, | ||
) | ||
self._document_types = [] if document_types is None else document_types |
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.
I get why this has been done, but I don't love it as an approach - it feels a bit too much like the properties of one particular medium making the ergonomics for every other medium materially worse. At this "proof of concept" stage I can live with it; but as a long term solution, I'd rather see a dummy DocumentSet
implementation that is used when the platform doesn't support documents.
@@ -344,13 +334,11 @@ def __init__( | |||
# We need the command set to exist so that startup et al. can add commands; | |||
# but we don't have an impl yet, so we can't set the on_change handler | |||
self._commands = CommandSet() | |||
self._status_icons = StatusIconSet() |
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.
Similar to DocumentSet
- I don't love it, but I can live with this for now.
Are you OK to merge this now? Even if we're not actively using MicroPython yet, it would be useful to have the CI job to make sure we maintain this initial level of compatibility. |
Work in progress for making Toga sufficiently compatible with MicroPython to work with Invent.
PR Checklist: