-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Animation: Fix kivy.animation.Sequence and kivy.animation.Parallel consistency #5926
Conversation
I wonder if |
Oh, Never mind, the function already exists in master. I'll let someone with animation usage review it. |
yep it override |
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.
Looks good to me, great that you added tests to cover the case, tests passed here and the change is straightforward, it's a bit sad to have duplication here, but maybe refactoring to remove it would make things more complicated (a new parent class?), i'm fine with that for now.
Hi, and thanks for the review.
yep and I didn't come up with any good idea. Should I fix that like this? class MoreNestedSequentialAnimationTestCase(unittest.TestCase): # changed base class
def setUp(self):
...
def tearDown(self):
...
def test_have_properties_to_animate(self): # test only this method
... And regardless of that, I'll fix def test_have_properties_to_animate(self):
self.assertFalse(self.a.have_properties_to_animate(self.w))
self.a.start(self.w)
self.assertTrue(self.a.have_properties_to_animate(self.w))
self.a.stop(self.w)
self.assertFalse(self.a.have_properties_to_animate(self.w)) which should be better than current code. |
I meant for the Sequence and Parallel to have a common method, (but i'm not sure which meaning of "like" you used here, if you meant "in the same way as", then ok). i.e:
i think is slightly better than having the exact same method in two classes. |
8d1d922
to
77062bc
Compare
ah I understand what you mean.
|
I'm analyzing several things, so don't merge this PR yet |
c9ba059
to
f4cab40
Compare
|
f4cab40
to
85f7f3e
Compare
Yes, there are certainly bugs left in them, thank you for looking into it. |
85f7f3e
to
fcc7641
Compare
np :) |
thanks |
kivy/animation.py
Outdated
@property | ||
def transition(self): | ||
# This property is impossible to implement | ||
raise NotImplementedError() |
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 think adding a message to the exception would be better, but that got me thinking, maybe we could just use AttributeError("Can't lookup transition attribute of a CompoundAnimation") since this is accessed as an attribute. What do you think?
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.
Yep I agree with adding a message to the exception
.
And about AttributeError, I can't decide because for me both looks fine. But according to the reference, AttributeError might be better.
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.
Because NotImplementedError
means real implementation required
, which is not our case.
so yeah AttibuteError is better I think.
fcc7641
to
801a2ab
Compare
dc9c08b
to
801a2ab
Compare
7436971
to
8f2db9a
Compare
@tshirtman Hi, is it better to |
Hey, if you are willing to, rebasing/updating would be great. |
Or maybe |
62a0341
to
2e2ea47
Compare
kivy/animation.py
Outdated
# for example: using ChainMap | ||
d = dict(self.anim1.animated_properties) | ||
d.update(self.anim2.animated_properties) | ||
return d |
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.
Might be better not to use ChainMap
. Because if we re-implement it like this:
@property
def animated_properties(self):
return ChainMap(self.anim2.animated_properties, self.anim1.animated_properties)
the user will be able to write anim2.animated_properties
. The following implementation might be better.
@property
def animated_properties(self):
return {**self.anim1.animated_properties, **self.anim2.animated_properties, }
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.
in both case it's an object that is returned to the user, that they can write to, but that doesn't mean they can write to animation.animated_properties, unless the property defines a setter.
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.
Oh sorry, I meant this part
Lookups search the underlying mappings successively until a key is found. In contrast, writes, updates, and deletions only operate on the first mapping.
in the doc.
To prevent the user from writing to the underlying dictionaries(anim1.animated_properties
and anim2.animated_properties
), we need to implement it like either of
@property
def animated_properties(self):
return {**self.anim1.animated_properties, **self.anim2.animated_properties, }
or
@property
def animated_properties(self):
return ChainMap({}, self.anim2.animated_properties, self.anim1.animated_properties)
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.
ok, that makes sense indeed. I guess the second one is a bit more efficient, as it doesn't unnecessarily copy data around, while still allowing to update the values for this animation (as they'll be saved in the first dict, they'll override values for the same key in the other dicts), and also they'll reflect updates to the animated properties of the underlying animations, in case they happen (and unless an overriding value has been put in the first dict, of course).
hm, if we can't then it's better not to document, i don't see how only firing for anim1 would be correct, ideally it would fire for both, and have the progress value take both durations into account (so if anim 1 is 3s and anim 2 is 2s, 1s into the first animation, we'd get progress at 0.2, 1/5th of the total), but i didn't check how hard it would be to implement that. |
Sounds like you are confusing
Ok, got it. |
🤦 you are right, maybe we could report based on the longest, but yeah, in any case, no need to think about that in this PR, it would be a new feature imho, to be evaluated independently.
👍 |
892e1b8
to
ba2b7f3
Compare
e42ff07
to
c32160e
Compare
@tshirtman
Yeah I agree with that way if we implement it. |
@tshirtman I mean Could you approve the PR? |
sorry i missed the update (again). I'll approve now. |
fix #5204 #5443 #5929