joplin icon indicating copy to clipboard operation
joplin copied to clipboard

When marking a to-do as completed there's a slight delay before the list is updated

Open laurent22 opened this issue 3 months ago • 13 comments

It should be updated immediately (like on mobile?)

laurent22 avatar Mar 25 '24 12:03 laurent22

Which OS has this issue? I run joplin on Arch Linux and it is working instantly for me.

DarkFalc0n avatar Mar 25 '24 14:03 DarkFalc0n

All of them probably

laurent22 avatar Mar 25 '24 15:03 laurent22

The only probable reason I could find is that the await Note.save(...) holds the event emit (at /packages/app-desktop/gui/NoteList/NoteList.tsx). The emit could be initiated before the save function, because the latter can take up some time for larger notes.

	const noteItem_checkboxClick = async (event: any, item: any) => {
		const checked = event.target.checked;
		const newNote = {
			id: item.id,
			todo_completed: checked ? time.unixMs() : 0,
		};
		await Note.save(newNote, { userSideValidation: true });
		eventManager.emit(EventName.TodoToggle, { noteId: item.id, note: newNote });
	};

DarkFalc0n avatar Mar 25 '24 16:03 DarkFalc0n

@DarkFalc0n - so just interchanging the two lines should fix it?

nebiyuelias1 avatar Mar 26 '24 06:03 nebiyuelias1

Interchanging could lead to other cases of bad error handling where there is an exception and the note state couldn't be saved but the check will still update. The current order of functions makes more sense since any error in saving the note can be reflected even before updating the list (and the check mark)

DarkFalc0n avatar Mar 26 '24 06:03 DarkFalc0n

It needs more thought to be put into it

DarkFalc0n avatar Mar 26 '24 06:03 DarkFalc0n

@DarkFalc0n, I think you could interchange them & add e.g. try, catch block:

		eventManager.emit(EventName.TodoToggle, { noteId: item.id, note: newNote });
		try {
			await Note.save(newNote, { userSideValidation: true });
		} catch(e) {
			eventManager.emit(EventName.TodoToggle, { noteId: item.id, note: newNote });
		}

Maybe re-throw the error if needed.

G0maa avatar Mar 27 '24 09:03 G0maa

This could definitely work, lets see what others think of this

DarkFalc0n avatar Mar 27 '24 09:03 DarkFalc0n

Actually, I'm not sure if this particular NoteList.tsx component is being used, I think the one that's being used is NoteList2.tsx.

G0maa avatar Mar 27 '24 19:03 G0maa

That's right, we'll need to remove NoteList.tsx. I kept it there for some time just in case there's a major issue with the new note list

laurent22 avatar Mar 27 '24 20:03 laurent22

There's a similar logic in NoteList2.tsx anyway 👀 https://github.com/laurent22/joplin/blob/06aa64016f2d501c37882cf1db43dba18668f143/packages/app-desktop/gui/NoteListItem/NoteListItem.tsx#L53-L59

But I'm not sure if it's really what's causing this behaviour, I added few ~~hindered~~ to-dos and it feels the same regardless of commenting the await or not.

G0maa avatar Mar 27 '24 20:03 G0maa

I've been playing around this by adding:

await new Promise(resolve => setTimeout(resolve, 2000));

before Note.save() and props.dispatch(), you can see the behavior in this gif: 2024-03-28 14-26-22

I think the solution I suggested above can't be applied here, props.dispatch() depends on the Note.save().

G0maa avatar Mar 28 '24 13:03 G0maa

I can't replicate it on dev anymore

laurent22 avatar Apr 27 '24 09:04 laurent22