Skip to content

Fix ItemGroup/Launcher memory leak - #605

Open
lenemter wants to merge 1 commit into
mainfrom
lenemter/fix-itemgroup-launcher-mem-leak
Open

Fix ItemGroup/Launcher memory leak#605
lenemter wants to merge 1 commit into
mainfrom
lenemter/fix-itemgroup-launcher-mem-leak

Conversation

@lenemter

@lenemter lenemter commented Aug 22, 2026

Copy link
Copy Markdown
Member

Fixes a Launcher memory leak that was caused by not calling cleanup () from ItemGroup and by not freeing badge animation objects.

There's still a memory leak in WorkspaceIconGroup, I'll fix it in a separate branch.

Some other changes:

  • I renamed "cached items" into "items marked for removal" as it's easier to understand what it actually does.
  • Also I replaced Object.set_data with a 2 hash tables that convert items to their original objects and vice versa. I did this because I hate Object.set_data, and it's easy to make mistakes when using it (for example the code had item.get_data<Object> ("dock-obj"), but that key was never set)

@lenemter
lenemter requested a review from a team August 22, 2026 08:20

@leolost2605 leolost2605 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also I replaced Object.set_data with a 2 hash tables that convert items to their original objects and vice versa. I did this because I hate Object.set_data, and it's easy to make mistakes when using it (for example the code had item.get_data<Object> ("dock-obj"), but that key was never set)

Makes sense but can we do that in a separate commit?

I renamed "cached items" into "items marked for removal" as it's easier to understand what it actually does.

My initial idea was to always have a clean remove and add and those along with most of the methods never have to worry about if the item was already here or not that only happens via caching the items.
I did like it better because IMO it reduces the danger of messing up because you have a clean separation between the different operations.
I'm ofc open to arguments but in any case this should be a separate commit.

And IMO the nomenclature there makes sense because we do cache the item for a mainloop iteration after it has been removed and if it happens to appear again we reuse it.

Comment thread src/ItemGroup.vala
remove_items_id = 0;

foreach (var item in items_marked_for_removal.get_values ()) {
item.cleanup ();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of calling it here because the animation is still running and the item won't be disposed. Currently that's not a problem but it means we have to be more careful later about what we can and can't do. I would prefer it being called when the item then actually gets disposed.

However in general I think the best solution would be to avoid having a cleanup alltogether by having the animation targets act on a child widget and not on the baseitem itself, always using static functions for callbacks, etc. instead of stuffing more things in the cleanup method. I think we should work towards that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants