Attachments (3)
Change History (23)
by , 3 years ago
Attachment: | 21360.patch added |
---|
comment:1 by , 3 years ago
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.
comment:2 by , 3 years ago
Cc: | added |
---|
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:
- Preset preferences -> Add preset Allergy
- Configure toolbar and add the preset Allergy|Baker icon
- Verify that the icon (and thus the preset) works as expected
- 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.
follow-up: 5 comment:3 by , 3 years ago
How does this work with plugin actions?
First I thought sure, go ahead, but the more I think about the trickier it gets:
- Install some presets
- Nicely configure a customized toolbar
- Dislike the order of groups in preset menu
- 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.
follow-up: 6 comment:4 by , 3 years ago
The toolbar should listen to TaggingPresetListener::taggingPresetsModified
and update itself accordingly.
comment:5 by , 3 years ago
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.
comment:6 by , 3 years ago
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.
by , 3 years ago
Attachment: | 21360-v3.patch added |
---|
comment:7 by , 3 years ago
Cc: | added |
---|
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.
follow-up: 9 comment:8 by , 3 years ago
Probably just clearing actions
in ToolbarPreferences.loadActions
would suffice. actions
and regactions
indeed look kind of redundant, and actions
never gets properly cleared.
comment:9 by , 3 years ago
Replying to marcello@…:
Probably just clearing
actions
inToolbarPreferences.loadActions
would suffice.actions
andregactions
indeed look kind of redundant, andactions
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.
follow-up: 11 comment:10 by , 3 years ago
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.
comment:11 by , 3 years ago
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 makesactions
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.
comment:12 by , 3 years ago
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.
comment:13 by , 3 years ago
OK if I commit my patch as is and you add further changes as a new patch?
comment:17 by , 3 years ago
comment:18 by , 3 years ago
@gaben: Is this about EasyPresets? Please open a new ticket and give more details.
minimal patch to reduce memory leak