Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
laurent22 opened this issue Mar 25, 2024 · 13 comments
Labels
backlog We'll get to it... eventually... bug It's a bug desktop All desktop platforms

Comments

@laurent22
Copy link
Owner

It should be updated immediately (like on mobile?)

@laurent22 laurent22 added bug It's a bug desktop All desktop platforms high High priority issues v3.0 labels Mar 25, 2024
@DarkFalc0n
Copy link
Contributor

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

@laurent22
Copy link
Owner Author

All of them probably

@DarkFalc0n
Copy link
Contributor

DarkFalc0n commented Mar 25, 2024

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 });
	};

@nebiyuelias1
Copy link

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

@DarkFalc0n
Copy link
Contributor

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
Copy link
Contributor

It needs more thought to be put into it

@G0maa
Copy link

G0maa commented Mar 27, 2024

@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.

@DarkFalc0n
Copy link
Contributor

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

@G0maa
Copy link

G0maa commented Mar 27, 2024

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.

@laurent22
Copy link
Owner Author

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

@G0maa
Copy link

G0maa commented Mar 27, 2024

There's a similar logic in NoteList2.tsx anyway 👀

if (changeEvent.elementId === 'todo-checkbox') {
await Note.save({
id: changeEvent.noteId,
todo_completed: changeEvent.value ? Date.now() : 0,
}, { userSideValidation: true });
props.dispatch({ type: 'NOTE_SORT' });

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
Copy link

G0maa commented Mar 28, 2024

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().

@laurent22 laurent22 self-assigned this Apr 15, 2024
@laurent22 laurent22 added backlog We'll get to it... eventually... and removed high High priority issues v3.0 labels Apr 27, 2024
@laurent22
Copy link
Owner Author

I can't replicate it on dev anymore

@laurent22 laurent22 removed their assignment May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog We'll get to it... eventually... bug It's a bug desktop All desktop platforms
Projects
None yet
Development

No branches or pull requests

4 participants