-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Changes from 18 commits
bd677c4
eb67977
302b380
d8d86f2
75d2601
4491cf0
84c02c9
8d61611
3c14807
191c8a5
32c003b
3536393
4aa26bc
78c2ac7
46007c5
ebde62b
4d05817
360de71
25820d3
b80c487
228aa96
aa3a5fa
343c4de
b68fd07
fbc1793
d1db3ec
53ddafb
4cc80d1
90dfd0d
a4fcf8e
6ef7c3d
5f6671d
d90dee1
263d33b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import SyncTargetRegistry from '@joplin/lib/SyncTargetRegistry'; | |
const shared = require('@joplin/lib/components/shared/config-shared.js'); | ||
import ClipperConfigScreen from '../ClipperConfigScreen'; | ||
import restart from '../../services/restart'; | ||
import getDefaultPluginSettings from '@joplin/lib/services/plugins/defaultPlugins'; | ||
const { KeymapConfigScreen } = require('../KeymapConfig/KeymapConfigScreen'); | ||
|
||
const settingKeyToControl: any = { | ||
|
@@ -60,12 +61,26 @@ class ConfigScreenComponent extends React.Component<any, any> { | |
this.setState({ settings: this.props.settings }); | ||
} | ||
|
||
updatePluginsStates(value: any) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use a descriptive parameter name |
||
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')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now checking in |
||
this.updatePluginsStates(getDefaultPluginSettings()); | ||
Setting.setValue('updatedDefaultPluginsInstallStates', true); | ||
} | ||
} | ||
|
||
private async handleSettingButton(key: string) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1534,6 +1534,14 @@ class Setting extends BaseModel { | |
storage: SettingStorage.Database, | ||
}, | ||
|
||
updatedDefaultPluginsInstallStates: { | ||
value: false, | ||
type: SettingItemType.Bool, | ||
public: false, | ||
appTypes: [AppType.Desktop], | ||
storage: SettingStorage.File, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
lastSettingDefaultMigration: { | ||
value: -1, | ||
type: SettingItemType.Int, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import Setting from '../../models/Setting'; | |
import Logger from '../../Logger'; | ||
import RepositoryApi from './RepositoryApi'; | ||
import produce from 'immer'; | ||
import path = require('path'); | ||
const compareVersions = require('compare-versions'); | ||
const uslug = require('@joplin/fork-uslug'); | ||
|
||
|
@@ -28,6 +29,14 @@ export interface Plugins { | |
[key: string]: Plugin; | ||
} | ||
|
||
export interface SettingAndValue { | ||
[settingName: string]: string; | ||
} | ||
|
||
export interface InitialSettings { | ||
[pluginId: string]: SettingAndValue; | ||
} | ||
|
||
export interface PluginSetting { | ||
enabled: boolean; | ||
deleted: boolean; | ||
|
@@ -68,6 +77,12 @@ export default class PluginService extends BaseService { | |
return this.instance_; | ||
} | ||
|
||
public initialSettings: InitialSettings = { | ||
'io.github.jackgruber.backup': { | ||
'path': '/JoplinBackup', | ||
}, | ||
}; | ||
|
||
private appVersion_: string; | ||
private store_: any = null; | ||
private platformImplementation_: any = null; | ||
|
@@ -106,6 +121,10 @@ export default class PluginService extends BaseService { | |
}; | ||
} | ||
|
||
public get initialPluginsSettings(): InitialSettings { | ||
return this.initialSettings; | ||
} | ||
|
||
private deletePluginAt(pluginId: string) { | ||
if (!this.plugins_[pluginId]) return; | ||
|
||
|
@@ -416,7 +435,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 | ||
|
@@ -429,9 +448,35 @@ 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; } | ||
} | ||
|
||
public async installDefaultPlugins(pathToPlugins: string, pluginSettings: PluginSettings, service: PluginService): Promise<PluginSettings> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why you pass the service to itself... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. It's not necessary. I will remove it. |
||
if (!Setting.value('firstStart')) return pluginSettings; | ||
const defaultPlugins = await shim.fsDriver().readDirStats(pathToPlugins); | ||
|
||
for (const plugin of defaultPlugins) { | ||
const defaultPluginPath: string = path.join(`${pathToPlugins}/${plugin.path}`); | ||
await service.installPlugin(defaultPluginPath, false); | ||
|
||
pluginSettings = produce(pluginSettings, (draft: PluginSettings) => { | ||
draft[filename(plugin.path)] = defaultPluginSetting(); | ||
}); | ||
} | ||
Setting.setValue('firstStart', 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why you are setting firstStart here?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have now removed the first-app-start logic and implemented my own check. |
||
return pluginSettings; | ||
} | ||
|
||
public setSettingsForDefaultPlugins(initialSettings: InitialSettings) { | ||
Object.keys(initialSettings).forEach(pluginId => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now, I have added checks here. |
||
Object.keys(initialSettings[pluginId]).forEach((setting) => { | ||
Setting.setValue(`plugin-${pluginId}.${setting}`, initialSettings[pluginId][setting]); | ||
}); | ||
}); | ||
} | ||
|
||
private async pluginPath(pluginId: string) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { defaultPluginSetting, PluginSettings } from './PluginService'; | ||
|
||
const defaultPluginsId = ['io.github.jackgruber.backup', 'plugin.calebjohn.rich-markdown']; | ||
|
||
export default function getDefaultPluginSettings(): PluginSettings { | ||
const settings: PluginSettings = {}; | ||
defaultPluginsId.map((pluginId: string) => { | ||
settings[pluginId] = defaultPluginSetting(); | ||
}); | ||
return settings; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that used somewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}; | ||
|
||
validatePluginId(manifest.id); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use bridge().buildDir()