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

Mobile (testing environment): Last editor input is lost if changes are made less than about 100ms before closing the app #11125

Open
personalizedrefrigerator opened this issue Sep 26, 2024 · 6 comments
Labels
bug It's a bug high High priority issues mobile All mobile platforms

Comments

@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Sep 26, 2024

Operating system

Linux

Joplin version

86c035c

Desktop version info

N/A

Current behaviour

Updating the mobile note editor, then quickly unmounting the component results in loss of the last-entered data.

In particular,

  • In a testing environment, waiting > 100ms since the last edit seems to consistently allow the last changes to be saved.
  • If the component is unmounted < 100ms (e.g. 98ms) since the last edit, the last changes are lost.

On a real device, this 100ms threshold may be greater.

Test case: commit.

Expected behaviour

The last change to the editor should be preserved, even if made very briefly before closing the editor.

Notes

Logs

No response

@mrjo118
Copy link
Contributor

mrjo118 commented Nov 3, 2024

Does unmounting the component happen just when exiting the app, or does it happen also when exiting edit mode to save the note?

@mrjo118
Copy link
Contributor

mrjo118 commented Nov 4, 2024

I can see that if I add a log entry in the componentWillUnmount() function in app-mobile Note.tsx, that it gets called when exiting the note completely i.e. going back to the note list. It isn't getting called if I close the app via the Android recent list while the note is open.

Should the title of this issue therefore instead be: 'Last editor input is lost if changes are made less than about 100ms before closing the note' ?

Also, maybe the problem and solution outlined here could aid with a resolution for the issue? https://medium.com/@minduladilthushan/resolving-the-state-not-updating-immediately-issue-in-react-js-febf5959c0cf

@personalizedrefrigerator
Copy link
Collaborator Author

Should the title of this issue therefore instead be: 'Last editor input is lost if changes are made less than about 100ms before closing the note' ?

Possibly, though it depends on the cause of the issue:

  • Note.tsx uses an AsyncActionQueue to schedule note saves. If scheduled items in the queue aren't being processed when the app is closed from the recent app list, then the title should remain "before closing the app".
  • The note editor runs in a WebView, which transfers its updated content to Note.tsx when changed. If this issue is caused when the last editor content fails to transfer to Note.tsx, then the issue title should be changed to end with "before closing the note".

@mrjo118
Copy link
Contributor

mrjo118 commented Nov 4, 2024

I had a play and I was able to make your test in https://github.com/laurent22/joplin/pull/11126/files pass with a 50ms wait for jest.advanceTimersByTimeAsync.

It looks like this code in Note.tsx is the cause:
// Avoid saving immediately -- the NoteEditor's content isn't controlled by its props
// and updating this.state.note immediately causes slow rerenders.
//
// See #10130
private onMarkdownEditorTextChange = debounce((event: EditorChangeEvent) => {
shared.noteComponent_change(this, 'body', event.value);
}, 100);

If I change the timeout passed to debounce as 1, then the test passes.
It seems to be that the fix you put in to prevent freezing when typing quickly is causing missed input when exiting a note too quickly :P.

It can be surmised that if processAllNow (called when unmounting) completes before debounce does, then the change to be added following completion of debounce will be lost. This is because the change is not added to the save action queue until after the 100 ms, and the component would have been already unmounted.

So I think to fix this, in onMarkdownEditorTextChange the change needs to be made without using debounce, but instead push a slightly modified makeSaveAction which waits the 100ms before before executing it's logic. There is a risk though that if doing the delay in the action, that you can get a backlog of save action queue items because the queue runs synchronously. But maybe this could be worked around by instead of waiting 100ms explicitly, record a timestamp 100ms after the current time in the action when creating it, then only apply a delay if that time has not yet passed

@mrjo118
Copy link
Contributor

mrjo118 commented Nov 4, 2024

Apologies, I jumped the gun a bit. After adding some additional logging and changing the code a bit, I can confirm that a note change scheduled using debounce does indeed execute, even after componentWillUnmount and the save action queue has completed. So that is not the issue.
Looking at your old PR #10134 , if in some cases it takes several seconds to work through the backlog of the save action queue, then indeed the user may have exited the note and then exited the app as well within that time. As soon as the user exits the app, any background processing will stop, so any items left in the queue will be lost. I confirmed this by adding logging at the end of the componentWillUnmount method, setting a delay of 10 seconds and writing another log in the callback. If I exit the note so the first log entry is written, then exit the app using the back button before the 10 seconds is up, the second log entry never gets written. The componentWillUnmount method is asynchronous so the user will be back on the list view before the processing completes.

Would it be possible with a React Native app to make some sort of visual indicator on the note screen while the note is saving, which persists onto the note list after exiting the note, until the save is completed? Or another option and maybe simpler would be to make the back button on the note (when switching from edit to to view mode) wait at intervals until the save action queue is empty, before executing the code to switch it to view mode. That way it will block the UI until all changes are saved

@mrjo118
Copy link
Contributor

mrjo118 commented Nov 5, 2024

I made some further tests, and it turns out there are 3 issues:
-Last editor input is lost if the app is closed before all queued changes have been saved - In manual testing of queued changes, I found that the 100 ms buffer between registering save actions greatly reduces the backlog and time required to execute it. As the the time to execute is unlikely to become significantly long, and large notes take a bit of time to render the note after exiting edit mode anyway, this is probably not that likely to occur providing that the user always exits the note before closing the app
-Last editor input is lost if the user exits a note within 100 ms after making changes, where consecutive changes were made - I have attached a video below demonstrating this using the prebuilt Joplin 3.1.5 apk on Android emulator. The user would have to be super quick to make a change and exit both edit and view mode of the note for this occur, so again it's unlikely to happen. It's probably related to the fact that the note was unmounted before the change could be even finish rendering in view mode, so the state might not yet have been updated
-Last editor input is sometimes lost when exiting the note editor after making changes - I have been able to fairly consistently reproduce this using the prebuilt Joplin 3.1.5 apk on Android emulator using a very large note by making text changes following pasting a chunk of text. I found that there is a specific error which appears in the logs during the period which I edit the note (which looks to align roughly with when I pasted the text) when the issue is reproduced. I've raised a new bug report with my findings for it #11317. This could possibly be the cause of the intermittent lost input issues which Joplin has had for a long time

Last.Input.Lost.After.Exiting.Note.Quickly.2024-11-05.21-45-23-418.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug high High priority issues mobile All mobile platforms
Projects
None yet
Development

No branches or pull requests

3 participants