From ed31d8202b3ef0b3fbd2c3a7ebe667706e434400 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Tue, 18 Jun 2024 01:59:08 -0700 Subject: [PATCH] Mobile: Fixes #10589: Fix selected note changes on moving to a different folder (#10630) --- .eslintignore | 1 + .gitignore | 1 + .../app-mobile/components/screens/Note.tsx | 18 +++-- .../components/shared/note-screen-shared.ts | 1 + packages/lib/models/Note.ts | 5 +- packages/lib/models/utils/types.ts | 1 + .../lib/{reducer.test.js => reducer.test.ts} | 69 ++++++++++++++----- packages/lib/reducer.ts | 6 +- 8 files changed, 78 insertions(+), 24 deletions(-) rename packages/lib/{reducer.test.js => reducer.test.ts} (89%) diff --git a/.eslintignore b/.eslintignore index 6df412a8e..bda5afa94 100644 --- a/.eslintignore +++ b/.eslintignore @@ -964,6 +964,7 @@ packages/lib/ntp.js packages/lib/onedrive-api.test.js packages/lib/onedrive-api.js packages/lib/path-utils.js +packages/lib/reducer.test.js packages/lib/reducer.js packages/lib/registry.test.js packages/lib/registry.js diff --git a/.gitignore b/.gitignore index a990d2f41..dbc20c568 100644 --- a/.gitignore +++ b/.gitignore @@ -943,6 +943,7 @@ packages/lib/ntp.js packages/lib/onedrive-api.test.js packages/lib/onedrive-api.js packages/lib/path-utils.js +packages/lib/reducer.test.js packages/lib/reducer.js packages/lib/registry.test.js packages/lib/registry.js diff --git a/packages/app-mobile/components/screens/Note.tsx b/packages/app-mobile/components/screens/Note.tsx index dc0fddd88..5ff345f8b 100644 --- a/packages/app-mobile/components/screens/Note.tsx +++ b/packages/app-mobile/components/screens/Note.tsx @@ -1399,15 +1399,18 @@ class NoteScreenComponent extends BaseScreenComponent implements B }, 50); } - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied - private async folderPickerOptions_valueChanged(itemValue: any) { + private async folderPickerOptions_valueChanged(itemValue: string) { const note = this.state.note; const isProvisionalNote = this.props.provisionalNoteIds.includes(note.id); if (isProvisionalNote) { await this.saveNoteButton_press(itemValue); } else { - await Note.moveToFolder(note.id, itemValue); + await Note.moveToFolder( + note.id, + itemValue, + { dispatchOptions: { preserveSelection: true } }, + ); } note.parent_id = itemValue; @@ -1428,7 +1431,13 @@ class NoteScreenComponent extends BaseScreenComponent implements B onValueChange: this.folderPickerOptions_valueChanged, }; - if (this.folderPickerOptions_ && options.selectedFolderId === this.folderPickerOptions_.selectedFolderId) return this.folderPickerOptions_; + if ( + this.folderPickerOptions_ + && options.selectedFolderId === this.folderPickerOptions_.selectedFolderId + && options.enabled === this.folderPickerOptions_.enabled + ) { + return this.folderPickerOptions_; + } this.folderPickerOptions_ = options; return this.folderPickerOptions_; @@ -1683,7 +1692,6 @@ const NoteScreen = connect((state: AppState) => { return { noteId: state.selectedNoteIds.length ? state.selectedNoteIds[0] : null, noteHash: state.selectedNoteHash, - folderId: state.selectedFolderId, itemType: state.selectedItemType, folders: state.folders, searchQuery: state.searchQuery, diff --git a/packages/lib/components/shared/note-screen-shared.ts b/packages/lib/components/shared/note-screen-shared.ts index 57035e7fb..e4de3d9d3 100644 --- a/packages/lib/components/shared/note-screen-shared.ts +++ b/packages/lib/components/shared/note-screen-shared.ts @@ -102,6 +102,7 @@ shared.saveNoteButton_press = async function(comp: BaseNoteScreenComponent, fold const saveOptions = { userSideValidation: true, fields: BaseModel.diffObjectsFields(comp.state.lastSavedNote, note), + dispatchOptions: { preserveSelection: true }, }; const hasAutoTitle = comp.state.newAndNoTitleChangeNoteId || (isProvisionalNote && !note.title); diff --git a/packages/lib/models/Note.ts b/packages/lib/models/Note.ts index 83dfa8b1f..a346fd535 100644 --- a/packages/lib/models/Note.ts +++ b/packages/lib/models/Note.ts @@ -624,7 +624,7 @@ export default class Note extends BaseItem { }); } - public static async moveToFolder(noteId: string, folderId: string) { + public static async moveToFolder(noteId: string, folderId: string, saveOptions: SaveOptions|null = null) { if (folderId === this.getClass('Folder').conflictFolderId()) throw new Error(_('Cannot move note to "%s" notebook', this.getClass('Folder').conflictFolderTitle())); // When moving a note to a different folder, the user timestamp is not @@ -643,7 +643,7 @@ export default class Note extends BaseItem { updated_time: time.unixMs(), }; - return Note.save(modifiedNote, { autoTimestamp: false }); + return Note.save(modifiedNote, { autoTimestamp: false, ...saveOptions }); } public static changeNoteType(note: NoteEntity, type: string) { @@ -841,6 +841,7 @@ export default class Note extends BaseItem { provisional: isProvisional, ignoreProvisionalFlag: ignoreProvisionalFlag, changedFields: changedFields, + ...options?.dispatchOptions, }); } diff --git a/packages/lib/models/utils/types.ts b/packages/lib/models/utils/types.ts index b4b7e7973..bbea1b0ec 100644 --- a/packages/lib/models/utils/types.ts +++ b/packages/lib/models/utils/types.ts @@ -49,6 +49,7 @@ export interface SaveOptions { provisional?: boolean; ignoreProvisionalFlag?: boolean; dispatchUpdateAction?: boolean; + dispatchOptions?: { preserveSelection: boolean }; changeSource?: number; disableReadOnlyCheck?: boolean; } diff --git a/packages/lib/reducer.test.js b/packages/lib/reducer.test.ts similarity index 89% rename from packages/lib/reducer.test.js rename to packages/lib/reducer.test.ts index 4d9e00dc5..87b9abec5 100644 --- a/packages/lib/reducer.test.js +++ b/packages/lib/reducer.test.ts @@ -1,9 +1,11 @@ -const { setupDatabaseAndSynchronizer, switchClient, createNTestNotes, createNTestFolders, createNTestTags } = require('./testing/test-utils.js'); -const reducer = require('./reducer').default; -const { defaultState, MAX_HISTORY } = require('./reducer'); +import { setupDatabaseAndSynchronizer, switchClient, createNTestNotes, createNTestFolders, createNTestTags } from './testing/test-utils'; +import reducer, { defaultState, MAX_HISTORY, State } from './reducer'; +import { BaseItemEntity, FolderEntity, NoteEntity, TagEntity } from './services/database/types'; +import Note from './models/Note'; +import BaseModel from './BaseModel'; // const { ALL_NOTES_FILTER_ID } = require('./reserved-ids'); -function initTestState(folders, selectedFolderIndex, notes, selectedNoteIndexes, tags = null, selectedTagIndex = null) { +function initTestState(folders: FolderEntity[], selectedFolderIndex: number, notes: NoteEntity[], selectedNoteIndexes: number[], tags: TagEntity[] = null, selectedTagIndex: number = null) { let state = defaultState; if (selectedFolderIndex !== null) { @@ -32,7 +34,7 @@ function initTestState(folders, selectedFolderIndex, notes, selectedNoteIndexes, return state; } -function goToNote(notes, selectedNoteIndexes, state) { +function goToNote(notes: NoteEntity[], selectedNoteIndexes: number[], state: State) { if (selectedNoteIndexes !== null) { const selectedIds = []; for (let i = 0; i < selectedNoteIndexes.length; i++) { @@ -43,7 +45,7 @@ function goToNote(notes, selectedNoteIndexes, state) { return state; } -function goBackWard(state) { +function goBackWard(state: State) { if (!state.backwardHistoryNotes.length) return state; state = reducer(state, { type: 'HISTORY_BACKWARD', @@ -51,7 +53,7 @@ function goBackWard(state) { return state; } -function goForward(state) { +function goForward(state: State) { if (!state.forwardHistoryNotes.length) return state; state = reducer(state, { type: 'HISTORY_FORWARD', @@ -59,8 +61,8 @@ function goForward(state) { return state; } -function createExpectedState(items, keepIndexes, selectedIndexes) { - const expected = { items: [], selectedIds: [] }; +function createExpectedState(items: BaseItemEntity[], keepIndexes: number[], selectedIndexes: number[]) { + const expected = { items: [] as BaseItemEntity[], selectedIds: [] as string[] }; for (let i = 0; i < selectedIndexes.length; i++) { expected.selectedIds.push(items[selectedIndexes[i]].id); @@ -71,7 +73,7 @@ function createExpectedState(items, keepIndexes, selectedIndexes) { return expected; } -function getIds(items, indexes = null) { +function getIds(items: BaseItemEntity[], indexes: number[]|null = null) { const ids = []; for (let i = 0; i < items.length; i++) { if (!indexes || i in indexes) { @@ -86,7 +88,6 @@ describe('reducer', () => { beforeEach(async () => { await setupDatabaseAndSynchronizer(1); await switchClient(1); - }); // tests for NOTE_DELETE @@ -301,7 +302,7 @@ describe('reducer', () => { // tests for TAG_DELETE it('should delete selected tag', (async () => { const tags = await createNTestTags(5); - let state = initTestState(null, null, null, null, tags, [2]); + let state = initTestState(null, null, null, null, tags, 2); // test action state = reducer(state, { type: 'TAG_DELETE', id: tags[2].id }); @@ -314,7 +315,7 @@ describe('reducer', () => { it('should delete tag when a tag above is selected', (async () => { const tags = await createNTestTags(5); - let state = initTestState(null, null, null, null, tags, [2]); + let state = initTestState(null, null, null, null, tags, 2); // test action state = reducer(state, { type: 'TAG_DELETE', id: tags[4].id }); @@ -327,7 +328,7 @@ describe('reducer', () => { it('should delete tag when a tag below is selected', (async () => { const tags = await createNTestTags(5); - let state = initTestState(null, null, null, null, tags, [2]); + let state = initTestState(null, null, null, null, tags, 2); // test action state = reducer(state, { type: 'TAG_DELETE', id: tags[0].id }); @@ -616,7 +617,7 @@ describe('reducer', () => { const oldTags = tags.slice(0, 5); { // Case 1. The input which is deep equal to the current state.tags doesn't change state.tags. - const oldState = initTestState(null, null, null, null, oldTags, [2]); + const oldState = initTestState(null, null, null, null, oldTags, 2); const newTags = oldTags.slice(); // test action const newState = reducer(oldState, { type: 'TAG_UPDATE_ALL', items: newTags }); @@ -624,11 +625,47 @@ describe('reducer', () => { } { // Case 2. A different input changes state.tags. - const oldState = initTestState(null, null, null, null, oldTags, [2]); + const oldState = initTestState(null, null, null, null, oldTags, 2); const newTags = oldTags.slice().splice(3, 1, tags[5]); // test action const newState = reducer(oldState, { type: 'TAG_UPDATE_ALL', items: newTags }); expect(newState.tags).not.toBe(oldState.tags); } }); + + // Regression test for #10589. + it.each([ + true, false, + ])('should preserve note selection if specified while moving a note (preserveSelection: %j)', async (preserveSelection) => { + const folders = await createNTestFolders(3); + const notes = await createNTestNotes(5, folders[0]); + + // select the 1st folder and the 1st note + let state = initTestState(folders, 0, notes, [0]); + state = goToNote(notes, [0], state); + + expect(state.selectedNoteIds).toHaveLength(1); + + BaseModel.dispatch = jest.fn((action: unknown) => { + state = reducer(state, action); + }); + + // Dispatching with preserveSelection should preserve the selected note (as is done on + // mobile). + await Note.moveToFolder( + state.selectedNoteIds[0], + folders[1].id, + preserveSelection ? { dispatchOptions: { preserveSelection: true } } : undefined, + ); + + expect(BaseModel.dispatch).toHaveBeenCalled(); + if (preserveSelection) { + expect(state.selectedNoteIds).toMatchObject([notes[0].id]); + } else { + expect(state.selectedNoteIds).toMatchObject([notes[1].id]); + } + // Original note should no longer be present in the sidebar + expect(state.notes.every(n => n.id !== notes[0].id)).toBe(true); + expect(state.selectedFolderId).toBe(folders[0].id); + }); }); diff --git a/packages/lib/reducer.ts b/packages/lib/reducer.ts index 8abc4515c..d7ae98f2a 100644 --- a/packages/lib/reducer.ts +++ b/packages/lib/reducer.ts @@ -1005,7 +1005,11 @@ const reducer = produce((draft: Draft = defaultState, action: any) => { draft.notes = newNotes; - if (noteFolderHasChanged) { + // Ensure that the selected note is still in the current folder. + // For example, if the user drags the current note to a different folder, + // a new note should be selected. + // In some cases, however, the selection needs to be preserved (e.g. the mobile app). + if (noteFolderHasChanged && !action.preserveSelection) { let newIndex = movedNotePreviousIndex; if (newIndex >= newNotes.length) newIndex = newNotes.length - 1; if (!newNotes.length) newIndex = -1;