joplin icon indicating copy to clipboard operation
joplin copied to clipboard

Desktop: Install default plugins on first-app-start

Open mak2002 opened this issue 2 years ago • 8 comments

Hey, This is a draft PR for installing default plugins. To implement this, I am installing plugins before the code that loads and runs the plugins, as to avoid restarting.

One issue I noticed is that, after this installation is complete, if we searched any plugin that we installed, we still see the "Install' button instead of "Installed". But after restarting the app, it displays it correctly as ''Installed". I think we need to update pluginSettings in order to fix this. But apart from that I didn't notice any issue.

Feel free to give any suggestions you have.

mak2002 avatar Jun 16 '22 15:06 mak2002

@mak2002 For future reference, you can mark a PR as draft while creating it or after creating the PR

CalebJohn avatar Jun 16 '22 16:06 CalebJohn

Please try to make progress on this PR. In general we prefer not to keep draft PR for too long. If you're not sure what you need to do to finish please ask us.

laurent22 avatar Jun 26 '22 17:06 laurent22

Sorry for making slow progress. I had some college stuff going on last week, so that's why less activity. In the upcoming week, I will add tests and some initial settings for plugins. If there is anything other than this that I should do, please let me know.

mak2002 avatar Jun 26 '22 17:06 mak2002

Ok no problem, take your time in that case. The thing with draft PRs is that I stumble upon them... and I can't do much because it doesn't make sense to review if it's not finished (it already takes me forever to review PRs that are supposed to be finished). Perhaps we will not allow draft PRs in the future, especially during GSoC.

laurent22 avatar Jun 26 '22 17:06 laurent22

Hey, Now the only thing remaining is tests for defaultPluginsSettings.

mak2002 avatar Jun 29 '22 17:06 mak2002

Hey, The PR is ready to be reviewed now! I have included .jpl files of some plugins for now but will remove it before merging as there will be another script handling this part.

mak2002 avatar Jul 02 '22 08:07 mak2002

Tests are passing now. Don't know why they were failing randomly.

mak2002 avatar Jul 11 '22 03:07 mak2002

Hey @laurent22, Sorry for causing trouble for you. I think I have missed some cases like adding new default plugin, duplication of plugin IDs. I will fix issues which you mentioned above asap. I will make sure all of my code is ready to merge.

mak2002 avatar Jul 22 '22 12:07 mak2002

Thanks for the update, the fact that most of your code is now in a utility file makes this feature easier to manage.

Does your code now handle updates correctly?

  • What if a new default plugin is added? Does it get correctly added to the user's plugins?
  • Does it handle correctly existing user (who already have settings) and new users (who don't)?
  • What happens when a default plugin setting changes?
    • Does it get correctly set for the user?
    • What happens when the user has already set that setting to a different value?

laurent22 avatar Aug 27 '22 11:08 laurent22

  • What if a new default plugin is added? Does it get correctly added to the user's plugins?

Yes. The new plugin gets installed and added correctly to user's plugins.

  • Does it handle correctly existing user (who already have settings) and new users (who don't)?

Yes, It does. If the user already has some default plugins installed, then it will skip the installation and won't set default settings for them.

What happens when a default plugin setting changes?

  • Does it get correctly set for the user?
  • What happens when the user has already set that setting to a different value?

Do you mean if we change the default plugins settings from the code? If yes, then it won't affect users who already have default plugins installed. It will only affect the users who will be getting default plugins for first time.

mak2002 avatar Sep 01 '22 05:09 mak2002

Thanks for clarifying @mak2002, let's merge!

laurent22 avatar Sep 01 '22 10:09 laurent22

Hey @laurent22 , Can we revert this commit so that I can remove some plugin.jpl files that I had kept for demo purposes? Sorry for the inconvenience.

mak2002 avatar Sep 02 '22 04:09 mak2002

@mak2002 for now please just make a PR removing the files. That way Laurent can decide how he wants to proceed when he sees this.

CalebJohn avatar Sep 02 '22 05:09 CalebJohn