Desktop: Fixes #11833: Restore notebook from trash without requiring selection

[rathoreSahil]

What this PR does?

• Resolves the issue where restore notebook does not work if the notebook is not selected first.
• Fixes Restore notebook from trash does not work if notebook not selected first #11833

Issue Description

• Open Joplin profile with multiple notebooks.
• Delete a notebook (move to trash).
• Right-click deleted notebook in trash. A context menu with "Restore notebook" appears.
• Click "Restore notebook". Nothing happens.

Root Cause

• The enable condition for restore notebook option was the 'folderIsDeleted' flag in the WhenClauseContext object.
  
  

• The stateToWhenClauseContext function is responsible for converting current state to WhenClauseContext object.
• It sets the context options for the current menu item based on the commandFolder which if not explicitly set is taken to be the currently selected entity by default.

• Since the currently selected item does not have 'folderIsDeleted' property set to true hence the restore notebook option is disabled.

Issue resolution

• We can pass WhenClauseContextOptions to the 'stateToWhenClauseContext' function through which we can explicitly specify the id of the command folder for the current menu item.
• The itemId of the currently clicked item was already being passes as an argument to the function which converts the command to a stateful menu item.
• I extracted the itemId and passed it as current commandFolderId option to the 'stateToWhenClauseContext' context function and this resolves the issue.

Video

Before

before.mov

After

after.mov

[personalizedrefrigerator]

Thank you for working on this! There's a failing test:

➤ YN0000: [@joplin/lib]: Summary of all failing tests
➤ YN0000: [@joplin/lib]: FAIL services/CommandService.test.js
➤ YN0000: [@joplin/lib]:   ● services_CommandService › should create stateful menu items
➤ YN0000: [@joplin/lib]: 
➤ YN0000: [@joplin/lib]:     TypeError: Cannot read properties of undefined (reading 'length')
➤ YN0000: [@joplin/lib]: 
➤ YN0000: [@joplin/lib]:       147 | 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
➤ YN0000: [@joplin/lib]:       148 | 	public static byId(items: any[], id: string) {
➤ YN0000: [@joplin/lib]:     > 149 | 		for (let i = 0; i < items.length; i++) {
➤ YN0000: [@joplin/lib]:           | 		                          ^
➤ YN0000: [@joplin/lib]:       150 | 			if (items[i].id === id) return items[i];
➤ YN0000: [@joplin/lib]:       151 | 		}
➤ YN0000: [@joplin/lib]:       152 | 		return null;
➤ YN0000: [@joplin/lib]: 
➤ YN0000: [@joplin/lib]:       at Function.length [as byId] (BaseModel.ts:149:29)
➤ YN0000: [@joplin/lib]:       at CommandService.byId [as stateToWhenClauseContext_] (services/commands/stateToWhenClauseContext.ts:62:66)
➤ YN0000: [@joplin/lib]:       at CommandService.stateToWhenClauseContext_ [as currentWhenClauseContext] (services/CommandService.ts:341:15)
➤ YN0000: [@joplin/lib]:       at MenuUtils.currentWhenClauseContext [as commandToStatefulMenuItem] (services/commands/MenuUtils.ts:113:42)
➤ YN0000: [@joplin/lib]:       at Object.commandToStatefulMenuItem (services/CommandService.test.ts:281:26)
➤ YN0000: [@joplin/lib]: 

[rathoreSahil]

Problem

The issue was that earlier we were passing folderId as a raw string to the 'commandToStatefulMenuItem' function and i was extracting it from args and setting the WhenClauseContextOption, but the problem is that the itemId will not always be a folderId, it might also be a note id or a tag id hence the test case was failing where only a single note id was being passed to the function.

There are multiple ways to solve this:

• One way was to pass in a type along with the item id but that would lead to inconsistency in the code and might have led to changes in a lot a places just to maintain consistency.
• Another way is to pass in an object as { folderId: itemId } instead of plain itemId. We can then check whether the id is a folder id or not and set the context option accordingly else pass option as undefined which would lead to normal functionality.

Additional Changes

Also since the argument type changed I had to make a small change in the restoreFolder.ts file as well where i take an option object and extract folder Id from it.

[personalizedrefrigerator]

Based on a quick look through the code, this looks good to me!

[laurent22]

I'm not a fan of this. "options" which can be a string, the conditional, etc. It feels messy and it's in a place where code is already quite complex so it would be good to find a cleaner/clearer way to do this