Fix all ScreenManagers sharing the same transition #5370
Conversation
All ScreenManagers share the same SlideTransition instance by default, which can cause some timing issues and other weirdness (and is more generally a bad idea). This PR fixes the issue.
If the ScreenManager's "on_current" is called before self.transition is set, it will crash because on_current tries self.transition.stop(). This fixes the issue by forcing the transition to be set by the time event bindings are done.
kivy/uix/screenmanager.py
Outdated
| @@ -964,6 964,7 @@ def _get_screen_names(self): | |||
| ''' | |||
|
|
|||
| def __init__(self, **kwargs): | |||
| self.transition = SlideTransition() | |||
There was a problem hiding this comment.
I would suggest using:
if 'transition' not in kwargs:
kwargs['transition'] = SlideTransition()
This way if transition keyword is used in creating of ScreenManager instance then default SlideTransition instance doesn't get created.
There was a problem hiding this comment.
Agreed my good friend, thanks for the idea! :-)
| @@ -964,6 964,8 @@ def _get_screen_names(self): | |||
| ''' | |||
|
|
|||
| def __init__(self, **kwargs): | |||
| if 'transition' not in kwargs: | |||
| self.transition = SlideTransition() | |||
There was a problem hiding this comment.
I'd perhaps choose to rewrite only the kwargs part and not put it directly into the instance because the transition is a Kivy property and those are evaluated from kwargs in the super() call to any EventDispatcher instance afaik. However when tested, the assignment triggers on_transition correctly, sooo.. still looks good to me :)
Edit: ah, @pythonic64 apparently suggested the same approach, my bad.
There was a problem hiding this comment.
Yes, that's what he suggested, but I felt like we didn't need that much indirection to simply set a variable, and I discussed with tshirtman in meatspace and we reached the same conclusion, so there you have it :-) if you still feel setting kwargs is a better idea, I can amend my PR of course!
There was a problem hiding this comment.
Well, it doesn't break the on_* event, so it should be ok. I'll wait for the CI and merge.
|
Thanks 🍡 |
All ScreenManagers share the same SlideTransition instance by default, which can cause some timing issues and other weirdness (and is more generally a bad idea). This PR fixes the issue.