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

Changing the currently opened note from plugins does not refresh the note content #5955

Open
BeatLink opened this issue Jan 5, 2022 · 13 comments
Labels
bug It's a bug desktop All desktop platforms medium Medium priority issues

Comments

@BeatLink
Copy link

BeatLink commented Jan 5, 2022

When modifying a note from a plugin, for example while using joplin.data.put, if that note is the currently opened one, the content does not update. The user must leave the note then reopen it to see the changes.

Environment

Joplin version: 2.6.10
Platform: Linux
OS specifics: Fedora KDE 35

Steps to reproduce

  1. Run joplin and open a note
  2. From plugin code, modify that same note
  3. Notice that the note content does not update after modification from plugin code
  4. Leave the note and enter it again
  5. Notice that the content is now updated

Describe what you expected to happen

Either all Joplin APIs that change a note should trigger a UI update or the Joplin Plugin API needs to expose a function to refresh the currently opened note.

Logfile

@BeatLink BeatLink added the bug It's a bug label Jan 5, 2022
@laurent22 laurent22 added desktop All desktop platforms high High priority issues labels Jan 5, 2022
@Daeraxa
Copy link
Collaborator

Daeraxa commented Jan 5, 2022

Is this purely a plugin API issue? The same behaviour is seen when a note is modified via sync whilst it is open.

@BeatLink
Copy link
Author

BeatLink commented Jan 5, 2022

Is this purely a plugin API issue? The same behaviour is seen when a note is modified via sync whilst it is open.

Youre right. I think i saw some issues open relating to syncing a note while its open.

@BeatLink
Copy link
Author

BeatLink commented Jan 6, 2022

These are the issues in question: #5582 and #4700. The former seems to have been fixed in a fairly recent commit but im not sure if that fix would solve this issue.

@Daeraxa
Copy link
Collaborator

Daeraxa commented Jan 6, 2022

You are right, somehow I forgot about that feature entirely as I normally only experience the issue on mobile for which it is not resolved (from what I can tell).

@ThibaultJanBeyer
Copy link
Contributor

ThibaultJanBeyer commented Jan 8, 2022

I have the same issue while developing, the current workaround is using editor.setText in addition to the data.put. This is less than ideal, especially since it is not syncing things like the note title, etc.

@ThibaultJanBeyer
Copy link
Contributor

ThibaultJanBeyer commented Jan 9, 2022

Ok so I looked 👀 into this. Found following:

  • in NoteEditor.tsx there is a mapStateToProps
  • When I update the note body/title via API, the state.notes is updated correctly
  • Which in turn, also re-runs the render code in NoteEditor
  • HOWEVER, the changes are not picked up because the NoteEditor.tsx never uses the state information of a note:
    • useFormNote.ts only listens to noteId, isProvisional, formNote changes
    • This is why editor.setText works because that method is just setting the formNote property directly

So we can either:

  • On API PUT also update the formNote hook directly.
  • Or: Within useFormNote listen for state.notes changes and then update the formNote accordingly.
  • ??

So I tried the second option kinda like this (pseudocode):

useEffect(() => {
		const currentNote = state.notes?.find(note => note?.id === formNote?.id);
		if (!currentNote) return;
		setFormNote(currentNote);
	}, [state.notes]);

This does work in such that the note is synced after a PUT in API. Great!
BUT this seems to fuckup the normal behavior: when changing the body or title directly manually it only partially gets synced. That would be a no-go.
So now I'm looking at why this happens and if it can be improved or the other solution used.

Maybe that helps. g2g now if I've more time I can continue looking into it

@laurent22
Copy link
Owner

@ThibaultJanBeyer, I think we have some code that updates the editor note when it is updated via sync. Perhaps we could use a similar hook when it's updated via PUT?

@ThibaultJanBeyer
Copy link
Contributor

ThibaultJanBeyer commented Jan 15, 2022

Sounds good, I tried to find that sync code updating the note. But as I'm new to the code, @laurent22 do you mind pointing me where that is? Thank you!

PS: I also checked and if I check for changes in title and body only it will work (pseudocode):

useEffect(() => {
  const currentNote = await Note.load(noteId);
  if (!currentNote) return;
  if (/* compare currentNote with formNote and they don't match */) {
    await initNoteState(currentNote);
  }
}, [state.notes]);

Rather this or have a special hook on PUT? What do you think?

@laurent22
Copy link
Owner

Check syncStarted in useFormNote

@ThibaultJanBeyer
Copy link
Contributor

ThibaultJanBeyer commented Jan 15, 2022

Well, the only thing that is changed by PUT is the note being saved. Which dispatches a NOTE_UPDATE_ONE and also updated the Note in the state store.

So I can think of 2 solutions, with their respective pros and cons:

  1. We just listen to the notes in the state store state.notes as mentioned above and if the currentNote is not aligned with the formNote, we update the formNote in the same way as syncStarted in useFormNote.
    (+) Easy to implement (a few code-changes only)
    (+) Generic solution: any other case where the current opened note is out of sync with the notes from the store, it gets updated (not just put)
    (-) Requires to compare the note formNote state vs the note from the store on every note saving (which seems fast enough but yeah… Not ideal)
    (-) Might override user-changes is some edge cases, no idea

This could be a generic solution handling both, the PUT update and the SYNC update…

  1. Or we add new state properties like NOTE_API_UPDATE_STARTED and NOTE_API_UPDATE_COMPLETED similar to syncStarted we listen to state.noteApiUpdateStarted or something and handle it in the same way as syncStarted in useFormNote.
    (+) Only checks for updates on the note on PUT calls
    (-) Bloating the code with API call info and also in formNote the similar code as sync started… Quite ugly…
    (-) Non-generic solution (if the note is out of sync due to something else, it'll stay out of sync)

I guess solution 2 is the "safer" choice…

g2g, let me know what you prefer

@laurent22
Copy link
Owner

How about pulling from the state something like props.noteUpdatedTime? (state.note[].updated_time) Then you compare that to the currently loaded note and if it differs you know it's been changed outside the editor.

@ThibaultJanBeyer
Copy link
Contributor

Created a PR, please check it and test all the use-cases. I ran yarn test which passed, but not sure if that tests all edge cases correctly, especially the merge conflicts on sync I'm not sure about :S
I also see that it's failing, got to stop for today again, can look at it another day!

ThibaultJanBeyer pushed a commit to ThibaultJanBeyer/joplin that referenced this issue Apr 2, 2022
laurent22 added a commit that referenced this issue Sep 25, 2023
… plugins or the data API does not refresh the note content

Causes an infinite rendering loop when creating a new note
@laurent22
Copy link
Owner

Unfortunately had to revert it in 34c4b83 due to infinite rendering loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug desktop All desktop platforms medium Medium priority issues
Projects
None yet
4 participants