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

Animatable: fix transitions memory leak #12861

Merged

Conversation

DmitryZhelnin
Copy link
Contributor

@DmitryZhelnin DmitryZhelnin commented Sep 11, 2023

What does the pull request do?

Fixes memory leak in Animatable.

What is the current behavior?

Sometimes subscriptions to Transitions collection changes are leaked.

What is the updated/expected behavior with this PR?

Leaks are fixed.

How was the solution implemented (if it's not obvious)?

Subscription is made only if control is already attached to visual tree.

Fixed issues

Fixes #12860

Copy link

@damian-666 damian-666 left a comment

Choose a reason for hiding this comment

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

@kekekeks can you help me on this if im speaking out of turn or making wrong assumptions, i just making a very general suggestion and someone with a lot of history has to be a sort of code police and nnone wants to be that "fault finder".. but its for everyones benefit.

TDLR my suggestion are at the bottom : its jsut i've owned wpf code and its get to where sometime you want to rewrite it so unmanagleble and delicate .. .im there now.with my own wpf IDE project. and thats why im looking at avalonia again.

im new here but was in industry 15 years in Silicon valley and game tooling and we had alot of UI.. and otehr compandies including my own., with wpf tooling.. owning code just get more expensive as it grows .. i am seeing signs of code churn and too mjch state management.. ive watched in for 10 years its getting very amazing but.. its got 3 ways to do ui and 7 platorms thats amazing but.. its scary.

one thing i llook for is symmetry.. and redunancy.
noone wants to break anything so they tend to add fixes and state to represent the conditions under which the bug happens.. or leave existing state there when it should not be needed. but that adds exponential complexity. they dont wanna destroy anyones work .

the "dependency property "was added to UIElement when its not real data. so whenever possilbe shouid be used. (Please verify i dont know react)

also overdetermination.. means too many variables that can be a function or a query or a combination of another essential one(s) state , meaning its redundant . the best fix removes code especially state that represents a visualization.. thie wpf paradigm means bind to actual ( persistent) data members via properties, the lifetime of the data either as a setting or in a file.** .

its means careful review and wide understanding, but not in every case.. anyone can try to change a base class. if that class has an antipattern related to the bug(s) they wann fix. jsut test all the samples.. sometiems breaking fxing a thing , breaking 5 other things, is ok if you can fix all that by simplification unless you are close to a release or on a master branch. like this is..

_transitionsEnabled ? its his real data? is it needed? it it a dependency property.. is there another way to query it..

_isSubscribedToTransitionsCollection

newTransitions.CollectionChanged += TransitionsCollectionChangedHandler;
_isSubscribedToTransitionsCollection = true;

                can you just do  newTransitions.CollectionChanged -= TransitionsCollectionChangedHandler;    somehwere.. its not going to do anything if not subcribed to is own handler.. then remove _isSubscribedToTransitionsCollection     my staff idd that to fix leaks an i never have a problem..   if you unlisten twice its not a big deal... its not null..   however. on a massive grid it could be slow... i am not sure..   i
                
                 also i dont know OSS etiquette tell me when or where to be quiet .. im not invested in this yet.

@timunie
Copy link
Contributor

timunie commented Sep 12, 2023

@damian-666 we are totally aware of the need to refractor some areas. If you have followed the development of v11 vs 0.10.x, you probably have seen the many breaking changes we introduced. One of the motivation doing that was to get a more stable API while still allowing us to do needed refracturing later on. Still there are some area for improvement, so your contribution is welcome.

@damian-666
Copy link

Thanks, I looked  over the code in the base class and its seems like it's ok.    i'll be aware that you already bit the bullet  v10 to v11.   and il plan to build from the develop branch as things stabilize, and i probably will be on net8 or 9 by the time i really transtiion to this.. i watched on and off it since 2017

  some old apis can be marked  [deprecated]  or hide it  to completion .. spit a warning it they still use it.. that can save alot of suffering on clients.. thta use closed parts and cant update binaries..
 
i see teh
  OnPropertyChanged and I would be afraid to touch this, this type of nested listening in this mehtod, gives me intractable trouble in my wpf code.        the order of method calls will be different in different platforms  so its just reality.  
 
 
    -maybe- _isSubscribedToTransitionsCollection can be removed but

the two ways according to SE are puting -= then += to avoid double notification. or uisng the adding of the extra state as it was done here

https://stackoverflow.com/questions/3597950/how-can-i-check-if-an-event-has-been-subscribed-to-in-net

i like the business model, of this project, so i wont get burned by closed sources again.. rahter pay for consulting..to gurus

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0039507-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0039616-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 added this pull request to the merge queue Sep 19, 2023
Merged via the queue into AvaloniaUI:master with commit 9eeb3ee Sep 19, 2023
6 checks passed
@maxkatz6 maxkatz6 added the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Dec 7, 2023
@maxkatz6 maxkatz6 added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Jan 17, 2024
maxkatz6 pushed a commit that referenced this pull request Jan 17, 2024
…lready enabled. (#12861)

Co-authored-by: Dmitry Zhelnin <d.zhelnin@of-group.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Animatable: subscriptions to Transitions collection are leaked
5 participants