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

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

Merged
merged 34 commits into from
Sep 1, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
bd677c4
initial commit
mak2002 Jun 16, 2022
eb67977
don't load default plugins immediately after installing
mak2002 Jun 16, 2022
302b380
Merge branch 'dev' into install-default-plugins
laurent22 Jun 18, 2022
d8d86f2
fix installed state of default plugins
mak2002 Jun 21, 2022
75d2601
Merge branch 'install-default-plugins' of https://github.com/mak2002/…
mak2002 Jun 21, 2022
4491cf0
organize the code for installing default plugins
mak2002 Jun 23, 2022
84c02c9
improve types of functions
mak2002 Jun 24, 2022
8d61611
add function for default Plugins settings
mak2002 Jun 25, 2022
3c14807
add _built_in field in plugins manifest
mak2002 Jun 25, 2022
191c8a5
added test
mak2002 Jun 28, 2022
32c003b
implement initial settings for plugins
mak2002 Jun 29, 2022
3536393
removed code for mkdir command
mak2002 Jun 29, 2022
4aa26bc
remove unnecessary files
mak2002 Jun 29, 2022
78c2ac7
added tests for initial settings for default plugins
mak2002 Jul 2, 2022
46007c5
remove async keyword from app.ts
mak2002 Jul 2, 2022
ebde62b
Merge branch 'laurent22:dev' into install-default-plugins
mak2002 Jul 6, 2022
4d05817
Merge branch 'laurent22:dev' into install-default-plugins
mak2002 Jul 10, 2022
360de71
Merge branch 'laurent22:dev' into install-default-plugins
mak2002 Jul 13, 2022
25820d3
Merge branch 'dev' into install-default-plugins
mak2002 Jul 22, 2022
b80c487
refactor code
mak2002 Jul 25, 2022
228aa96
small fix
mak2002 Jul 25, 2022
aa3a5fa
Merge branch 'install-default-plugins' of https://github.com/mak2002/…
mak2002 Jul 25, 2022
343c4de
remove old plugins
mak2002 Jul 25, 2022
b68fd07
refactor code
mak2002 Jul 28, 2022
fbc1793
Merge branch 'laurent22:dev' into install-default-plugins
mak2002 Jul 30, 2022
d1db3ec
move default plugins functions into separate folder | update tests
mak2002 Jul 31, 2022
53ddafb
remove test plugins
mak2002 Jul 31, 2022
4cc80d1
small fix
mak2002 Jul 31, 2022
90dfd0d
add comment for checkArrayAndUpdate method
mak2002 Jul 31, 2022
a4fcf8e
make function for initial settings to support profileDir
mak2002 Jul 31, 2022
6ef7c3d
refactor plugins info object
mak2002 Aug 10, 2022
5f6671d
fix import
mak2002 Aug 10, 2022
d90dee1
update import in ConfigScreen
mak2002 Aug 11, 2022
263d33b
Merge branch 'laurent22:dev' into install-default-plugins
mak2002 Aug 30, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/app-desktop/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import checkForUpdates from './checkForUpdates';
import { AppState } from './app.reducer';
import syncDebugLog from '@joplin/lib/services/synchronizer/syncDebugLog';
import eventManager from '@joplin/lib/eventManager';
import installDefaultPlugins from './installDefaultPlugins';
// import { runIntegrationTests } from '@joplin/lib/services/e2ee/ppkTestUtils';

const pluginClasses = [
Expand Down Expand Up @@ -261,7 +262,7 @@ class Application extends BaseApplication {
service.initialize(packageInfo.version, PlatformImplementation.instance(), pluginRunner, this.store());
service.isSafeMode = Setting.value('isSafeMode');

const pluginSettings = service.unserializePluginSettings(Setting.value('plugins.states'));
let pluginSettings = service.unserializePluginSettings(Setting.value('plugins.states'));

{
// Users can add and remove plugins from the config screen at any
Expand All @@ -273,6 +274,7 @@ class Application extends BaseApplication {
}

try {
pluginSettings = await installDefaultPlugins(pluginSettings, service);
if (await shim.fsDriver().exists(Setting.value('pluginDir'))) {
await service.loadAndRunPlugins(Setting.value('pluginDir'), pluginSettings);
}
Expand Down
Binary file not shown.
Binary file not shown.
14 changes: 14 additions & 0 deletions packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,26 @@ class ConfigScreenComponent extends React.Component<any, any> {
this.setState({ settings: this.props.settings });
}

updatePluginsStates(value: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const key = 'plugins.states';
const md = Setting.settingMetadata(key);
shared.updateSettingValue(this, key, value);

if (md.autoSave) {
shared.scheduleSaveSettings(this);
}
}

CalebJohn marked this conversation as resolved.
Show resolved Hide resolved
componentDidMount() {
if (this.props.defaultSection) {
this.setState({ selectedSectionName: this.props.defaultSection }, () => {
this.switchSection(this.props.defaultSection);
});
}
if (!Setting.value('updatedDefaultPluginsInstallStates')) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mak2002 mak2002 Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.updatePluginsStates(Setting.value('defaultPlugins'));
Setting.setValue('updatedDefaultPluginsInstallStates', true);
}
}

private async handleSettingButton(key: string) {
Expand Down
22 changes: 22 additions & 0 deletions packages/app-desktop/installDefaultPlugins.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import Setting from '@joplin/lib/models/Setting';
import shim from '@joplin/lib/shim';
import PluginService, { defaultPluginSetting, PluginSettings } from '@joplin/lib/services/plugins/PluginService';
import path = require('path');
import { filename } from '@joplin/lib/path-utils';
import produce from 'immer';

export default async function installDefaultPlugins(pluginSettings: PluginSettings, service: PluginService): Promise<PluginSettings> {
if (Setting.value('firstStart')) {
const defaultPlugins = await shim.fsDriver().readDirStats(path.join(__dirname, '..', 'app-desktop/build/defaultPlugins/'));
for (const plugin of defaultPlugins) {
const defaultPluginPath: string = path.join(__dirname, '..', `app-desktop/build/defaultPlugins/${plugin.path}`);
await service.installPlugin(defaultPluginPath, false);

pluginSettings = produce(pluginSettings, (draft: PluginSettings) => {
draft[filename(plugin.path)] = defaultPluginSetting();
});
}
Setting.setValue('defaultPlugins', service.serializePluginSettings(pluginSettings));
return pluginSettings;
} else { return pluginSettings; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
23 changes: 23 additions & 0 deletions packages/lib/models/Setting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1504,6 +1504,22 @@ class Setting extends BaseModel {
storage: SettingStorage.Database,
},

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


defaultPlugins: {
value: {},
type: SettingItemType.Object,
public: true,
appTypes: [AppType.Desktop],
storage: SettingStorage.Database,
},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mak2002 mak2002 Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mak2002 mak2002 Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


lastSettingDefaultMigration: {
value: -1,
type: SettingItemType.Int,
Expand All @@ -1516,6 +1532,13 @@ class Setting extends BaseModel {
public: false,
},

// isFirstAppStart: {
// value: true,
// type: SettingItemType.Bool,
// storage: SettingStorage.File,
// public: false,
// },

// 'featureFlag.syncAccurateTimestamps': {
// value: false,
// type: SettingItemType.Bool,
Expand Down
10 changes: 6 additions & 4 deletions packages/lib/services/plugins/PluginService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 | null> {
logger.info(`Installing plugin: "${jplPath}"`);

// Before moving the plugin to the profile directory, we load it
Expand All @@ -429,9 +429,11 @@ export default class PluginService extends BaseService {
await shim.fsDriver().copy(jplPath, destPath);

// Now load it from the profile directory
const plugin = await this.loadPluginFromPath(destPath);
if (!this.plugins_[plugin.id]) this.setPluginAt(plugin.id, plugin);
return plugin;
if (loadPlugin) {
const plugin = await this.loadPluginFromPath(destPath);
if (!this.plugins_[plugin.id]) this.setPluginAt(plugin.id, plugin);
return plugin;
} else { return null; }
}

private async pluginPath(pluginId: string) {
Expand Down