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.

Sign up for GitHub

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

Jump to bottom

Desktop: Resolves #6143: Add Installed Plugins in "About Joplin" #6357

Closed
wants to merge 11 commits into from
Closed

Desktop: Resolves #6143: Add Installed Plugins in "About Joplin" #6357

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
71e1465
Add Plugin Info to "About Joplin"
krishna8421 Apr 1, 2022
44dfbee
Added Test
krishna8421 Apr 2, 2022
81ed5d3
Added Dummy Plugins for Testing
krishna8421 Apr 2, 2022
408bc84
Remove Unwanted Tests
krishna8421 Apr 2, 2022
b5591d4
Merge branch 'laurent22:dev' into plugin-in-about
krishna8421 Apr 4, 2022
08f5fbc
Add test for checking plugins
krishna8421 Apr 4, 2022
e696aae
Merge branch 'dev' into plugin-in-about
laurent22 Apr 5, 2022
f65e91c
Merge branch 'laurent22:dev' into plugin-in-about
krishna8421 Apr 6, 2022
2ef6db8
Merge branch 'laurent22:dev' into plugin-in-about
krishna8421 Apr 11, 2022
124170c
Merge branch 'dev' into plugin-in-about
krishna8421 Apr 13, 2022
146e83c
Merge branch 'laurent22:dev' into plugin-in-about
krishna8421 Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1822,6 +1822,9 @@ packages/lib/uuid.js.map
packages/lib/versionInfo.d.ts
packages/lib/versionInfo.js
packages/lib/versionInfo.js.map
packages/lib/versionInfo.test.d.ts
packages/lib/versionInfo.test.js
packages/lib/versionInfo.test.js.map
packages/plugin-repo-cli/commands/updateRelease.d.ts
packages/plugin-repo-cli/commands/updateRelease.js
packages/plugin-repo-cli/commands/updateRelease.js.map
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1812,6 +1812,9 @@ packages/lib/uuid.js.map
packages/lib/versionInfo.d.ts
packages/lib/versionInfo.js
packages/lib/versionInfo.js.map
packages/lib/versionInfo.test.d.ts
packages/lib/versionInfo.test.js
packages/lib/versionInfo.test.js.map
packages/plugin-repo-cli/commands/updateRelease.d.ts
packages/plugin-repo-cli/commands/updateRelease.js
packages/plugin-repo-cli/commands/updateRelease.js.map
Expand Down
62 changes: 62 additions & 0 deletions packages/lib/versionInfo.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { _ } from './locale';
import { getPluginsArr, PluginInfo } from './versionInfo';
import PluginService from '@joplin/lib/services/plugins/PluginService';
import Plugin from '@joplin/lib/services/plugins/Plugin';

describe('versionInfo', function() {

it('should not push plugin to body if no plugin is found',() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what this test is doing.
If it's checking getPluginsArr then why do you need the rest remaining code ('if' and 'map')? If it's checking the logic inside the if then what's point of checking it when this code is only used here in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I'll remove that too.

const plugins = getPluginsArr(PluginService.instance().plugins);
const body = [];
if (plugins.length > 0) {
body.push(`\n${_('Plugins:')}`);
plugins.map((plugin)=>{
body.push(`${plugin.name}: ${plugin.version}`);
});
}
expect(body.length).toBe(0);
});

it('should return a empty array', () => {
const pluginsObjEmpty = {};
const plugins: PluginInfo[] = getPluginsArr(pluginsObjEmpty);
expect(plugins.length).toBe(0);
});

it('should return correct information about plugin', () => {
const TestPlugin1 = new Plugin(
'',
{
manifest_version: 1,
id: 'test-plugin-1',
name: 'TestPlugin1',
version: '0.1.2',
app_min_version: '1.0.0',
},
'',
()=>{},
''
);
const TestPlugin2 = new Plugin(
'',
{
manifest_version: 1,
id: 'test-plugin-2',
name: 'TestPlugin2',
version: '0.2.1',
app_min_version: '1.0.0',
},
'',
()=>{},
''
);

const plugins: PluginInfo[] = getPluginsArr({ TestPlugin1, TestPlugin2 });
expect(plugins.length).toBe(2);
expect(plugins[0].name).toBe('TestPlugin1');
expect(plugins[0].version).toBe('0.1.2');
expect(plugins[1].name).toBe('TestPlugin2');
expect(plugins[1].version).toBe('0.2.1');
});

});
Copy link
Owner

Choose a reason for hiding this comment

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

None of this feels very useful. getPluginsArr is just a "for" loop and we assume that loops work. What you should test instead is the versionInfo() function - make sure that when there are plugins, it includes them in the output of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll add it.

28 changes: 28 additions & 0 deletions packages/lib/versionInfo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,24 @@
import { _ } from './locale';
import Setting from './models/Setting';
import { reg } from './registry';
import PluginService, { Plugins } from '@joplin/lib/services/plugins/PluginService';

export type PluginInfo = {
name: string;
version: string;
};

// Get Plugins in Array Form
export const getPluginsArr = (pluginsObj: Plugins): PluginInfo[] => {
const plugins: PluginInfo[] = [];
for (const pluginId in pluginsObj) {
plugins.push({
name: pluginsObj[pluginId].manifest.name,
version: pluginsObj[pluginId].manifest.version,
});
}
return plugins;
};
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand the purpose of this function. You're looping on the array to create a new array. Then later you loop on this new array to create the plugin strings. So why not eliminate this intermediate step and loop on the first array to directly create the plugin strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PluginService.instance().plugins gives an object(with all plugins also as object) with id as the key, so I converted it to an array of objects and fill the array with only the name and version of the plugin.


export default function versionInfo(packageInfo: any) {
const p = packageInfo;
Expand All @@ -9,6 +27,8 @@ export default function versionInfo(packageInfo: any) {
gitInfo = _('Revision: %s (%s)', p.git.hash, p.git.branch);
if (p.git.branch === 'HEAD') gitInfo = gitInfo.slice(0, -7);
}


const copyrightText = 'Copyright © 2016-YYYY Laurent Cozic';
const now = new Date();

Expand All @@ -32,6 +52,14 @@ export default function versionInfo(packageInfo: any) {
console.info(gitInfo);
}

const plugins = getPluginsArr(PluginService.instance().plugins);
Copy link
Owner

Choose a reason for hiding this comment

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

From where versionInfo() is called, you already have the list of plugins in props.plugins, so you don't need to load the PluginService here. Just pass props.plugins to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

props.plugins returns this with type of PluginState:
Screenshot 2022-04-03 at 8 38 20 PM

if (plugins.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just move this code inside the getPluginsArr function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The gitInfo push to the body was down there so I thought to leave same types of items at a similar palace, also getPluginsArr just take the obj and return the array we need.

Should I change it?

body.push(`\n${_('Plugins:')}`);
plugins.map((plugin)=>{
body.push(`${plugin.name}: ${plugin.version}`);
});
}

return {
header: header.join('\n'),
body: body.join('\n'),
Expand Down