Desktop: Added options to set default templates #2857

johodh · 2020-03-22T21:36:24Z

Added two new dropdowns under Settings -> Note. One for setting default note template, and one for default todo template. Options for both are read from templateDir and default option is None. Chosen templates appear as expected on ctrl+N and ctrl+T
“File -> Templates -> New note from template” of course overrides this, so that item is still intact even if a default template is chosen. Inserting templates also works like before.
The Setting.value(‘availableTemplates’) is only updated when shim.isElectron = True, and otherwise defaults to an empty array.

Known issues:

Default templates are read from Setting.value('availableTemplates')[optionnumber].value when creating new note/todo. If the current default template, or a template with lower index number than the current one gets deleted/moved from template folder and templates are refreshed, the wrong template will be read on New note. Maybe not a huge problem, but i think it would be cleaner if it defaulted back to "None" at least when current is deleted. Haven't found a good way to solve this yet.

ζ npm run test -- --filter=Setting                                                              [897d9cc2] 

> joplin@1.0.161 test /home/jo/Workspace/js/johodh-joplin/CliClient
> gulp buildTests -L && jasmine --config=tests/support/jasmine.json "--filter=Setting"

Testing with sync target: memory
Randomized with seed 64661
Started
.


Ran 1 of 265 specs
1 spec, 0 failures
Finished in 0.162 seconds
Randomized with seed 64661 (jasmine --random=true --seed=64661)

ElectronClient/app.js

ElectronClient/gui/MainScreen.jsx

ReactNativeClient/lib/models/Setting.js

johodh · 2020-03-24T19:46:56Z

Couple of rookie mistakes on my end, as i expected :) Good points.

There is one minor problem left regarding the deletion of templates. On delete -> refresh the dropdowns go to "None", but Setting.value('default****') still equals the value of the deleted template which means you're stuck with them on new note/todo. To get past that as a user you have to change to another template and apply. If you delete all your template when a default is set i think you'd need to load in new templates to stop them from showing.

I've attempted a solution for this by matching defaultNote and defaultTodo with the templates array on refresh/startup, which seems to work. I'll push it in the coming commit.

johodh · 2020-03-24T20:31:34Z

ElectronClient/app.js

 				const templates = await TemplateUtils.loadTemplates(Setting.value('templateDir'));
-				if (shim.isElectron()) Setting.setValue('availableTemplates', templates);
+				Setting.setValue('availableTemplates', templates);
+
+				setTimeout(() => {
+					if (!templates.filter(i => i.value == Setting.value('defaultNote')).length) Setting.setValue('defaultNote', '0');
+					if (!templates.filter(i => i.value == Setting.value('defaultTodo')).length) Setting.setValue('defaultTodo', '0');
+				}, 2000);


This appears twice. Would making it into a function inside templateUtils be ok?

Actually I think it should be pushed into MainScren.jsx That way it will happen right away when a user deletes a template.

Also I think Array.find Might be a bit cleaner.

Ah, didn't know about Array.find. I'll try it out.

I've tried putting this test inside Mainscreen createNewNote,. Problem is it seems to need a timeout of about 1-2 sec for the templates array not to turn up blank (which corrupts the test), and i want the test to determine if template should be null or not. I guess just moving the current tests to createNewNote would work. Drawback is the deleted template would still appear on the first new note/todo and then not.

That 1-2 sec is really weird, and I suspect it won't be needed if you move the code to createNewNote. Keep in mind that you can access the templates in MainScreen by using
this.props.templates

Still not sure why it only worked with timeout at first. I might have messed up syntax when testing without or something. Anyway, seems to work as expected when put in createNewNote now!

CalebJohn · 2020-03-26T19:24:23Z

I'm sorry to do this, but I'm afraid that I gave you a bad suggestion earlier. Using the template value as the Setting key doesn't work well because if the user edits a template then the default will be lost, which is not good. The better behavior would be to index the options with the label, since this will stay stable

options[templates[i].label] = templates[i].label;

then in createNewNote you will have to fetch the body from this.props.templates

johodh · 2020-03-26T20:20:21Z

I'm sorry to do this, but I'm afraid that I gave you a bad suggestion earlier. Using the template value as the Setting key doesn't work well because if the user edits a template then the default will be lost, which is not good. The better behavior would be to index the options with the label, since this will stay stable
options[templates[i].label] = templates[i].label;
then in createNewNote you will have to fetch the body from this.props.templates

Good point and no worries, hadn't thought of that myself. Editing works without reset on latest commit.

johodh · 2020-04-23T09:08:00Z

@CalebJohn Anything else i should do here?

CalebJohn · 2020-04-23T21:29:12Z

Hi @johodh Sorry for going dark on you here. This LGTM, but we have a semi-official feature freeze happening now. All the bug fixing pull requests are being priortized now, and this will have to come after that, sorry. Thanks for following up.

laurent22 · 2020-05-09T09:59:08Z

Thank you for the pull request @johodh, I'm slowly trying to go through our backlog. The implementation looks good overall, but I have an issue with the availableTemplates setting variable because it's redundant data. We can already know the list of available templates by getting them from the file system so we don't need to duplicate this info in the settings.

So my suggestion would be to add a new method to TemplateUtils called availableTemplates() and a corresponding class property availableTemplates_. Whenever loadTemplate is called, you cache the result to availableTemplates_ and then you can call availableTemplates() from the settings.

johodh · 2020-05-10T17:42:13Z

@laurent22 Thanks for the feedback! That does seem like a better idea. I hope the last commit should do it.

Btw, I was able to read TemplateUtils.availableTemplates_ in Setting as well, so i'm curious if the method is really needed. Also, can i do something about the failed checks? They seemed to be related to mustache and not my changes.

laurent22 · 2020-05-14T20:58:24Z

ReactNativeClient/lib/models/Setting.js

+					const templates = TemplateUtils.availableTemplates();
+					const options = { 0: 'None' };
+					for (let i = 0; i < templates.length; i++) {
+						options[templates[i].label] = templates[i].label;
+					}
+					return options;


Identifying the template by array index is too fragile because if a template is added or deleted it will break. They should be identified by name.

The templates are identified by label in the options array. Label is the same as file name (the index loop is just for iterating through all available template objects). I've played around with this a bit, and if a template file is deleted joplin defaults back to no template on next reload or joplin restart. Adding new template files also does not mess with the currently seleced template.

laurent22 · 2020-05-14T20:58:53Z

ReactNativeClient/lib/models/Setting.js

+					const templates = TemplateUtils.availableTemplates();
+					const options = { 0: 'None' };
+					for (let i = 0; i < templates.length; i++) {
+						options[templates[i].label] = templates[i].label;
+					}
+					return options;


Please refactor to avoid the duplicate code. Maybe create a function defaultTemplateOptions() somewhere

laurent22 · 2020-05-14T20:59:48Z

ReactNativeClient/lib/models/Setting.js

@@ -346,6 +347,40 @@ class Setting extends BaseModel {
 					};
 				},
 			},
+			defaultNote: {


Please rename to editor.defaultNoteTemplate and editor.defaultTodoTemplate

laurent22 · 2020-05-14T21:00:00Z

ReactNativeClient/lib/models/Setting.js

@@ -346,6 +347,40 @@ class Setting extends BaseModel {
 					};
 				},
 			},
+			defaultNote: {
+				value: '0',


Defaults to empty string ''

laurent22 · 2020-05-14T21:00:44Z

ReactNativeClient/lib/models/Setting.js

+				isEnum: true,
+				public: true,
+				appTypes: ['desktop'],
+				label: () => _('Choose default template for new note:'),


'New note default template' and 'New to-do default template'

laurent22 · 2020-05-14T21:03:14Z

ElectronClient/gui/MainScreen.jsx

+			if (!template) {
+				const templateType = isTodo ? 'defaultTodo' : 'defaultNote';
+				const settingsLabel = Setting.value(templateType);
+				if (settingsLabel != 0) {
+					const matchLabel = this.props.templates.find(i => i.label == settingsLabel);
+					if (!matchLabel) {
+						Setting.setValue(templateType, '0');
+					} else {
+						template = matchLabel.value;
+					}
+				}
+			}


This ought to be simpler:

if (!template) { template = Setting.value(isTodo ? 'defaultTodo' : 'defaultNote');` }

Please don't set any setting in here, as we don't want side-effects.

I realized the Setting.setValue isn't necessary. I put it there for safety, thinking the set defaultTodo/defaultNote options might remain after (deleting a template) and refreshing. They don't, so i'll remove it.

I'll also cut down line 103 and 104 to one line like you propose. Only the template filename is stored in Setting.value(default****), not the contents, so we still need to get the contents from props. This change was made as Caleb realized storing the template contents in options won't allow the user to edit a template without losing the default.

Sorry, i was wrong about the first part. The settings do remain even after refresh, which leads to a problem when deleting templates and refreshing. I'll see what i can do.

laurent22 · 2020-05-14T21:08:17Z

Btw, I was able to read TemplateUtils.availableTemplates_ in Setting as well, so i'm curious if the method is really needed.

By convention properties that end with "_" are private and should not be accessed from elsewhere.

laurent22 · 2020-05-14T21:08:37Z

Also please could you add tests for this feature?

laurent22 · 2020-05-14T21:12:07Z

Also, can i do something about the failed checks? They seemed to be related to mustache and not my changes.

Hmm, right I think we have a problem here because the Setting class is now loading the TemplateUtil class, even though that's not needed for the tests (or for the CLI and mobile app).

I think there's some plumbing to do to allow the Setting class to access other classes properly, probably using dependency injection. So could you put this feature on hold for now please, until this is sorted out? Sorry about that, I didn't realise it would be a problem for this pull request.

johodh · 2020-05-17T16:12:17Z

I'll look in to the requested changes, add tests and push a new commit for you to review. Please have a look at my response on your first comment. Should i do something in particular to put the feature on hold, or do i just leave it here as is until the plumbing is sorted out? (I'm new to github :))

johodh · 2020-05-27T18:18:17Z

ElectronClient/gui/MainScreen.jsx

+				const templateFileName = Setting.value(isTodo ? 'editor.defaultTodoTemplate' : 'editor.defaultNoteTemplate');
+				if (templateFileName) {
+					if (typeof(this.props.templates.find(i => i.label == templateFileName)) != 'undefined') {
+						template = this.props.templates.find(i => i.label == templateFileName).value;


I got rid of Setting.setValue(...).

After deleting a template and then refreshing templates the Setting.value('editor.default****Template') still returns the name of the deleted file, however the dropdown in settings will default back to 'None'. I made an if (typeof()) test to stop errors getting thrown on new note when a default template has been deleted.
This also means that if you re-add the deleted template it will be selected as default again, until you actively change to something else in the settings panel.

johodh · 2020-05-27T18:38:14Z

Can't seem to run any tests without running in to the Mustache-problem.

..joplin/CliClient <master ✘ [?]>                                                                                 (20:34:07)  
ζ npm test                                                                                                                                 [46e68221] 

> joplin@1.0.161 test ..joplin/CliClient
> gulp buildTests -L && jasmine --config=tests/support/jasmine.json

internal/modules/cjs/loader.js:979
  throw err;
  ^

Error: Cannot find module 'mustache'
Require stack:
- ..joplin/CliClient/tests-build/lib/TemplateUtils.js
- ..joplin/CliClient/tests-build/lib/models/Setting.js
- ..joplin/CliClient/tests-build/lib/models/BaseItem.js
- ..joplin/CliClient/tests-build/lib/models/Resource.js
- ..joplin/CliClient/tests-build/lib/joplin-database.js
- ..joplin/CliClient/tests-build/test-utils.js
- ..joplin/CliClient/tests-build/ArrayUtils.js
- ..joplin/CliClient/node_modules/jasmine/lib/jasmine.js
- ..joplin/CliClient/node_modules/jasmine/bin/jasmine.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:976:15)
    at Function.Module._load (internal/modules/cjs/loader.js:859:27)
    at Module.require (internal/modules/cjs/loader.js:1036:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at Object.<anonymous> (..joplin/CliClient/tests-build/lib/TemplateUtils.js:3:18)
    at Module._compile (internal/modules/cjs/loader.js:1147:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1167:10)
    at Module.load (internal/modules/cjs/loader.js:996:32)
    at Function.Module._load (internal/modules/cjs/loader.js:896:14)
    at Module.require (internal/modules/cjs/loader.js:1036:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '..joplin/CliClient/tests-build/lib/TemplateUtils.js',
    '..joplin/CliClient/tests-build/lib/models/Setting.js',
    '..joplin/CliClient/tests-build/lib/models/BaseItem.js',
    '..joplin/CliClient/tests-build/lib/models/Resource.js',
    '..joplin/CliClient/tests-build/lib/joplin-database.js',
    '..joplin/CliClient/tests-build/test-utils.js',
    '..joplin/CliClient/tests-build/ArrayUtils.js',
    '..joplin/CliClient/node_modules/jasmine/lib/jasmine.js',
    '..joplin/CliClient/node_modules/jasmine/bin/jasmine.js'
  ]
}
npm ERR! Test failed.  See above for more details

laurent22 · 2020-06-17T17:30:37Z

What I meant by plumbing is that certain core features are currently missing to be able to implement this, so for now I would prefer to close the pull request. When these core features are implemented we can look at it again.

johodh · 2020-06-17T21:15:00Z

What I meant by plumbing is that certain core features are currently missing to be able to implement this, so for now I would prefer to close the pull request. When these core features are implemented we can look at it again. <

I see. I wish i could help you out with that, but i suspect i still have too much to learn at this point to get into coding core features. I'd be happy to finish this PR later on.

johodh added 2 commits March 22, 2020 22:11

Added options to set default templates

897d9cc

Small fix

d33fe5e

CalebJohn reviewed Mar 24, 2020

View reviewed changes

ElectronClient/app.js Outdated Show resolved Hide resolved

CalebJohn reviewed Mar 24, 2020

View reviewed changes

ElectronClient/gui/MainScreen.jsx Outdated Show resolved Hide resolved

CalebJohn reviewed Mar 24, 2020

View reviewed changes

ReactNativeClient/lib/models/Setting.js Outdated Show resolved Hide resolved

After review changes and delete template fix

693040f

johodh commented Mar 24, 2020

View reviewed changes

Moved delete check inside createNewNote

dcf0a4b

Fix to support editing of current default templ.

b6b53e8

laurent22 added the high High priority issues label Apr 30, 2020

availableTemplates_ now prop of TemplateUtils

979f8e2

laurent22 requested changes May 14, 2020

View reviewed changes

Requested changes

46e6822

johodh commented May 27, 2020

View reviewed changes

laurent22 closed this Jun 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Added options to set default templates #2857

Desktop: Added options to set default templates #2857

johodh commented Mar 22, 2020 •

edited

johodh commented Mar 24, 2020

johodh Mar 24, 2020

CalebJohn Mar 24, 2020

CalebJohn Mar 24, 2020

johodh Mar 25, 2020

CalebJohn Mar 25, 2020

johodh Mar 26, 2020

CalebJohn commented Mar 26, 2020

johodh commented Mar 26, 2020

johodh commented Apr 23, 2020

CalebJohn commented Apr 23, 2020

laurent22 commented May 9, 2020

johodh commented May 10, 2020

laurent22 May 14, 2020

johodh May 17, 2020

laurent22 May 14, 2020

laurent22 May 14, 2020

laurent22 May 14, 2020

laurent22 May 14, 2020

laurent22 May 14, 2020

johodh May 27, 2020 •

edited

johodh May 27, 2020

laurent22 commented May 14, 2020

laurent22 commented May 14, 2020

laurent22 commented May 14, 2020

johodh commented May 17, 2020

johodh May 27, 2020

johodh commented May 27, 2020

laurent22 commented Jun 17, 2020

johodh commented Jun 17, 2020

Desktop: Added options to set default templates #2857

Desktop: Added options to set default templates #2857

Conversation

johodh commented Mar 22, 2020 • edited

johodh commented Mar 24, 2020

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

CalebJohn commented Mar 26, 2020

johodh commented Mar 26, 2020

johodh commented Apr 23, 2020

CalebJohn commented Apr 23, 2020

laurent22 commented May 9, 2020

johodh commented May 10, 2020

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

johodh May 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented May 14, 2020

laurent22 commented May 14, 2020

laurent22 commented May 14, 2020

johodh commented May 17, 2020

Choose a reason for hiding this comment

johodh commented May 27, 2020

laurent22 commented Jun 17, 2020

johodh commented Jun 17, 2020

johodh commented Mar 22, 2020 •

edited

johodh May 27, 2020 •

edited