-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Conversation
@mak2002 For future reference, you can mark a PR as draft while creating it or after creating the PR |
@@ -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> { |
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.
I'd prefer we keep this method generic, with no mention of default plugins. Why is this option needed?
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.
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.
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.
Ok then please name the option as loadPlugin
, defaults to true
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.
Done.
} | ||
Setting.setValue('defaultPlugins', service.serializePluginSettings(pluginSettings)); | ||
return pluginSettings; | ||
} else { return null; } |
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.
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.
await service.installPlugin(defaultPluginPath, false); | ||
|
||
pluginSettings = produce(pluginSettings, (draft: PluginSettings) => { | ||
draft[plugin.path.replace('.jpl', '')] = defaultPluginSetting(); |
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.
Please use lib/path to remove the file extension. It's more robust than a search and replace
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.
Done.
@@ -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> { |
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.
Promise<Plugin | null>
} | ||
Setting.setValue('defaultPlugins', service.serializePluginSettings(pluginSettings)); | ||
return pluginSettings; | ||
} else { return pluginSettings; } |
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.
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.
packages/lib/models/Setting.ts
Outdated
public: true, | ||
appTypes: [AppType.Desktop], | ||
storage: SettingStorage.Database, | ||
}, |
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.
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?
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.
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.
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.
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.
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.
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
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.
If it's a constant, just make it constant? Anything else I don't know
packages/lib/models/Setting.ts
Outdated
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 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.
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. |
Sorry for making slow progress. I had some college stuff going on last week, so that's why less activity. |
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. |
Hey, Now the only thing remaining is tests for defaultPluginsSettings. |
Hey, The PR is ready to be reviewed now! |
Tests are passing now. Don't know why they were failing randomly. |
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.
@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.
packages/app-desktop/app.ts
Outdated
@@ -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/'); |
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()
} 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It's not necessary. I will remove it.
draft[filename(plugin.path)] = defaultPluginSetting(); | ||
}); | ||
} | ||
Setting.setValue('firstStart', 0); |
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.
Why you are setting firstStart here??
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.
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 comment
The 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.
} | ||
|
||
public setSettingsForDefaultPlugins(initialSettings: InitialSettings) { | ||
Object.keys(initialSettings).forEach(pluginId => { |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Now, I have added checks here.
settings[pluginId] = defaultPluginSetting(); | ||
}); | ||
return settings; | ||
} |
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.
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.
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 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?
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.
Now checking in installedDefaultPlugins
array to see if new default plugin is available.
Hey @laurent22, |
packages/app-desktop/app.ts
Outdated
} catch (error) { | ||
this.logger().error(`There was an error loading default plugins from ${Setting.value('pluginDir')}:`, error); | ||
} | ||
|
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.
Please move new functionality into a default Plugins module.
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.
Ok. I was thinking of a way to only call function to install default plugins here, I will see what I can do.
@@ -60,12 +61,28 @@ 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 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.
} | ||
|
||
// this is used for setting initial "installed" state for plugins | ||
public setInstalledState(): any { |
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.
Please rename to something descriptive (e.g. getDefaultPluginInstallState
). And please add a return type.
packages/lib/models/Setting.ts
Outdated
setInitialDefaultPluginsSettings: { | ||
value: [], | ||
type: SettingItemType.Array, | ||
public: false, | ||
storage: SettingStorage.File, | ||
}, | ||
|
||
preInstalledDefaultPlugins: { | ||
value: '', | ||
type: SettingItemType.Object, | ||
public: false, | ||
storage: SettingStorage.File, | ||
}, | ||
|
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.
Please remove these in favour of using the installedDefaultPlugins
field alone.
|
||
export const initialSettings: InitialSettings = { | ||
'io.github.jackgruber.backup': { | ||
'path': `${Setting.value('profileDir')}`, |
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.
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.
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.
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.
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.
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.
@@ -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'; |
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.
Please don't import*
. Only import the functions that you need
packages/lib/models/Setting.ts
Outdated
value: [], | ||
type: SettingItemType.Array, | ||
public: false, | ||
storage: SettingStorage.File, |
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.
Should be database here
packages/lib/models/Setting.ts
Outdated
// 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 { |
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.
maybe call it setArrayValue
. Make the method public
'path': `${Setting.value('profileDir')}`, | ||
}, | ||
}; | ||
return initialSettings; |
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.
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.
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.
This way it is a lot cleaner.
@@ -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 comment
The 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 comment
The 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.
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?
|
Yes. The new plugin gets installed and added correctly to user's plugins.
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.
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. |
Thanks for clarifying @mak2002, let's merge! |
Hey @laurent22 , Can we revert this commit so that I can remove some |
@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. |
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.