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.

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: Resolves #6167 : Move to notebook cannot distinguish notebooks with the same name #6400

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
106 changes: 106 additions & 0 deletions packages/app-desktop/gui/MainScreen/commands/moveToFolder.test.js
@@ -0,0 +1,106 @@
const moveToFolder = require('./moveToFolder');

Copy link
Owner

Choose a reason for hiding this comment

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

All new files should be TypeScript

Copy link
Owner

Choose a reason for hiding this comment

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

Also unfortunately I have no idea what these test units do. Have a look at what other test units do. For example the title is often it('should do this or that'). What does your code do? Explain it clearly in the test title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also unfortunately I have no idea what these test units do

It tests the AddOptions function that returns paths of folders with different scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have a look at what other test units do. For example the title is often it('should do this or that'). What does your code do? Explain it clearly in the test title.

OK, I will try to refactor the code and convert the unit test to TS.


describe('AddOptions: Test The Path of Folders', () => {

const options = new moveToFolder.AddOptions();

beforeEach(() => {
options.startFolders = [];
});

it('Empty Folder Title', () => {
const testCases = [{
id: 1, title: '',
children: [{
id: 2, title: '',
}],
}];

options.addOptions(testCases, 0, '');
const data = options.startFolders;
expect(data[0].path).toEqual('');
});

it('Different Paths', () => {
const testCases = [
{ id: 8, title: 'my Note' },
{
id: 2, title: 'first',
children: [{
id: 3, title: 'second',
children: [{
id: 4, title: 'third',
children: [{ id: 5, title: 'fourth' }],
}, { id: 6, title: 'fourth' }],
}, { id: 7, title: 'fourth' }],
}];

const answers = ['my Note', 'first', 'first/second', 'first/second/third', 'first/second/third/fourth', 'first/second/fourth', 'first/fourth'];

options.addOptions(testCases, 0, '');
const data = options.startFolders;
for (let i = 0; i < data.length; i++) {
expect(data[i].path).toEqual(answers[i]);
}

});

it('Long Path', () => {
const testCases = [{
id: 1, title: '1',
children: [{
id: 2, title: '2',
children: [{
id: 3, title: '3',
children: [{
id: 4, title: '4',
children: [{
id: 5, title: '5',
children: [{
id: 6, title: '6',
children: [{
id: 7, title: '7',
children: [{
id: 8, title: '8',
children: [{
id: 9, title: '9',
children: [{
id: 10, title: '10',
children: [{
id: 11, title: '11',
children: [{
id: 12, title: '12',
children: [{
id: 13, title: '13',
children: [{
id: 14, title: '14',
children: [{ id: 15, title: '15' }],
}],
}],
}],
}],
}],
}],
}],
}],
}],
}],
}],
}],
}, { id: 16, title: '16' }],
}];

options.addOptions(testCases, 0, '');
const data = options.startFolders;
let answer = '';
for (let i = 1; i <= data.length - 1; i++) {
answer += i;
expect(data[i - 1].path).toEqual(answer);
answer += '/';
}
expect(data[data.length - 1].path).toEqual('1/16');

});

});
31 changes: 18 additions & 13 deletions packages/app-desktop/gui/MainScreen/commands/moveToFolder.ts
Expand Up @@ -8,31 +8,36 @@ export const declaration: CommandDeclaration = {
label: () => _('Move to notebook'),
};

export class AddOptions {

private maxDepth = 15;
public startFolders: any[] = [];
Copy link
Owner

Choose a reason for hiding this comment

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

FolderEntity. No "any" of you can avoid it


public addOptions(folders: any[], depth: number, path: string) {
for (let i = 0; i < folders.length; i++) {
const folder = folders[i];
const newPath = path + folder.title;
this.startFolders.push({ key: folder.id, value: folder.id, label: folder.title, title: folder.title, path: newPath, indentDepth: depth, _indentDepth: depth });
if (folder.children) this.addOptions(folder.children, (depth + 1) < this.maxDepth ? depth + 1 : this.maxDepth, `${newPath}/`);
}
}
}

export const runtime = (comp: any): CommandRuntime => {
return {
execute: async (context: CommandContext, noteIds: string[] = null) => {
noteIds = noteIds || context.state.selectedNoteIds;

const folders: any[] = await Folder.sortFolderTree();
const startFolders: any[] = [];
const maxDepth = 15;

const addOptions = (folders: any[], depth: number) => {
for (let i = 0; i < folders.length; i++) {
const folder = folders[i];
startFolders.push({ key: folder.id, value: folder.id, label: folder.title, indentDepth: depth });
if (folder.children) addOptions(folder.children, (depth + 1) < maxDepth ? depth + 1 : maxDepth);
}
};

addOptions(folders, 0);
const options = new AddOptions();
Copy link
Owner

Choose a reason for hiding this comment

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

the naming is not good. Those are not options.

options.addOptions(folders, 0, '');

comp.setState({
promptOptions: {
label: _('Move to notebook:'),
inputType: 'dropdown',
value: '',
autocomplete: startFolders,
autocomplete: options.startFolders,
onClose: async (answer: any) => {
if (answer) {
for (let i = 0; i < noteIds.length; i++) {
Expand Down
29 changes: 28 additions & 1 deletion packages/app-desktop/gui/PromptDialog.jsx
Expand Up @@ -216,6 +216,33 @@ class PromptDialog extends React.Component {
}
};

const searchByTitle = option => {
return option.title;
};

const onInputChange = (input) => {
if (input) {
// To convert Notebooks title to paths.
if (this.props.autocomplete) {
for (let i = 0; i < this.props.autocomplete.length; i++) {
this.props.autocomplete[i].label = <span> {this.props.autocomplete[i].path}</span>;
this.props.autocomplete[i].indentDepth = 0;
}
}

} else {
// To convert the Notebooks title into a tree again
if (this.props.autocomplete) {
for (let i = 0; i < this.props.autocomplete.length; i++) {
// To return the previous 'label & indentDepth' value.
this.props.autocomplete[i].label = this.props.autocomplete[i].title;
this.props.autocomplete[i].indentDepth = this.props.autocomplete[i]._indentDepth;
}
}
}

};

const descComp = this.props.description ? <div style={styles.desc}>{this.props.description}</div> : null;

let inputComp = null;
Expand All @@ -225,7 +252,7 @@ class PromptDialog extends React.Component {
} else if (this.props.inputType === 'tags') {
inputComp = <CreatableSelect className="tag-selector" styles={styles.select} theme={styles.selectTheme} ref={this.answerInput_} value={this.state.answer} placeholder="" components={makeAnimated()} isMulti={true} isClearable={false} backspaceRemovesValue={true} options={this.props.autocomplete} onChange={onSelectChange} onKeyDown={event => onKeyDown(event)} />;
} else if (this.props.inputType === 'dropdown') {
inputComp = <Select className="item-selector" styles={styles.select} theme={styles.selectTheme} ref={this.answerInput_} components={makeAnimated()} value={this.props.answer} defaultValue={this.props.defaultValue} isClearable={false} options={this.props.autocomplete} onChange={onSelectChange} onKeyDown={event => onKeyDown(event)} />;
inputComp = <Select className="item-selector" styles={styles.select} theme={styles.selectTheme} ref={this.answerInput_} components={makeAnimated()} value={this.props.answer} defaultValue={this.props.defaultValue} isClearable={false} options={this.props.autocomplete} getOptionValue={searchByTitle} onInputChange={onInputChange} onChange={onSelectChange} onKeyDown={event => onKeyDown(event)} />;
} else {
inputComp = <input style={styles.input} ref={this.answerInput_} value={this.state.answer} type="text" onChange={event => onChange(event)} onKeyDown={event => onKeyDown(event)} />;
}
Expand Down