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

Fixed early access of _impl from style application #2919

Closed
wants to merge 3 commits into from

Conversation

HalfWhitt
Copy link
Contributor

Fixes #2916

It turns out a change I made in beeware/travertino#148 unmasked an underlying bug in Toga. BaseStyle.copy used to contain this:

for style in self._PROPERTIES[self.__class__]:
    try:
        setattr(dup, style, getattr(self, f"_{style}"))
    except AttributeError:
        pass

It's only trying catch the error if the style attribute isn't set on itself. But then I changed it to:

dup.update(**self)

And the above errors show up. It was also catching lookup errors when a style tries to apply itself too early, before an implementation has been assigned.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@HalfWhitt
Copy link
Contributor Author

HalfWhitt commented Oct 18, 2024

What I've changed makes the bugs go away, but I'm not positive about the spelling. Should Pack be reaching in and looking for _impl like I've done? Or would it make more sense for Applicator to report a ready property (or similar)?

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a huge problem with pack reaching into the node looking for an impl; my bigger concern is what else this might be masking.

It might amount to nothing; but the implication is that any style being applied before the node is fully initialised won't be applied onto the newly created widget.

So - my question is what is triggering the apply() calls at that point in style's lifecycle? Is there anything meaningful being lost here, or is it an incidental update Can we avoid that update being applied?

Alternatively - could this be addressed by deferring the assignment of the applicator until we have a widget that is ready to have styles applied to it?

And, in either case - do we need to trigger an "apply all existing styles" pass, either when the node is "ready", or when the applicator is assigned?

@HalfWhitt
Copy link
Contributor Author

HalfWhitt commented Oct 18, 2024

Okay, I think I figured this out better!

Node's initializer copies the style provided to it:

class Node:
    def __init__(self, style, applicator=None, children=None):
        self.applicator = applicator
        self.style = style.copy(applicator)

And BaseStyle.copy() (both before and after my overhaul) applies the applicator before setting the properties on the new copy:

def copy(self, applicator=None):
    "Create a duplicate of this style declaration."
    dup = self.__class__()
    dup._applicator = applicator
    for style in self._PROPERTIES.get(self.__class__, set()):
        try:
            setattr(dup, style, getattr(self, "_%s" % style))
        except AttributeError:
            pass
    return dup
def copy(self, applicator=None):
    "Create a duplicate of this style declaration."
    dup = self.__class__()
    dup._applicator = applicator
    dup.update(**self)
    return dup

This means it's making calls out to apply, before the node/widget's initializer is even done.

Swapping the order of those two lines means that even leaving Toga as it is, Pack will check against self._applicator and won't try and access the implementation during the copy.

So I can submit that over in Travertino, and close this.

However, this raises another question. When creating a widget/node and supplying the style parameter, the resulting widget has a copy of that style, and that copy has the widget's applicator. But if you set the style attribute after the fact, no copying or applicator-setting is done.

>>> style = Pack(text_align="center")
>>> widget = toga.Widget(style=style)
>>> widget.style is style
False
>>> widget.style._applicator is widget.applicator
True
>>> widget.style = style
>>> widget.style is style
True
>>> widget.style._applicator is None
True

I definitely don't 100% grok all the intended interactions among the components of the overall style/layout mechanism, but that seems off to me. Should Node.style be a property that copies and sets applicator when assigned?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'ExampleWidget' object has no attribute '_impl' when using main branch Travertino
2 participants