Desktop: Resolves #6167 : Move to notebook cannot distinguish notebooks with the same name #6400

bishoy-magdy · 2022-04-12T17:53:44Z

According to #6167, There are many ways to display the paths of folders.,I see the best way to show the path while preserving the search form is by adding the path title for each folder by using the label in the selector and adding a span with a title for each folder, Plus resetting the search by value only and I will get the path while traveling the folders in addOptions function.

And I fixed the bug that shows white space when there is no title on the folder in Move to notebook by adding an Untitled label.

Example:

demo.mp4

laurent22 · 2022-04-13T11:44:02Z

It should show the full note path, including notebook on each line. Otherwise you indeed need something like a tooltip but nobody will know there's a tooltip, and it's not convenient if you have to move your mouse over each title.

bishoy-magdy · 2022-04-13T23:18:44Z

It should show the full note path, including notebook on each line. Otherwise you indeed need something like a tooltip but nobody will know there's a tooltip, and it's not convenient if you have to move your mouse over each title.

Like this?

I think in this case, we don't need indentDepth. We can use this space to display the path!

And searchByValue will compare only the notebook values (to search properly).

laurent22 · 2022-04-13T23:34:19Z

I think it should display a tree, except when searching in which case it should disambiguate with the notebook name. I think that's what the original issue was suggesting.

bishoy-magdy · 2022-04-14T01:54:14Z

I think it should display a tree, except when searching in which case it should disambiguate with the notebook name. I think that's what the original issue was suggesting.

I think you mean like this:

demo.mp4

and I used onInputChange to update the view.

laurent22 · 2022-06-07T17:00:55Z

packages/app-desktop/gui/MainScreen/commands/moveToFolder.ts

+			if (folder.title === '') {
+				folder.title = 'Untitled';
+			}
+			const new_path = path + folder.title;


laurent22 · 2022-06-07T17:01:34Z

packages/app-desktop/gui/MainScreen/commands/moveToFolder.test.js

+		options.startFolders = [];
+	});
+
+	it('Empty Notebook Title', () => {


"empty folder title"

laurent22 · 2022-06-07T17:02:25Z

packages/app-desktop/gui/MainScreen/commands/moveToFolder.test.js

+	});
+
+	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' }] }];


Put that over multiple lines please. It's unreadable now

laurent22 · 2022-06-07T17:02:34Z

packages/app-desktop/gui/MainScreen/commands/moveToFolder.test.js

+	});
+
+	it('Diffrent 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' }] }];


over multiple lines please

laurent22 · 2022-06-07T17:02:38Z

packages/app-desktop/gui/MainScreen/commands/moveToFolder.test.js

+
+	});
+
+	it('Diffrent Paths', () => {


laurent22 · 2022-06-07T17:03:53Z

packages/app-desktop/gui/MainScreen/commands/moveToFolder.ts

+			const folder = folders[i];
+			// When NoteBook doesn't get a title
+			if (folder.title === '') {
+				folder.title = 'Untitled';


What does it do if you leave it as an empty string? I'd prefer an empty string because untitled notebooks is not really something we support

What does it do if you leave it as an empty string?

Any untitled notebook will appear in the search bar as an empty string like this:

What I did was just show the word 'Untitled' for any untitled notebook in the search.

Would you still prefer to remove this condition?

Yes please, remove it

laurent22 · 2022-06-07T17:04:10Z

packages/app-desktop/gui/PromptDialog.jsx

+			return option._value;
+		};
+
+		const onInputChange = (_input) => {


laurent22 · 2022-06-07T17:04:20Z

packages/app-desktop/gui/PromptDialog.jsx

+		};
+
+		const onInputChange = (_input) => {
+			if (_input !== '') {


if (input)

laurent22 · 2022-06-20T13:03:01Z

Any update for this PR?

bishoy-magdy · 2022-06-23T05:32:57Z

Any update for this PR?

Done :)
Sorry for the delay in updating the changes, and I did some refactoring of variables.

laurent22 · 2022-06-26T16:39:29Z

packages/app-desktop/gui/MainScreen/commands/moveToFolder.test.js

@@ -0,0 +1,106 @@
+const moveToFolder = require('./moveToFolder');
+


All new files should be TypeScript

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.

Also unfortunately I have no idea what these test units do

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

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.

laurent22 · 2022-06-26T16:44:49Z

packages/app-desktop/gui/MainScreen/commands/moveToFolder.ts

+export class AddOptions {
+
+	private maxDepth = 15;
+	public startFolders: any[] = [];


FolderEntity. No "any" of you can avoid it

laurent22 · 2022-06-26T16:45:16Z

packages/app-desktop/gui/MainScreen/commands/moveToFolder.ts

-			};
-
-			addOptions(folders, 0);
+			const options = new AddOptions();


the naming is not good. Those are not options.

laurent22 · 2022-07-10T13:49:13Z

Not sure what's the status of this PR?

bishoy-magdy · 2022-07-15T23:18:30Z

Not sure what's the status of this PR?

When adding a new unit test (.test.ts) file in MainScreen/commands it crashes when building the Joplin app, and I see a unit test file automatically added in the index.ts file. Is there a solution to this problem? Or do I need to move a unit test file somewhere else?

yarn run watch log error:
gui/MainScreen/commands/index.ts:8:25 - error TS1005: 'from' expected.

8 import * as moveToFolder.test from './moveToFolder.test';
                          ~
gui/MainScreen/commands/index.ts:8:31 - error TS1005: ';' expected.
8 import * as moveToFolder.test from './moveToFolder.test';
                                ~~~~
gui/MainScreen/commands/index.ts:8:36 - error TS1005: ';' expected.
8 import * as moveToFolder.test from './moveToFolder.test';

laurent22 · 2022-12-21T10:05:44Z

I believe this has been implemented in another pull request. Thanks anyway for looking into it @bishoy-magdy!

bishoy-magdy added 12 commits February 8, 2022 02:35

Update CodeMirror.tsx

eedd9ec

Update CodeMirror.tsx

dc9c9b8

Update CodeMirror.tsx

d10252b

Update CodeMirror.tsx

c19a66c

Update CodeMirror.tsx

63683e9

Merge branch 'laurent22:dev' into dev

d2c88d1

Merge branch 'laurent22:dev' into dev

1105eea

Merge branch 'laurent22:dev' into dev

c9dd7ec

Merge branch 'laurent22:dev' into dev

03f3187

Update moveToFolder.ts

a301f30

Update PromptDialog.jsx

1c1e535

Create moveToFolder.test.js

af62fb3

laurent22 and others added 4 commits April 13, 2022 12:44

Merge branch 'dev' into dev

7802a97

Update PromptDialog.jsx

ff2ab91

Update moveToFolder.ts

a055611

Update moveToFolder.test.js

9c7e580

bishoy-magdy added 3 commits April 14, 2022 03:48

Update moveToFolder.ts

7d86145

Update moveToFolder.test.js

6079bc9

Update PromptDialog.jsx

24e3c80

laurent22 reviewed Jun 7, 2022

View reviewed changes

bishoy-magdy added 3 commits June 23, 2022 07:01

Update PromptDialog.jsx

ce8fa6a

Update moveToFolder.test.js

df71ce5

Update moveToFolder.ts

f1f90a0

laurent22 reviewed Jun 26, 2022

View reviewed changes

laurent22 closed this Dec 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Resolves #6167 : Move to notebook cannot distinguish notebooks with the same name #6400

Desktop: Resolves #6167 : Move to notebook cannot distinguish notebooks with the same name #6400

bishoy-magdy commented Apr 12, 2022

laurent22 commented Apr 13, 2022

bishoy-magdy commented Apr 13, 2022

laurent22 commented Apr 13, 2022

bishoy-magdy commented Apr 14, 2022

laurent22 Jun 7, 2022

laurent22 Jun 7, 2022

laurent22 Jun 7, 2022

laurent22 Jun 7, 2022

laurent22 Jun 7, 2022

laurent22 Jun 7, 2022

bishoy-magdy Jun 12, 2022

laurent22 Jun 20, 2022

laurent22 Jun 7, 2022

laurent22 Jun 7, 2022

laurent22 commented Jun 20, 2022

bishoy-magdy commented Jun 23, 2022 •

edited

laurent22 Jun 26, 2022

laurent22 Jun 26, 2022

bishoy-magdy Jul 1, 2022

bishoy-magdy Jul 1, 2022

laurent22 Jun 26, 2022

laurent22 Jun 26, 2022

laurent22 commented Jul 10, 2022

bishoy-magdy commented Jul 15, 2022

laurent22 commented Dec 21, 2022

		@@ -0,0 +1,106 @@
		const moveToFolder = require('./moveToFolder');

Desktop: Resolves #6167 : Move to notebook cannot distinguish notebooks with the same name #6400

Desktop: Resolves #6167 : Move to notebook cannot distinguish notebooks with the same name #6400

Conversation

bishoy-magdy commented Apr 12, 2022

laurent22 commented Apr 13, 2022

bishoy-magdy commented Apr 13, 2022

laurent22 commented Apr 13, 2022

bishoy-magdy commented Apr 14, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Jun 20, 2022

bishoy-magdy commented Jun 23, 2022 • edited

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

laurent22 commented Jul 10, 2022

bishoy-magdy commented Jul 15, 2022

laurent22 commented Dec 21, 2022

bishoy-magdy commented Jun 23, 2022 •

edited