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: Added options to set default templates #2857

Closed
wants to merge 7 commits into from
Closed

Desktop: Added options to set default templates #2857

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
897d9cc
Added options to set default templates
johodh Mar 22, 2020
d33fe5e
Small fix
johodh Mar 23, 2020
693040f
After review changes and delete template fix
johodh Mar 24, 2020
dcf0a4b
Moved delete check inside createNewNote
johodh Mar 26, 2020
b6b53e8
Fix to support editing of current default templ.
johodh Mar 26, 2020
979f8e2
availableTemplates_ now prop of TemplateUtils
johodh May 10, 2020
46e6822
Requested changes
johodh May 27, 2020
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
1 change: 0 additions & 1 deletion ElectronClient/app.js
View file
Open in desktop
Expand Up @@ -599,7 +599,6 @@ class Application extends BaseApplication {
label: _('Refresh templates'),
click: async () => {
const templates = await TemplateUtils.loadTemplates(Setting.value('templateDir'));

this.store().dispatch({
type: 'TEMPLATE_UPDATE_ALL',
templates: templates,
Expand Down
13 changes: 12 additions & 1 deletion ElectronClient/gui/MainScreen.jsx
View file
Open in desktop
Expand Up @@ -99,7 +99,18 @@ class MainScreenComponent extends React.Component {
const createNewNote = async (template, isTodo) => {
const folderId = Setting.value('activeFolderId');
if (!folderId) return;

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;
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

@johodh johodh May 27, 2020 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

const body = template ? TemplateUtils.render(template) : '';

const newNote = await Note.save({
Expand Down
7 changes: 6 additions & 1 deletion ReactNativeClient/lib/TemplateUtils.js
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@ const { time } = require('lib/time-utils.js');
const Mustache = require('mustache');

const TemplateUtils = {};

TemplateUtils.availableTemplates_ = [];

// Mustache escapes strings (including /) with the html code by default
// This isn't useful for markdown so it's disabled
Expand Down Expand Up @@ -62,7 +62,12 @@ TemplateUtils.loadTemplates = async function(filePath) {
});
}

TemplateUtils.availableTemplates_ = templates;
return templates;
};

TemplateUtils.availableTemplates = function() {
return TemplateUtils.availableTemplates_;
};

module.exports = TemplateUtils;
35 changes: 35 additions & 0 deletions ReactNativeClient/lib/models/Setting.js
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ const { toTitleCase } = require('lib/string-utils.js');
const { rtrimSlashes } = require('lib/path-utils.js');
const { _, supportedLocalesToLanguages, defaultLocale } = require('lib/locale.js');
const { shim } = require('lib/shim');
const TemplateUtils = require('lib/TemplateUtils');

class Setting extends BaseModel {
static tableName() {
Expand Down Expand Up @@ -346,6 +347,40 @@ class Setting extends BaseModel {
};
},
},
defaultNote: {
Copy link
Owner

Choose a reason for hiding this comment

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

Please rename to editor.defaultNoteTemplate and editor.defaultTodoTemplate

value: '0',
Copy link
Owner

Choose a reason for hiding this comment

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

Defaults to empty string ''

type: Setting.TYPE_STRING,
section: 'note',
isEnum: true,
public: true,
appTypes: ['desktop'],
label: () => _('Choose default template for new note:'),
Copy link
Owner

Choose a reason for hiding this comment

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

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

options: () => {
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;
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

},
},
defaultTodo: {
value: '0',
type: Setting.TYPE_STRING,
section: 'note',
isEnum: true,
public: true,
appTypes: ['desktop'],
label: () => _('Choose default template for new todo:'),
options: () => {
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;
Copy link
Owner

Choose a reason for hiding this comment

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

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

},
},

// Deprecated - use markdown.plugin.*
'markdown.softbreaks': { value: false, type: Setting.TYPE_BOOL, public: false, appTypes: ['mobile', 'desktop'] },
Expand Down