Desktop: Install default plugins on first-app-start #6585

mak2002 · 2022-06-16T15:55:23Z

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.

CalebJohn · 2022-06-16T16:24:41Z

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

laurent22 · 2022-06-20T11:04:03Z

packages/lib/services/plugins/PluginService.ts

@@ -416,7 +416,7 @@ export default class PluginService extends BaseService {
 		return this.installPluginFromRepo(repoApi, pluginId);
 	}

-	public async installPlugin(jplPath: string): Promise<Plugin> {
+	public async installPlugin(jplPath: string, defaultPlugin: boolean = false): Promise<Plugin> {


I'd prefer we keep this method generic, with no mention of default plugins. Why is this option needed?

If defaultPlugin is true, then we won't be loading the plugins right away after installing them. As we are installing default plugins before loading regular plugins, we just need to load them once along with regular plugins.

Ok then please name the option as loadPlugin, defaults to true

…joplin into install-default-plugins

laurent22 · 2022-06-23T14:23:39Z

packages/app-desktop/installDefaultPlugins.ts

+		}
+		Setting.setValue('defaultPlugins', service.serializePluginSettings(pluginSettings));
+		return pluginSettings;
+	} else { return null; }


Why return null here? Just return the unmodified PluginSettings? Then the called doesn't need to have logic to deal with either null or an object.

laurent22 · 2022-06-23T14:24:40Z

packages/app-desktop/installDefaultPlugins.ts

+			await service.installPlugin(defaultPluginPath, false);
+
+			pluginSettings = produce(pluginSettings, (draft: PluginSettings) => {
+				draft[plugin.path.replace('.jpl', '')] = defaultPluginSetting();


Please use lib/path to remove the file extension. It's more robust than a search and replace

laurent22 · 2022-06-23T14:27:37Z

packages/lib/services/plugins/PluginService.ts

@@ -416,7 +416,7 @@ export default class PluginService extends BaseService {
 		return this.installPluginFromRepo(repoApi, pluginId);
 	}

-	public async installPlugin(jplPath: string): Promise<Plugin> {
+	public async installPlugin(jplPath: string, loadPlugin: boolean = true): Promise<Plugin> {


Promise<Plugin | null>

laurent22 · 2022-06-24T10:59:34Z

packages/app-desktop/installDefaultPlugins.ts

+		}
+		Setting.setValue('defaultPlugins', service.serializePluginSettings(pluginSettings));
+		return pluginSettings;
+	} else { return pluginSettings; }


Maybe it's just me but generally I try to reduce indentation as much as possible, as it makes it easier to follow the logic. In this case, you could have an early exit at the top (if (!Setting.value('firstStart')) return pluginSettings;), and then unindent all the code below.

laurent22 · 2022-06-24T11:00:35Z

packages/lib/models/Setting.ts

+				public: true,
+				appTypes: [AppType.Desktop],
+				storage: SettingStorage.Database,
+			},


Any reason why this is a dynamic settings? Feels like it should just be a constant since it doesn't change for a given version?

You are right, It won't change for a given version. Are you suggesting I put it in some const variable? Because I think by putting defaultPlugins in Setting.ts it will be easier to retrieve it from anywhere.

I'd just put a defaultPlugins.ts file in lib/services/plugins. That way it can be easily included anywhere. Putting it in Setting.ts could cause dependency loops since this file is already included in many places.

Hey, I was thinking of making this defaultPlugins.ts as a class which will contain all the plugins' path, IDs etc. Do you think I should go ahead with this? Or should I just put it in PluginService.ts which is also in lib/services/plugins

If it's a constant, just make it constant? Anything else I don't know

mak2002 · 2022-06-25T16:27:55Z

packages/lib/models/Setting.ts

+			updatedDefaultPluginsInstallStates: {
+				value: false,
+				type: SettingItemType.Bool,
+				public: false,
+				appTypes: [AppType.Desktop],
+				storage: SettingStorage.File,
+			},


I think this won't cause a dependency loop. Please correct me if I am wrong.

laurent22 · 2022-06-26T17:00:32Z

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.

mak2002 · 2022-06-26T17:13:06Z

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.

laurent22 · 2022-06-26T17:35:55Z

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.

mak2002 · 2022-06-29T17:50:33Z

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

mak2002 · 2022-07-02T08:12:54Z

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 · 2022-07-11T03:27:17Z

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

laurent22

@mak2002, I'm struggling to provide guidance in your PRs because it's not small logic issues here and there - I feel some of it should be rearchitectured or rewritten.

I'm concerned about merging your code at this point as it's touching critical parts of the codebase.

laurent22 · 2022-07-22T09:04:10Z

packages/app-desktop/app.ts

@@ -273,6 +274,11 @@ class Application extends BaseApplication {
 		}

 		try {
+			const devEnv = Setting.constants_.env === Env.Dev;
+			let pluginsPath = '';
+			devEnv ? pluginsPath = path.join(__dirname, '..', 'app-desktop/build/defaultPlugins/') : pluginsPath = path.join(process.resourcesPath, 'build/defaultPlugins/');


use bridge().buildDir()

laurent22 · 2022-07-22T09:05:09Z

packages/lib/services/plugins/PluginService.ts

+		} else { return null; }
+	}
+
+	public async installDefaultPlugins(pathToPlugins: string, pluginSettings: PluginSettings, service: PluginService): Promise<PluginSettings> {


why you pass the service to itself...

Yes. It's not necessary. I will remove it.

laurent22 · 2022-07-22T09:06:15Z

packages/lib/services/plugins/PluginService.ts

+				draft[filename(plugin.path)] = defaultPluginSetting();
+			});
+		}
+		Setting.setValue('firstStart', 0);


Why you are setting firstStart here??

In fact this all firstStart is probably incorrect. It means that default plugins won't be enabled for existing installations. You need to have your own check to find out if a particular default plugin is already installed or not before installing it.

I have now removed the first-app-start logic and implemented my own check.

laurent22 · 2022-07-22T09:09:45Z

packages/lib/services/plugins/PluginService.ts

+	}
+
+	public setSettingsForDefaultPlugins(initialSettings: InitialSettings) {
+		Object.keys(initialSettings).forEach(pluginId => {


I don't see checks in here whether the settings are already set or not. It sounds like you'd overwrite user settings on each start.

Now, I have added checks here.

laurent22 · 2022-07-22T09:15:39Z

packages/lib/services/plugins/defaultPlugins.ts

+		settings[pluginId] = defaultPluginSetting();
+	});
+	return settings;
+}


I feel this function doesn't do anything useful. Also you are duplicating the list of default plugin IDs here and in the place where settings are defined. That list should only appear once in the codebase.

laurent22 · 2022-07-22T09:17:56Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

 	componentDidMount() {
 		if (this.props.defaultSection) {
 			this.setState({ selectedSectionName: this.props.defaultSection }, () => {
 				this.switchSection(this.props.defaultSection);
 			});
 		}
+		if (!Setting.value('updatedDefaultPluginsInstallStates')) {


Doesn't it mean the plugin settings will be set only once? How about when we add a new default plugin?

Now checking in installedDefaultPlugins array to see if new default plugin is available.

mak2002 · 2022-07-22T12:44:23Z

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.

…joplin into install-default-plugins

CalebJohn · 2022-07-25T15:17:14Z

packages/app-desktop/app.ts

+		} catch (error) {
+			this.logger().error(`There was an error loading default plugins from ${Setting.value('pluginDir')}:`, error);
+		}
+


Please move new functionality into a default Plugins module.

Ok. I was thinking of a way to only call function to install default plugins here, I will see what I can do.

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

CalebJohn · 2022-07-25T15:19:51Z

packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx

@@ -60,12 +61,28 @@ class ConfigScreenComponent extends React.Component<any, any> {
 		this.setState({ settings: this.props.settings });
 	}

+	updatePluginsStates(value: any) {


Please use a descriptive parameter name value should be newPluginStates. And please add typing.

packages/app-desktop/app.ts

CalebJohn · 2022-07-25T15:23:28Z

packages/lib/services/plugins/PluginService.ts

+	}
+
+	// this is used for setting initial "installed" state for plugins
+	public setInstalledState(): any {


Please rename to something descriptive (e.g. getDefaultPluginInstallState). And please add a return type.

CalebJohn · 2022-07-25T15:38:49Z

packages/lib/models/Setting.ts

+			setInitialDefaultPluginsSettings: {
+				value: [],
+				type: SettingItemType.Array,
+				public: false,
+				storage: SettingStorage.File,
+			},
+
+			preInstalledDefaultPlugins: {
+				value: '',
+				type: SettingItemType.Object,
+				public: false,
+				storage: SettingStorage.File,
+			},
+


Please remove these in favour of using the installedDefaultPlugins field alone.

mak2002 · 2022-07-31T10:41:44Z

packages/lib/services/plugins/defaultPlugins/desktopDefaultPluginsInfo.ts

+
+export const initialSettings: InitialSettings = {
+	'io.github.jackgruber.backup': {
+		'path': `${Setting.value('profileDir')}`,


I remember you suggested putting this into .config or json files, but we may need to use variables here, so that's why I put it in ts file.

Was there a discussion about this? Sorry if I missed it or if I'm repeating something but I think it's indeed ok to have the config directly in .ts file since it's not really dynamic (it's tied to the current build).

As previously discussed however we need all data related to the default plugins we support to be in the same place.

Was there a discussion about this?

I had discussion with my mentor CalebJohn about this. And I will make sure we put all the default plugins related data in the same place.

laurent22 · 2022-08-08T15:07:48Z

packages/app-cli/tests/services/plugins/defaultPluginsUtils.ts

@@ -0,0 +1,210 @@
+import { installDefaultPlugins, getDefaultPluginsInstallState, setSettingsForDefaultPlugins, checkPreInstalledDefaultPlugins } from '@joplin/lib/services/plugins/defaultPlugins/defaultPluginsUtils';
+import PluginRunner from '../../../app/services/plugins/PluginRunner';
+import * as fs from 'fs-extra';


Please don't import*. Only import the functions that you need

laurent22 · 2022-08-08T15:09:51Z

packages/lib/models/Setting.ts

+				value: [],
+				type: SettingItemType.Array,
+				public: false,
+				storage: SettingStorage.File,


Should be database here

laurent22 · 2022-08-08T15:11:15Z

packages/lib/models/Setting.ts

+	// this method checks if the 'value' passed is present in the Setting "Array"
+	// If yes, then it just returns 'true'. If its not present then, it will
+	// update it and return 'false'
+	static checkArrayAndUpdate(settingName: string, value: string): boolean {


maybe call it setArrayValue. Make the method public

laurent22 · 2022-08-08T15:15:01Z

packages/lib/services/plugins/defaultPlugins/desktopDefaultPluginsInfo.ts

+			'path': `${Setting.value('profileDir')}`,
+		},
+	};
+	return initialSettings;


Try to have a single object with all plugin related properties in there. For example:

export const defaultPlugins = { 'io.github.jackgruber.backup': { version: '1.0.2', settings: { 'path': `${Setting.value('profileDir')}`, }, 'plugin.calebjohn.rich-markdown': { version: '0.8.3', }, };

That way you have everything in one place and don't need to repeat the key for each piece of info you want to add.

This way it is a lot cleaner.

laurent22 · 2022-08-08T15:15:45Z

packages/lib/services/plugins/utils/manifestFromObject.ts

@@ -49,6 +49,7 @@ export default function manifestFromObject(o: any): PluginManifest {
 		permissions: permissions,

 		_recommended: getBoolean('_recommended', false, false),
+		_built_in: getBoolean('_built_in', false, false),


Is that used somewhere?

Not currently. But I thought maybe we can use it in the future. For example, if we decided to have a special badge for default plugins. But if you want, I will remove it.

laurent22 · 2022-08-27T11:02:45Z

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?

mak2002 · 2022-09-01T05:35:49Z

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.

laurent22 · 2022-09-01T10:44:20Z

Thanks for clarifying @mak2002, let's merge!

mak2002 · 2022-09-02T04:21:52Z

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.

CalebJohn · 2022-09-02T05:39:04Z

@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.

mak2002 added 2 commits June 16, 2022 21:12

initial commit

bd677c4

don't load default plugins immediately after installing

eb67977

CalebJohn marked this pull request as draft June 16, 2022 16:21

Merge branch 'dev' into install-default-plugins

302b380

laurent22 reviewed Jun 20, 2022

View reviewed changes

mak2002 added 3 commits June 21, 2022 19:14

fix installed state of default plugins

d8d86f2

Merge branch 'install-default-plugins' of https://github.com/mak2002/…

75d2601

…joplin into install-default-plugins

organize the code for installing default plugins

4491cf0

laurent22 reviewed Jun 23, 2022

View reviewed changes

improve types of functions

84c02c9

laurent22 reviewed Jun 24, 2022

View reviewed changes

add function for default Plugins settings

8d61611

mak2002 commented Jun 25, 2022

View reviewed changes

add _built_in field in plugins manifest

3c14807

mak2002 added 3 commits June 28, 2022 19:12

added test

191c8a5

implement initial settings for plugins

32c003b

removed code for mkdir command

3536393

mak2002 added 3 commits June 29, 2022 23:27

remove unnecessary files

4aa26bc

added tests for initial settings for default plugins

78c2ac7

remove async keyword from app.ts

46007c5

mak2002 marked this pull request as ready for review July 2, 2022 08:13

mak2002 added 2 commits July 6, 2022 23:26

Merge branch 'laurent22:dev' into install-default-plugins

ebde62b

Merge branch 'laurent22:dev' into install-default-plugins

4d05817

Merge branch 'laurent22:dev' into install-default-plugins

360de71

laurent22 reviewed Jul 22, 2022

View reviewed changes

Merge branch 'dev' into install-default-plugins

25820d3

mak2002 added 4 commits July 25, 2022 20:16

refactor code

b80c487

small fix

228aa96

Merge branch 'install-default-plugins' of https://github.com/mak2002/…

aa3a5fa

…joplin into install-default-plugins

remove old plugins

343c4de

CalebJohn requested changes Jul 25, 2022

View reviewed changes

mak2002 and others added 6 commits July 28, 2022 13:36

refactor code

b68fd07

Merge branch 'laurent22:dev' into install-default-plugins

fbc1793

move default plugins functions into separate folder | update tests

d1db3ec

remove test plugins

53ddafb

small fix

4cc80d1

add comment for checkArrayAndUpdate method

90dfd0d

mak2002 commented Jul 31, 2022

View reviewed changes

make function for initial settings to support profileDir

a4fcf8e

laurent22 reviewed Aug 8, 2022

View reviewed changes

mak2002 added 3 commits August 10, 2022 20:22

refactor plugins info object

6ef7c3d

fix import

5f6671d

update import in ConfigScreen

d90dee1

Merge branch 'laurent22:dev' into install-default-plugins

263d33b

laurent22 merged commit 01f4bb0 into laurent22:dev Sep 1, 2022

Daeraxa mentioned this pull request Sep 2, 2022

Desktop: Remove demo plugins folder and update pinned version of backup plugin #6801

Merged

Desktop: Install default plugins on first-app-start #6585

Desktop: Install default plugins on first-app-start #6585

Conversation

mak2002 commented Jun 16, 2022

CalebJohn commented Jun 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mak2002 Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mak2002 Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Jun 26, 2022

mak2002 commented Jun 26, 2022

laurent22 commented Jun 26, 2022

mak2002 commented Jun 29, 2022 • edited Loading

mak2002 commented Jul 2, 2022

mak2002 commented Jul 11, 2022

laurent22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mak2002 Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mak2002 Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

mak2002 commented Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Aug 27, 2022

mak2002 commented Sep 1, 2022

laurent22 commented Sep 1, 2022

mak2002 commented Sep 2, 2022 • edited Loading

CalebJohn commented Sep 2, 2022

mak2002 Jun 24, 2022 •

edited

Loading

mak2002 Jun 24, 2022 •

edited

Loading

mak2002 commented Jun 29, 2022 •

edited

Loading

mak2002 Jul 22, 2022 •

edited

Loading

mak2002 Jul 28, 2022 •

edited

Loading

mak2002 commented Jul 22, 2022 •

edited

Loading

mak2002 commented Sep 2, 2022 •

edited

Loading