Mobile: Remove redundant navigation history when notes or folders are deleted

[mrjo118]

The Joplin Data API allows permanently deleting notes and folders. In the desktop app, it correctly handles removing deleted notes and folders from the navigation history when this is done, via the 'FOLDER_DELETE' and 'NOTE_DELETE' event handlers in reducer.ts. This however only applies to the desktop app, and equivalent logic is missing in appReducer.ts which is used to handle the note navigation history on the mobile app. This means that when making such deletions via the API, it can break the navigation history on mobile, causing the back button to navigate to useless / empty screens. It may also happen for other reasons, such as if the sync runs while editing note, and the user has not yet synced some incoming deletions. This PR implements equivalent handling on the mobile app.

Testing

In order to test this change, I have have made local modifications to the WIP Secure Notes plugin, in order to invoke all of possible scenarios which new logic has been added for.

Testing for the FOLDER_DELETE event:

This was done via the following plugin code:

  const existingFolders = await joplin.data.get(['folders'], { fields: ['id', 'title'] });
  const existingFolder = existingFolders.items.find(folder => folder.title === 'test');
  const tempFolder = await joplin.data.post(['folders'], null, {
      title: 'tempfolder',
  });
  const tempNote = await joplin.data.post(['notes'], null, {
      title: 'temp',
      body: '',
      parent_id: tempFolder.id,
  });

  // Force refresh by switching notes
  await joplin.commands.execute('openNote', tempNote.id);
  await joplin.commands.execute('openNote', noteId);
  await joplin.commands.execute('openNote', tempNote.id);
  await joplin.commands.execute('openNote', noteId);
  await joplin.data.delete(['folders', tempFolder.id], { permanent: '1' });
  await joplin.data.delete(['folders', existingFolder.id], { permanent: '1' });

The video shows that when navigating between Test1 > test > Test1 that upon encrypting / decrypting a note (which triggers the above code), when the test folder is deleted, the test folder is removed from the 3 navigation entries, and then Test1 > Test1 is de-duplicated so that pressing the back button just once after exiting the note, navigates back to the starting 'All Notes' folder. The deletion of the temp note was done via the deletion of the tempFolder, where it's children are implicitly deleted, and this fires a NOTE_DELETE event which removes the tempNote navigation's made by the code. Then the other 2 navigations are deduplicated because they are now adjacent, and the remaining entry is removed because it is the current note. Therefore a single press of the back button after encrypt / decrypt will take you back to the the note list

folder.test.mp4

So this test actually covers all the NOTE_DELETE scenarios as well, but I also did a separate test for the NOTE_DELETE scenarios in isolation.

Testing for the NOTE_DELETE event:

This was done via the following plugin code:

  const note = await joplin.data.get(['notes', noteId], { fields: ['parent_id'] });
  const tempNote = await joplin.data.post(['notes'], null, {
      title: 'temp',
      body: '',
      parent_id: note.parent_id,
  });

  // Force refresh by switching notes
  await joplin.commands.execute('openNote', tempNote.id);
  await joplin.commands.execute('openNote', noteId);
  await joplin.commands.execute('openNote', tempNote.id);
  await joplin.commands.execute('openNote', noteId);
  await joplin.data.delete(['notes', tempNote.id], { permanent: '1' });

note.test.mp4

[laurent22]

Is that necessary the eslint-disable-next-line comments? If it is, and refactoring would be complicated, at least please set the comment correctly as this is not old code. Comment can be eg "assigning types to these variables would be too big of a refactoring"

[mrjo118]

Yep, they are needed. I've updated the comments

[laurent22]

I'm not sure I understand why this code is here. It's called FOLDER/NOTE_DELETE but it doesn't delete any note or folder. I guess it's because something else (probably in lib/reducer?) performs the actual deletion? And is it not possible to have this logic in lib/reducer?

[mrjo118]

The code needs to be here because this is where the state of the navigation history is stored for mobile. Reducer.ts basically does the same as what I have written here (also does not perform the actual deletion) and in the same way that needs to be there because it's where the state of the navigation history of the desktop app is