laurent22 / joplin

Joplin - the secure note taking and to-do app with synchronisation capabilities for Windows, macOS, Linux, Android and iOS.
https://joplinapp.org
Other
44.28k stars 4.79k forks source link

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

Open laurent22 opened 3 months ago

laurent22 commented 3 months ago

It should be updated immediately (like on mobile?)

DarkFalc0n commented 3 months ago

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

laurent22 commented 3 months ago

All of them probably

DarkFalc0n commented 3 months ago

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 commented 3 months ago

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

DarkFalc0n commented 3 months ago

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 commented 3 months ago

It needs more thought to be put into it

G0maa commented 3 months ago

@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 commented 3 months ago

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

G0maa commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

I can't replicate it on dev anymore