Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin config fix #7

Closed
wants to merge 3 commits into from
Closed

Conversation

scabala
Copy link
Contributor

@scabala scabala commented Sep 20, 2021

When using custom plugins, user-modified configuration is not saved in plugin's json file. It is caused by fact that instance of PluginConfigurationStore in plugin object and in each one of preference items are different objects. Therefore, when changing configuration in dialog and saving configuration from plugin, only default configuration is saved.

This PR fixes it by reusing plugins in preferences dialog instead of re-instantiating them each time dialog is executed.

his might be very ugly solution and I open for suggestions

@otsaloma
Copy link
Owner

Thanks for noticing, I now see the problem.

This solution doesn't work however, Window._plugins is only a list of active plugins. All plugins are needed for the preferences dialog. That's also why conf is a class attribute, because it needs to be available without instantiating the plugin.

A simpler fix would be to memoize util.load_plugin_module, but that needs some kind of handling not to break the :reload-plugins builtin command. If you want to look into it, go ahead, if you can't make sense of it, I can take a look too. The plugin config system turned out unfortunately complicated specifically because of having to deal with both active and inactive plugins and also in the code it's not always clear whether plugin is the name, class or instance or why it is what it is.

@scabala
Copy link
Contributor Author

scabala commented Sep 20, 2021

Ah, I should have know it would not be that easy. I'll try work with that a bit again.

@otsaloma
Copy link
Owner

I want to move on with this, I'm working on a fix now.

@otsaloma otsaloma closed this Sep 28, 2021
@otsaloma
Copy link
Owner

FYI

Should be fixed, though not tested thoroughly yet.

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.

3 participants