Memory leaks with TaggingPreset

[GerdP]

This is a follow up of ticket:21360#comment:5.

[GerdP]

minimal patch to reduce memory leak

[GerdP]

I don't yet understand why ToolbarPreferences keeps references to all actions (and thus also presets) that were ever loaded in field actions. This makes it impossible to free memory for presets.

[GerdP]

also refresh presets in toolbar

[GerdP]

What should happen when a preset is removed for which an icon exists in the toolbar? The current code behaves a bit strange when this is done. Example steps to reproduce:

1. Preset preferences -> Add preset Allergy
2. Configure toolbar and add the preset Allergy|Baker icon
3. Verify that the icon (and thus the preset) works as expected
4. Preset preferences -> Remove the preset Allergy

Note that the baker icon still exists and the preset still works. Only after a restart the icon is removed. I would expect that the preset and the icon are removed immediately after step 4.

[skyper]

How does this work with plugin actions?

First I thought sure, go ahead, but the more I think about the trickier it gets:

1. Install some presets
2. Nicely configure a customized toolbar
3. Dislike the order of groups in preset menu
4. Delete presets and add them in different order

As far as I know, icons are only reloaded on startup which does not help here, either.

[marcello@…]

The toolbar should listen to TaggingPresetListener::taggingPresetsModified and update itself accordingly.

[GerdP]

Replying to skyper:

How does this work with plugin actions?

Plugins are not unloaded. If you disable a plugin it is just not loaded on the next start.
The toolbar actions are stored in preference toolbar. This is just a collection of strings. If method ToolbarPreferences.refreshToolbarControl() is called, all "known" actions are parsed and the toolbar is refreshed. If an action is missing the list in toolbar is not changed, the icon is just not added to the toolbar.

[GerdP]

Replying to marcello@…:

The toolbar should listen to TaggingPresetListener::taggingPresetsModified and update itself accordingly.

Yes. I've already added the listener in my patch. I just don't know for sure what changes this should trigger.

[GerdP]

I think this is ready to commit.

• Memory leaks are fixed
• Toolbar icon disappears when corresponding preset is removed
• Toolbar icon reappears when corresponding preset is added

@Dirk: I still don't fully understand the purpose of the two maps actions and regactions in ToolbarPreferences. regactions was introduced for #2091. The committed code changes are more complex than the patches which were attached to the ticket and I have no idea yet if that was intended.

[marcello@…]

Probably just clearing actions in ToolbarPreferences.loadActions would suffice. actions and regactions indeed look kind of redundant, and actions never gets properly cleared.

[GerdP]

Replying to marcello@…:

Probably just clearing actions in ToolbarPreferences.loadActions would suffice. actions and regactions indeed look kind of redundant, and actions never gets properly cleared.

Do you mean instead of other code in my patch or in addition? Please attach a patch. My experience with dialogs is minimal.

[marcello@…]

It looks like actions is initialized from the menu and it is needed only for the toolbar preferences dialog. I don't see any reason to keep it around after the preferences dialog is closed. So I propose a patch that makes actions local to the dialog.

[stoecker]

Replying to marcello@…:

It looks like actions is initialized from the menu and it is needed only for the toolbar preferences dialog. I don't see any reason to keep it around after the preferences dialog is closed. So I propose a patch that makes actions local to the dialog.

I think that's right. In the beginning each action had to be registered. As that wasn't always done and probably never will be done somewhen a menu-scanning was added. These scanned entries very likely aren't relevant outside the dialog, but maybe they are required when loading/saving the config and handling the toolbar entries? Must be checked.

Actions can also be registered when not in the menu (and menu entries can be unregistered), so these two aren't identical, but they can have a lot of common entries.

[marcello@…]

Having two things (actions, regactions) that do almost the same is probably a bad idea. In fact actions is hogging memory right now. Shouldn't the actions in the menu be registered when the presets menu is created and unregistered when it is destroyed?

Actually unregistering menu actions won't work because the two maps are merged in ToolbarPreferences.getDefinedActions() and that is also one reason the button still shows after the preset is unregistered from regactions. To get rid of a preset button you have to clean actions and regactions, as you are doing in the patch.

That leaves me unconvinced of the necessity of having actions at all, since we can reconstruct it from the menu any time we want.

[GerdP]

OK if I commit my patch as is and you add further changes as a new patch?

[marcello@…]

Please go ahead. If I find a better patch I will submit it later.

[GerdP]

In 18321/josm:

fix #21446: Memory leaks with TaggingPreset

• don't always add ActiveLayerChangeListener in TaggingPreset constructor, only needed for some instances
• let ToolbarPreferences implement TaggingPresetListener to update toolbar and free (most) refs to presets when a preset was uninstalled

[Don-vip]

Milestone renamed

[gaben]

There is an issue with r18321, not all actions registering after the patch. I noticed this behaviour while tested my plugin. Compare the toolbar settings in r18319 and r18321. In the latter at least four core and possibly all plugin actions are missing.

[GerdP]

@gaben: Is this about EasyPresets? Please open a new ticket and give more details.

[gaben]

Not related to EasyPresets. See new ticket #21730.

[GerdP]

r18321 caused a regression, see #21740