From b9e6dc4c2beafed0562fa41f6cb05c987fdc2e08 Mon Sep 17 00:00:00 2001 From: Mo Date: Thu, 17 Nov 2022 09:35:45 -0600 Subject: [PATCH] fix: select first item when switching tags (#2026) --- .../ItemList/ItemListController.spec.ts | 106 ++++++++++++++++++ .../ItemList/ItemListController.ts | 36 +++--- .../Controllers/ItemList/ItemsReloadSource.ts | 9 ++ 3 files changed, 132 insertions(+), 19 deletions(-) create mode 100644 packages/web/src/javascripts/Controllers/ItemList/ItemListController.spec.ts create mode 100644 packages/web/src/javascripts/Controllers/ItemList/ItemsReloadSource.ts diff --git a/packages/web/src/javascripts/Controllers/ItemList/ItemListController.spec.ts b/packages/web/src/javascripts/Controllers/ItemList/ItemListController.spec.ts new file mode 100644 index 000000000..c20619996 --- /dev/null +++ b/packages/web/src/javascripts/Controllers/ItemList/ItemListController.spec.ts @@ -0,0 +1,106 @@ +import { SNTag } from '@standardnotes/snjs' +import { ContentType } from '@standardnotes/common' +import { InternalEventBus } from '@standardnotes/services' +import { WebApplication } from '@/Application/Application' +import { LinkingController } from '../LinkingController' +import { NavigationController } from '../Navigation/NavigationController' +import { NotesController } from '../NotesController' +import { SearchOptionsController } from '../SearchOptionsController' +import { SelectedItemsController } from '../SelectedItemsController' +import { ItemListController } from './ItemListController' +import { ItemsReloadSource } from './ItemsReloadSource' + +describe('item list controller', () => { + let controller: ItemListController + let navigationController: NavigationController + let selectionController: SelectedItemsController + + beforeEach(() => { + const application = {} as jest.Mocked + application.streamItems = jest.fn() + application.addEventObserver = jest.fn() + application.addWebEventObserver = jest.fn() + + navigationController = {} as jest.Mocked + selectionController = {} as jest.Mocked + + const searchOptionsController = {} as jest.Mocked + const notesController = {} as jest.Mocked + const linkingController = {} as jest.Mocked + const eventBus = new InternalEventBus() + + controller = new ItemListController( + application, + navigationController, + searchOptionsController, + selectionController, + notesController, + linkingController, + eventBus, + ) + }) + + describe('shouldSelectFirstItem', () => { + beforeEach(() => { + controller.getFirstNonProtectedItem = jest.fn() + + Object.defineProperty(selectionController, 'selectedUuids', { + get: () => new Set(), + configurable: true, + }) + }) + + it('should return false first item is file', () => { + controller.getFirstNonProtectedItem = jest.fn().mockReturnValue({ + content_type: ContentType.File, + }) + + expect(controller.shouldSelectFirstItem(ItemsReloadSource.UserTriggeredTagChange)).toBe(false) + }) + + it('should return false if selected tag is daily entry', () => { + const tag = { + isDailyEntry: true, + content_type: ContentType.Tag, + } as jest.Mocked + + Object.defineProperty(navigationController, 'selected', { + get: () => tag, + }) + + expect(controller.shouldSelectFirstItem(ItemsReloadSource.UserTriggeredTagChange)).toBe(false) + }) + + it('should return true if user triggered tag change', () => { + const tag = { + content_type: ContentType.Tag, + } as jest.Mocked + + Object.defineProperty(navigationController, 'selected', { + get: () => tag, + }) + + expect(controller.shouldSelectFirstItem(ItemsReloadSource.UserTriggeredTagChange)).toBe(true) + }) + + it('should return false if not user triggered tag change and there is an existing selected item', () => { + const tag = { + content_type: ContentType.Tag, + } as jest.Mocked + + Object.defineProperty(selectionController, 'selectedUuids', { + get: () => new Set(['123']), + }) + + Object.defineProperty(navigationController, 'selected', { + get: () => tag, + }) + + expect(controller.shouldSelectFirstItem(ItemsReloadSource.ItemStream)).toBe(false) + }) + + it('should return true if there are no selected items, even if not user triggered', () => { + expect(controller.shouldSelectFirstItem(ItemsReloadSource.ItemStream)).toBe(true) + }) + }) +}) diff --git a/packages/web/src/javascripts/Controllers/ItemList/ItemListController.ts b/packages/web/src/javascripts/Controllers/ItemList/ItemListController.ts index 09ff43387..eea5dcd43 100644 --- a/packages/web/src/javascripts/Controllers/ItemList/ItemListController.ts +++ b/packages/web/src/javascripts/Controllers/ItemList/ItemListController.ts @@ -38,22 +38,13 @@ import { log, LoggingDomain } from '@/Logging' import { NoteViewController } from '@/Components/NoteView/Controller/NoteViewController' import { FileViewController } from '@/Components/NoteView/Controller/FileViewController' import { TemplateNoteViewAutofocusBehavior } from '@/Components/NoteView/Controller/TemplateNoteViewControllerOptions' +import { ItemsReloadSource } from './ItemsReloadSource' const MinNoteCellHeight = 51.0 const DefaultListNumNotes = 20 const ElementIdSearchBar = 'search-bar' const ElementIdScrollContainer = 'notes-scrollable' -enum ItemsReloadSource { - ItemStream, - SyncEvent, - DisplayOptionsChange, - Pagination, - TagChange, - UserTriggeredTagChange, - FilterTextChange, -} - export class ItemListController extends AbstractViewController implements InternalEventHandlerInterface { completedFullSync = false noteFilterText = '' @@ -122,7 +113,7 @@ export class ItemListController extends AbstractViewController implements Intern application.streamItems([ContentType.Tag, ContentType.SmartView], async ({ changed, inserted }) => { const tags = [...changed, ...inserted] - const { didReloadItems } = await this.reloadDisplayPreferences() + const { didReloadItems } = await this.reloadDisplayPreferences({ userTriggered: false }) if (!didReloadItems) { /** A tag could have changed its relationships, so we need to reload the filter */ this.reloadNotesDisplayOptions() @@ -138,7 +129,7 @@ export class ItemListController extends AbstractViewController implements Intern this.disposers.push( application.addEventObserver(async () => { - void this.reloadDisplayPreferences() + void this.reloadDisplayPreferences({ userTriggered: false }) }, ApplicationEvent.PreferencesChanged), ) @@ -427,11 +418,11 @@ export class ItemListController extends AbstractViewController implements Intern return false } - private shouldSelectActiveItem = (activeItem: SNNote | FileItem | undefined) => { - return activeItem && !this.selectionController.isItemSelected(activeItem) + private shouldSelectActiveItem = (activeItem: SNNote | FileItem) => { + return !this.selectionController.isItemSelected(activeItem) } - private shouldSelectFirstItem = (itemsReloadSource: ItemsReloadSource) => { + shouldSelectFirstItem = (itemsReloadSource: ItemsReloadSource) => { const item = this.getFirstNonProtectedItem() if (item && isFile(item)) { return false @@ -445,6 +436,7 @@ export class ItemListController extends AbstractViewController implements Intern const userChangedTag = itemsReloadSource === ItemsReloadSource.UserTriggeredTagChange const hasNoSelectedItem = !this.selectionController.selectedUuids.size + return userChangedTag || hasNoSelectedItem } @@ -466,7 +458,7 @@ export class ItemListController extends AbstractViewController implements Intern log(LoggingDomain.Selection, 'Selecting next item after closing active one') this.selectionController.selectNextItem() } - } else if (this.shouldSelectActiveItem(activeItem) && activeItem) { + } else if (activeItem && this.shouldSelectActiveItem(activeItem)) { log(LoggingDomain.Selection, 'Selecting active item') await this.selectionController.selectItem(activeItem.uuid).catch(console.error) } else if (this.shouldSelectFirstItem(itemsReloadSource)) { @@ -510,7 +502,11 @@ export class ItemListController extends AbstractViewController implements Intern this.application.items.setPrimaryItemDisplayOptions(criteria) } - reloadDisplayPreferences = async (): Promise<{ didReloadItems: boolean }> => { + reloadDisplayPreferences = async ({ + userTriggered, + }: { + userTriggered: boolean + }): Promise<{ didReloadItems: boolean }> => { const newDisplayOptions = {} as DisplayOptions const newWebDisplayOptions = {} as WebDisplayOptions const selectedTag = this.navigationController.selected @@ -601,7 +597,9 @@ export class ItemListController extends AbstractViewController implements Intern this.reloadNotesDisplayOptions() - await this.reloadItems(ItemsReloadSource.DisplayOptionsChange) + await this.reloadItems( + userTriggered ? ItemsReloadSource.UserTriggeredTagChange : ItemsReloadSource.DisplayOptionsChange, + ) const didSortByChange = currentSortBy !== this.displayOptions.sortBy const didSortDirectionChange = currentSortDirection !== this.displayOptions.sortDirection @@ -813,7 +811,7 @@ export class ItemListController extends AbstractViewController implements Intern this.resetPagination() - const { didReloadItems } = await this.reloadDisplayPreferences() + const { didReloadItems } = await this.reloadDisplayPreferences({ userTriggered }) if (!didReloadItems) { this.reloadNotesDisplayOptions() diff --git a/packages/web/src/javascripts/Controllers/ItemList/ItemsReloadSource.ts b/packages/web/src/javascripts/Controllers/ItemList/ItemsReloadSource.ts new file mode 100644 index 000000000..314cdb24d --- /dev/null +++ b/packages/web/src/javascripts/Controllers/ItemList/ItemsReloadSource.ts @@ -0,0 +1,9 @@ +export enum ItemsReloadSource { + ItemStream, + SyncEvent, + DisplayOptionsChange, + Pagination, + TagChange, + UserTriggeredTagChange, + FilterTextChange, +}