CLI: Toggle short ids and mv notebooks for #1728 #6671

k33pn3xtlvl · 2022-07-16T23:59:19Z

I try to bundle all the works that has been done around the request #1728.

This is a summary of the events:

Issue CLI: Add an option to mkbook command to the CLI client for creating sub notebooks. Fixes #1728 #3554 had a logical dependency from the issue Create subnotebooks in terminal #1728.
This request (with a solution for #3554) was unfortunately not answered.
Nearly a year later, the issue CLI: Fixes #1728 mv command works on notebooks now #5191 was try to solving the move-notebook problem, but was canceled.

Respect and thanks to @DominicLavery and @kai11 for their work!

The currently solution looks like this.

Method 1:

Work fine with notebook names mv folder1 folder2. As it was described in #5191.
If the user try to move the notebook (folder) with mv folder1 folder3 an error message appears, because there is an ambiguity. See Screenshot.

Method 2:

If the title of the notebook is the same you must work with ids.
You press in the joplin cli ti for toggle ids an work with the short ids and use mv for example mv d82ed 419a0.

If this PR is merged we can go forward with create subnotebooks implementation #1728 .
A good suggestion came from @DominicLavery (#3554).

Again all credit goes to @DominicLavery and @kai11!

Based on the commit from DominicLavery@6428e66 add to note list the same. Need for issue laurent22#1728

Fix FolderListWidget view conflict

In my opinion the [solution](laurent22#5191 (comment)) works good. For the ambiguous notebooks you can use `toggle_ids` with `ti` and work with `mv`. The credit goes to kai11 !

personalizedrefrigerator

Nice work so far! After reading through the diff, I have a few (all optional) suggestions. Feel free to ignore any/all of them!

A note about marking comments as resolved:

joplin/CONTRIBUTING.md

Line 42 in 520d974

    
           - **Do not mark your reviewer's comments as "resolved"**. If you do that, the comments will be hidden and the reviewer will not know what are the pending issues in the pull request. Only the reviewer should resolve the comments.

Feel free to ignore this part of CONTRIBUTING.md — I don't have the ability to mark the below comments as resolved.

packages/app-cli/app/command-mv.js

Compliance with the style guide Co-authored-by: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com>

k33pn3xtlvl · 2022-07-25T13:12:12Z

@personalizedrefrigerator Thanks for your constructive suggestions for improvement.

Now i need someone who can approval this PR because from my site i see "1 workflow awaiting approval" (Need a maintainer to approve running workflow) or can i ignore this?

Please inform me if i need to do something additionally. (Maybe i have missed something).

k33pn3xtlvl · 2022-07-26T09:39:11Z

Is it possible that the workflow sporadically does not run properly?
I have looked at the workflow logs and i can`t really figure it out where the error could be in relation to the PR.

personalizedrefrigerator · 2022-07-26T09:40:07Z

Is it possible that the workflow sporadically does not run properly? I have looked at the workflow logs and i can`t really figure it out where the error could be in relation to the PR.

I think so. Pushing a new commit (e.g. merging in upstream/dev) should re-run the workflow!

k33pn3xtlvl · 2022-07-29T07:55:34Z

Sorry for my questions i am new on the Project honestly, but can we merge this PR?
I need this merge to move on with the main goal of the #1728. I would keep merge requests small as possible.

Is something still need from my side? (Awaiting for approval, again)

k33pn3xtlvl · 2022-07-29T15:25:13Z

@personalizedrefrigerator can you run the workflow again please.
The CI crashes on https://github.com/laurent22/joplin/runs/7575110850?check_suite_focus=true#step:10:4531 and it was fixed in 12a510c

roman-r-m · 2022-07-29T15:40:12Z

Strange. I thought I had approved it earlier today but it needs approval again. Approved now.

k33pn3xtlvl · 2022-07-30T14:20:55Z

Ok, now it maybe my fault. I saw that the PR is still open after the checks have passed successfully. I try to click the button "update" an some one need to approval the workflow again.

I don't really understand by what criterion the PR are merged and by whom. I thought the merge is done if the the workflow process are successful.

Do I understand it correctly that no more actions are necessary from my side even if the workflow has passed all checks?

Optional recommendation. It would be helpful for new people (like me), if the merge process is described in the CONTRIBUTING.md. (Maybe it's an unwritten law and i do not know it) Feel free to ignore it if its so!

personalizedrefrigerator · 2022-07-30T23:39:04Z

Ok, now it maybe my fault. I saw that the PR is still open after the checks have passed successfully. I try to click the button "update" an some one need to approval the workflow again.

I don't really understand by what criterion the PR are merged and by whom. I thought the merge is done if the the workflow process are successful.

Do I understand it correctly that no more actions are necessary from my side even if the workflow has passed all checks?

Optional recommendation. It would be helpful for new people (like me), if the merge process is described in the CONTRIBUTING.md. (Maybe it's an unwritten law and i do not know it) Feel free to ignore it if its so!

I don't have permission to merge pull requests or grant access to run the automated tests. Code reviews/checks by maintainers are usually done about once every week. If everything looks good when that's done the pull request is generally merged.

personalizedrefrigerator · 2022-07-30T23:41:48Z

I need this merge to move on with the main goal of the #1728. I would keep merge requests small as possible.

If you're willing to resolve future merge conflicts, you can open a second PR that includes the changes from this one!

It can be somewhat difficult to resolve merge conflicts and keep things consistent across branches, though. I generally have one or two development branches. I try to make changes to these branches first, then copy/backport individual changes to a new PR.

k33pn3xtlvl · 2022-08-25T07:05:11Z

@laurent22 Can you merge it or is anything from my side to do?
People can test the pre-release maybe they will found some good improve ideas.

laurent22 · 2022-08-27T11:07:07Z

packages/app-cli/app/app-gui.js

@@ -375,6 +375,11 @@ class AppGui {
 		this.showNoteMetadata(!this.widget('noteMetadata').shown);
 	}

+	toggleNotebookIds() {


In code, always use "folder", not "notebook".

laurent22 · 2022-08-27T11:08:05Z

packages/app-cli/app/command-mv.js


-		const notes = await app().loadItems(BaseModel.TYPE_NOTE, pattern);
-		if (!notes.length) throw new Error(_('Cannot find "%s".', pattern));
+		const dstDups = await Folder.search({ titlePattern: destination, limit: 2 });


We avoid excessive abbreviations. So at least "destDuplicates"

laurent22 · 2022-08-27T11:08:41Z

packages/app-cli/app/command-mv.js

-		if (!notes.length) throw new Error(_('Cannot find "%s".', pattern));
+		const dstDups = await Folder.search({ titlePattern: destination, limit: 2 });
+		if (dstDups.length > 1) {
+			throw new Error(_('Ambiguous notebook "%s". Please use notebook id instead - look for parent id in metadata', destination));


I don't understand what this mean "look for parent id in metadata". What concrete action should the user do?

Thanks, i updated it, it should be clearer now.

laurent22 · 2022-08-27T11:08:48Z

packages/app-cli/app/command-mv.js

-			await Note.moveToFolder(notes[i].id, folder.id);
+		const itemFolder = await app().loadItem(BaseModel.TYPE_FOLDER, pattern);
+		if (itemFolder) {
+			const srcDups = await Folder.search({ titlePattern: pattern, limit: 2 });


sourceDuplicates

laurent22 · 2022-08-27T11:09:06Z

packages/app-cli/app/command-mv.js

+		if (itemFolder) {
+			const srcDups = await Folder.search({ titlePattern: pattern, limit: 2 });
+			if (srcDups.length > 1) {
+				throw new Error(_('Ambiguous notebook "%s". Please use notebook id instead - look for parent id in metadata or use $b for current', pattern));


k33pn3xtlvl added 3 commits July 15, 2022 23:59

Add to CLI add toggle to show short ids

7df3688

Based on the commit from DominicLavery@6428e66 add to note list the same. Need for issue laurent22#1728

Merge branch 'toggle_ids' into dev

8f78486

Fix FolderListWidget view conflict

Solution from issue laurent22#5191 applied

309d24b

In my opinion the [solution](laurent22#5191 (comment)) works good. For the ambiguous notebooks you can use `toggle_ids` with `ti` and work with `mv`. The credit goes to kai11 !

personalizedrefrigerator reviewed Jul 17, 2022

View reviewed changes

packages/app-cli/app/command-mv.js Outdated Show resolved Hide resolved

packages/app-cli/app/command-mv.js Outdated Show resolved Hide resolved

packages/app-cli/app/command-mv.js Outdated Show resolved Hide resolved

packages/app-cli/app/command-mv.js Show resolved Hide resolved

Apply suggestions from code review

39d8565

Compliance with the style guide Co-authored-by: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com>

k33pn3xtlvl added 2 commits July 28, 2022 10:39

Merge branch 'dev' into dev

c30bd8f

Merge branch 'dev' into dev

a6d4f31

Merge branch 'dev' into dev

736a02c

Merge branch 'dev' into dev

158ab4b

Merge branch 'laurent22:dev' into dev

b55acd3

k33pn3xtlvl mentioned this pull request Aug 6, 2022

CLI: Create Subnotebooks #6722

Merged

laurent22 reviewed Aug 27, 2022

View reviewed changes

Implementation of the suggestions from laurent22

a956722

Daeraxa added the cli CLI app specific issue label Sep 3, 2022

laurent22 merged commit b5b281c into laurent22:dev Sep 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: Toggle short ids and mv notebooks for #1728 #6671

CLI: Toggle short ids and mv notebooks for #1728 #6671

k33pn3xtlvl commented Jul 16, 2022

personalizedrefrigerator left a comment •

edited

Loading

k33pn3xtlvl commented Jul 25, 2022

k33pn3xtlvl commented Jul 26, 2022

personalizedrefrigerator commented Jul 26, 2022 •

edited

Loading

k33pn3xtlvl commented Jul 29, 2022

k33pn3xtlvl commented Jul 29, 2022

roman-r-m commented Jul 29, 2022

k33pn3xtlvl commented Jul 30, 2022

personalizedrefrigerator commented Jul 30, 2022

personalizedrefrigerator commented Jul 30, 2022 •

edited

Loading

k33pn3xtlvl commented Aug 25, 2022

laurent22 Aug 27, 2022

laurent22 Aug 27, 2022

laurent22 Aug 27, 2022

k33pn3xtlvl Aug 28, 2022

laurent22 Aug 27, 2022

laurent22 Aug 27, 2022

CLI: Toggle short ids and mv notebooks for #1728 #6671

CLI: Toggle short ids and mv notebooks for #1728 #6671

Conversation

k33pn3xtlvl commented Jul 16, 2022

The currently solution looks like this.

Method 1:

Method 2:

personalizedrefrigerator left a comment • edited Loading

Choose a reason for hiding this comment

k33pn3xtlvl commented Jul 25, 2022

k33pn3xtlvl commented Jul 26, 2022

personalizedrefrigerator commented Jul 26, 2022 • edited Loading

k33pn3xtlvl commented Jul 29, 2022

k33pn3xtlvl commented Jul 29, 2022

roman-r-m commented Jul 29, 2022

k33pn3xtlvl commented Jul 30, 2022

personalizedrefrigerator commented Jul 30, 2022

personalizedrefrigerator commented Jul 30, 2022 • edited Loading

k33pn3xtlvl commented Aug 25, 2022

laurent22 Aug 27, 2022

Choose a reason for hiding this comment

laurent22 Aug 27, 2022

Choose a reason for hiding this comment

laurent22 Aug 27, 2022

Choose a reason for hiding this comment

k33pn3xtlvl Aug 28, 2022

Choose a reason for hiding this comment

laurent22 Aug 27, 2022

Choose a reason for hiding this comment

laurent22 Aug 27, 2022

Choose a reason for hiding this comment

personalizedrefrigerator left a comment •

edited

Loading

personalizedrefrigerator commented Jul 26, 2022 •

edited

Loading

personalizedrefrigerator commented Jul 30, 2022 •

edited

Loading