Desktop: Fixes #7731: Make note sort update logic use correct prior sort and drop-grouping (#7737)

packages/app-desktop/gui/NoteList/NoteList.tsx

@@ -182,7 +182,7 @@ const NoteListComponent = (props: Props) => {
 		const targetNoteIndex = dragTargetNoteIndex_(event);
 		const noteIds = JSON.parse(dt.getData('text/x-jop-note-ids'));
 
-		void Note.insertNotesAt(props.selectedFolderId, noteIds, targetNoteIndex);
+		void Note.insertNotesAt(props.selectedFolderId, noteIds, targetNoteIndex, props.uncompletedTodosOnTop, props.showCompletedTodos);
 	};
 
 
@@ -528,6 +528,8 @@ const mapStateToProps = (state: AppState) => {
 		provisionalNoteIds: state.provisionalNoteIds,
 		isInsertingNotes: state.isInsertingNotes,
 		noteSortOrder: state.settings['notes.sortOrder.field'],
+		uncompletedTodosOnTop: state.settings.uncompletedTodosOnTop,
+		showCompletedTodos: state.settings.showCompletedTodos,
 		highlightedWords: state.highlightedWords,
 		plugins: state.pluginService.plugins,
 		customCss: state.customCss,

packages/app-desktop/gui/NoteList/types.ts

@@ -12,6 +12,8 @@ export interface Props {
 	customCss: string;
 	notesParentType: string;
 	noteSortOrder: string;
+	uncompletedTodosOnTop: boolean;
+	showCompletedTodos: boolean;
 	resizableLayoutEventEmitter: any;
 	isInsertingNotes: boolean;
 	folders: FolderEntity[];

packages/lib/models/Note.ts

@@ -251,7 +251,7 @@ export default class Note extends BaseItem {
 		return output;
 	}
 
-	// Note: sort logic must be duplicated in previews();
+	// Note: sort logic must be duplicated in previews().
 	static sortNotes(notes: NoteEntity[], orders: any[], uncompletedTodosOnTop: boolean) {
 		const noteOnTop = (note: NoteEntity) => {
 			return uncompletedTodosOnTop && note.is_todo && !note.todo_completed;
@@ -815,11 +815,9 @@ export default class Note extends BaseItem {
 
 	// When notes are sorted in "custom order", they are sorted by the "order" field first and,
 	// in those cases, where the order field is the same for some notes, by created time.
-	static customOrderByColumns(type: string = null) {
+	// Further sorting by todo completion status, if enabled, is handled separately.
-		if (!type) type = 'object';
+	static customOrderByColumns() {
-		if (type === 'object') return [{ by: 'order', dir: 'DESC' }, { by: 'user_created_time', dir: 'DESC' }];
+		return [{ by: 'order', dir: 'DESC' }, { by: 'user_created_time', dir: 'DESC' }];
-		if (type === 'string') return 'ORDER BY `order` DESC, user_created_time DESC';
-		throw new Error(`Invalid type: ${type}`);
 	}
 
 	// Update the note "order" field without changing the user timestamps,
@@ -836,7 +834,7 @@ export default class Note extends BaseItem {
 	// of unecessary updates, so it's the caller's responsability to update
 	// the UI once the call is finished. This is done by listening to the
 	// NOTE_IS_INSERTING_NOTES action in the application middleware.
-	static async insertNotesAt(folderId: string, noteIds: string[], index: number) {
+	static async insertNotesAt(folderId: string, noteIds: string[], index: number, uncompletedTodosOnTop: boolean, showCompletedTodos: boolean) {
 		if (!noteIds.length) return;
 
 		const defer = () => {
@@ -852,13 +850,21 @@ export default class Note extends BaseItem {
 		});
 
 		try {
+			const getSortedNotes = async (folderId: string) => {
 				const noteSql = `
-				SELECT id, \`order\`, user_created_time, user_updated_time
+					SELECT id, \`order\`, user_created_time, user_updated_time,
+						is_todo, todo_completed, title
 					FROM notes
-				WHERE is_conflict = 0 AND parent_id = ?
+					WHERE
-			${this.customOrderByColumns('string')}`;
+						is_conflict = 0
+						${showCompletedTodos ? '' : 'AND todo_completed = 0'}
+					AND parent_id = ?
+				`;
+				const notes_raw: NoteEntity[] = await this.modelSelectAll(noteSql, [folderId]);
+				return await this.sortNotes(notes_raw, this.customOrderByColumns(), uncompletedTodosOnTop);
+			};
 
-			let notes = await this.modelSelectAll(noteSql, [folderId]);
+			let notes = await getSortedNotes(folderId);
 
 			// If the target index is the same as the source note index, exit now
 			for (let i = 0; i < notes.length; i++) {
@@ -878,7 +884,39 @@ export default class Note extends BaseItem {
 				}
 			}
 
-			if (hasSetOrder) notes = await this.modelSelectAll(noteSql, [folderId]);
+			if (hasSetOrder) notes = await getSortedNotes(folderId);
+
+			// If uncompletedTodosOnTop, then we should only consider the existing
+			// order of same-completion-window notes. A completed todo or non-todo
+			// dragged into the uncompleted list should end up at the start of the
+			// completed/non-todo list, and an uncompleted todo dragged into the
+			// completed/non-todo list should end up at the end of the uncompleted
+			// list.
+			// To make this determination we need to know the completion status of the
+			// item we are dropping. We apply several simplifications:
+			//  - We only care about completion status if uncompletedTodosOnTop
+			//  - We only care about completion status / position if the item being
+			//     moved is already in the current list; not if it is dropped from
+			//     another notebook.
+			//  - We only care about the completion status of the first item being
+			//     moved. If a moving selection includes both uncompleted and
+			//     completed/non-todo items, then the completed/non-todo items will
+			//     not get "correct" position (although even defining a "more correct"
+			//     outcome in such a case might be challenging).
+			let relevantExistingNoteCount = notes.length;
+			let firstRelevantNoteIndex = 0;
+			let lastRelevantNoteIndex = notes.length - 1;
+			if (uncompletedTodosOnTop) {
+				const uncompletedTest = (n: NoteEntity) => !(n.todo_completed || !n.is_todo);
+				const targetNoteInNotebook = notes.find(n => n.id === noteIds[0]);
+				if (targetNoteInNotebook) {
+					const targetUncompleted = uncompletedTest(targetNoteInNotebook);
+					const noteFilterCondition = targetUncompleted ? (n: NoteEntity) => uncompletedTest(n) : (n: NoteEntity) => !uncompletedTest(n);
+					relevantExistingNoteCount = notes.filter(noteFilterCondition).length;
+					firstRelevantNoteIndex = notes.findIndex(noteFilterCondition);
+					lastRelevantNoteIndex = notes.map(noteFilterCondition).lastIndexOf(true);
+				}
+			}
 
 			// Find the order value for the first note to be inserted,
 			// and the increment between the order values of each inserted notes.
@@ -886,14 +924,14 @@ export default class Note extends BaseItem {
 			let intervalBetweenNotes = 0;
 			const defaultIntevalBetweeNotes = 60 * 60 * 1000;
 
-			if (!notes.length) { // If there's no notes in the target notebook
+			if (!relevantExistingNoteCount) { // If there's no (relevant) notes in the target notebook
 				newOrder = Date.now();
 				intervalBetweenNotes = defaultIntevalBetweeNotes;
-			} else if (index >= notes.length) { // Insert at the end
+			} else if (index > lastRelevantNoteIndex) { // Insert at the end (of relevant group)
-				intervalBetweenNotes = notes[notes.length - 1].order / (noteIds.length + 1);
+				intervalBetweenNotes = notes[lastRelevantNoteIndex].order / (noteIds.length + 1);
-				newOrder = notes[notes.length - 1].order - intervalBetweenNotes;
+				newOrder = notes[lastRelevantNoteIndex].order - intervalBetweenNotes;
-			} else if (index === 0) { // Insert at the beginning
+			} else if (index <= firstRelevantNoteIndex) { // Insert at the beginning (of relevant group)
-				const firstNoteOrder = notes[0].order;
+				const firstNoteOrder = notes[firstRelevantNoteIndex].order;
 				if (firstNoteOrder >= Date.now()) {
 					intervalBetweenNotes = defaultIntevalBetweeNotes;
 					newOrder = firstNoteOrder + defaultIntevalBetweeNotes;

packages/lib/models/Note_CustomSortOrder.test.js

@@ -204,4 +204,76 @@ describe('models/Note_CustomSortOrder', function() {
 		expect(sortedNotes[3].id).toBe(notes[0].id);
 	}));
 
+	it('should account for completion-hidden and uncompleted-on-top todos', (async () => {
+		const folder1 = await Folder.save({});
+
+		const notes = [];
+		notes.push(await Note.save({ order: 1006, parent_id: folder1.id, is_todo: true, todo_completed: time.unixMs() })); await time.msleep(2);
+		notes.push(await Note.save({ order: 1005, parent_id: folder1.id, is_todo: true })); await time.msleep(2);
+		notes.push(await Note.save({ order: 1004, parent_id: folder1.id })); await time.msleep(2);
+		notes.push(await Note.save({ order: 1003, parent_id: folder1.id })); await time.msleep(2);
+		notes.push(await Note.save({ order: 1002, parent_id: folder1.id })); await time.msleep(2);
+		notes.push(await Note.save({ order: 1001, parent_id: folder1.id, is_todo: true })); await time.msleep(2);
+		notes.push(await Note.save({ order: 1000, parent_id: folder1.id, is_todo: true })); await time.msleep(2);
+
+		const sortNotes = async () => await Note.previews(folder1.id, {
+			fields: ['id', 'order', 'user_created_time', 'is_todo', 'todo_completed'],
+			order: Note.customOrderByColumns(),
+			uncompletedTodosOnTop: true,
+			showCompletedTodos: false,
+		});
+
+		// Initial sort is as expected in UI, with completed item missing, and uncompleted
+		// todos at top.
+		const initialSortedNotes = await sortNotes();
+		expect(initialSortedNotes.length).toBe(6);
+		expect(initialSortedNotes[0].id).toBe(notes[1].id);
+		expect(initialSortedNotes[1].id).toBe(notes[5].id);
+		expect(initialSortedNotes[2].id).toBe(notes[6].id);
+		expect(initialSortedNotes[3].id).toBe(notes[2].id);
+		expect(initialSortedNotes[4].id).toBe(notes[3].id);
+		expect(initialSortedNotes[5].id).toBe(notes[4].id);
+
+		// Regular reorder - place a note within its window
+		await Note.insertNotesAt(folder1.id, [initialSortedNotes[5].id], 4, true, false);
+
+		// Post-reorder, the updated note is at the index requested
+		const resortedNotes1 = await sortNotes();
+		expect(resortedNotes1.length).toBe(6);
+		expect(resortedNotes1[0].id).toBe(initialSortedNotes[0].id);
+		expect(resortedNotes1[1].id).toBe(initialSortedNotes[1].id);
+		expect(resortedNotes1[2].id).toBe(initialSortedNotes[2].id);
+		expect(resortedNotes1[3].id).toBe(initialSortedNotes[3].id);
+		expect(resortedNotes1[4].id).toBe(initialSortedNotes[5].id);
+		expect(resortedNotes1[5].id).toBe(initialSortedNotes[4].id);
+
+		// Limit reorder - place the note into another window
+		await Note.insertNotesAt(folder1.id, [resortedNotes1[4].id], 2, true, false);
+
+		// Post-reorder, the updated note is not at the index requested, but
+		// as close as possible: at the top edge of its window.
+		const resortedNotes2 = await sortNotes();
+		expect(resortedNotes2.length).toBe(6);
+		expect(resortedNotes2[0].id).toBe(resortedNotes1[0].id);
+		expect(resortedNotes2[1].id).toBe(resortedNotes1[1].id);
+		expect(resortedNotes2[2].id).toBe(resortedNotes1[2].id);
+		expect(resortedNotes2[3].id).toBe(resortedNotes1[4].id);
+		expect(resortedNotes2[4].id).toBe(resortedNotes1[3].id);
+		expect(resortedNotes2[5].id).toBe(resortedNotes1[5].id);
+
+		// Limit reorder - place an uncompleted todo into another window
+		await Note.insertNotesAt(folder1.id, [resortedNotes2[0].id], 4, true, false);
+
+		// Post-reorder, the updated todo is not at the index requested, but
+		// as close as possible: at the lower edge of its window.
+		const resortedNotes3 = await sortNotes();
+		expect(resortedNotes3.length).toBe(6);
+		expect(resortedNotes3[0].id).toBe(resortedNotes2[1].id);
+		expect(resortedNotes3[1].id).toBe(resortedNotes2[2].id);
+		expect(resortedNotes3[2].id).toBe(resortedNotes2[0].id);
+		expect(resortedNotes3[3].id).toBe(resortedNotes2[3].id);
+		expect(resortedNotes3[4].id).toBe(resortedNotes2[4].id);
+		expect(resortedNotes3[5].id).toBe(resortedNotes2[5].id);
+	}));
+
 });