xfangfang / borealis

Hardware accelerated, Nintendo Switch inspired UI library for PC, Android, iOS, PSV, PS4 and Nintendo Switch
Apache License 2.0
31 stars 21 forks source link

Fix: RecyclerDataSource memory leak #57

Closed ThisALV closed 8 months ago

ThisALV commented 8 months ago

RecyclerDataSource wasn't deleted

xfangfang commented 8 months ago

Look good to me, I know this issue, but I didn't fix it because I'm afraid some applications may rely on this behavior, and maintain the dataSource themselves. Before merging, I would like to ask @PoloNX @dragonflylee and @XITRIX if this will affect them.


btw, I am using another list in my own app, which supports variable height views or grid layout based on RecyclerList. If anyone is interested, I will synchronize it here.

https://github.com/xfangfang/wiliwili/blob/yoga/wiliwili/include/view/recycling_grid.hpp

XITRIX commented 8 months ago

To be honest I completely forgot the reason of why I commented it out, may be we could add bool releaseDataSourceOnDeinit with true by default, in case someone would like to maintain datasource's lifecycle manually

PoloNX commented 8 months ago

To be honest I completely forgot the reason of why I commented it out, may be we could add bool releaseDataSourceOnDeinit with true by default, in case someone would like to maintain datasource's lifecycle manually

It’s a good idea. I don’t need to maintain it personally

dragonflylee commented 8 months ago

maybe https://github.com/ITotalJustice/borealis/commit/842d4b290286c821065f0680610502edb1eb4d2e is another workaround solution

ThisALV commented 8 months ago

maybe ITotalJustice@842d4b2 is another workaround solution

Yup seems to work. Now I remember when I de-commented those lines I had segfault on dropdown item selection and I didn't understand why. I'll add another commit to address the issue by applying ITotalJustice fix to the PR.

ThisALV commented 8 months ago

Okay I've done the commit, now there's an optional bool parameter on setDataSource allowing to prevent data source deletion by the RecyclerFrame and Dropdown constructor uses it.

PoloNX commented 8 months ago

Okay I've done the commit, now there's an optional bool parameter on setDataSource allowing to prevent data source deletion by the RecyclerFrame and Dropdown constructor uses it.

If it defaults to true. Is it a breaking change?