Fixfor#227 - Githubissues

stefanofornari commented 1 year ago

related to PR #301 (sorry if there is a way to update a PR, I am not very familiar with the flow ... :( )

I took into account all comments. Regarding https://github.com/palexdev/MaterialFX/pull/301#discussion_r1141344615, I think I found an elegant solution that takes into account changes in content and when content is null. Please note the demo seems not triggering the listener set in line contentProperty().addListener( and I don't have a unit test to test it.

I tested with the demo app and it works.

stefanofornari commented 1 year ago

Ciao Alex, I have some time on Tuesday, if you have any comments by then I'll work on the them right away.

palexdev commented 1 year ago

Thanks! However it looks good to me, I just need to find some free time to work on it

palexdev commented 1 year ago

Merged, but I'll have to make some changes, I think you made it worse than the previous haha You are adding the listeners in the buildScene() method, which is called every time the content changes, you basically added a potential memory leak ^^'

stefanofornari commented 1 year ago

Merged, but I'll have to make some changes, I think you made it worse than the previous haha You are adding the listeners in the buildScene() method, which is called every time the content changes, you basically added a potential memory leak ^^'

I do not think I have added a leak because the listeners are removed when the content changes (lines 169-170). Am I missing something?

palexdev commented 1 year ago

You were removing the listeners from the content sizes (oldValure.remove...) but in the builldScene method you were adding them on the Window sizes

stefanofornari commented 1 year ago

Ups, that's very true, good catch. And actually you are making me realize those lines can be removed. Even more, the whole point of content being null does not make sense with this implementation. For the same reason I still do not see the leak. I will provide an update.

palexdev commented 1 year ago

Ups, that's very true, good catch. And actually you are making me realize those lines can be removed. Even more, the whole point of content being null does not make sense with this implementation. For the same reason I still do not see the leak. I will provide an update.

As it is implemented now no, there's no need to take into account the possibility of a null content, but in your very first PR it was needed. Ah, crap you are right, I fixed your PR but totally forgot to remove those two lines, well it's not a big deal, nothing will happen, better remember that though

The leak was there though:

You create the dialog, listeners are added by buildScene(...)
Content changes, listeners are removed on the content not the dialog (as we said before)
buildScene(...) is invoked again for the new content
Listeners are added to the Window sizes... again As you can see every time the content was changing the listeners were added again again and again on the Window sizes

No need for an update Stefano, I already fixed it. I just need to remember to remove those two useless lines in the content property listener

stefanofornari commented 1 year ago

Got it! I thought they would have been replaced being always the same instance.

palexdev commented 1 year ago

I may be wrong, but I think they use Lists to hold listeners, handlers and such, which means that they can contain the same instance multiple time Then to call the registered listeners/handlers they basically use a for loop, which means if you have the same listener multiple time...performance issues and memory leak

palexdev / MaterialFX

Fixfor#227 #303