From a1821d607e8a7305500d9f112255a63f11b8f04a Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Sat, 23 Sep 2023 17:50:24 +0100 Subject: [PATCH] Desktop: Fixes #5955: Changing the currently opened note from plugins or the data API does not refresh the note content --- .../tests/support/plugins/simple/index.js | 19 +++++- .../app-desktop/gui/NoteEditor/NoteEditor.tsx | 31 +++++++++- .../app-desktop/gui/NoteEditor/utils/types.ts | 5 +- .../gui/NoteEditor/utils/useFormNote.test.ts | 50 ++++++++++++++-- .../gui/NoteEditor/utils/useFormNote.ts | 60 +++++++++++++------ packages/app-desktop/testPluginDemo.sh | 21 ++++--- 6 files changed, 148 insertions(+), 38 deletions(-) diff --git a/packages/app-cli/tests/support/plugins/simple/index.js b/packages/app-cli/tests/support/plugins/simple/index.js index 0c35fd31b1..1501f873e0 100644 --- a/packages/app-cli/tests/support/plugins/simple/index.js +++ b/packages/app-cli/tests/support/plugins/simple/index.js @@ -1,6 +1,21 @@ joplin.plugins.register({ onStart: async function() { - const folder = await joplin.data.post(['folders'], null, { title: "my plugin folder" }); - await joplin.data.post(['notes'], null, { parent_id: folder.id, title: "testing plugin!" }); + // const folder = await joplin.data.post(['folders'], null, { title: "my plugin folder" }); + // await joplin.data.post(['notes'], null, { parent_id: folder.id, title: "testing plugin!" }); + + await joplin.commands.register({ + name: 'updateCurrentNote', + label: 'Update current note via the data API', + iconName: 'fas fa-music', + execute: async () => { + const noteIds = await joplin.workspace.selectedNoteIds(); + const noteId = noteIds.length === 1 ? noteIds[0] : null; + if (!noteId) return; + console.info('Modifying current note...'); + await joplin.data.put(['notes', noteId], null, { body: "New note body " + Date.now() }); + }, + }); + + await joplin.views.toolbarButtons.create('updateCurrentNoteButton', 'updateCurrentNote', 'editorToolbar'); }, }); \ No newline at end of file diff --git a/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx b/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx index f94c3daeab..c991774c8b 100644 --- a/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx +++ b/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx @@ -76,11 +76,18 @@ function NoteEditor(props: NoteEditorProps) { }, []); const effectiveNoteId = useEffectiveNoteId(props); + const effectiveNote = props.notes.find(n => n.id === effectiveNoteId); const { formNote, setFormNote, isNewNote, resourceInfos } = useFormNote({ syncStarted: props.syncStarted, decryptionStarted: props.decryptionStarted, noteId: effectiveNoteId, + + // The effective updated_time property of the note. It may be different + // from the last time the note was saved, if it was modified outside the + // editor (eg. via API). + dbNote: effectiveNote ? { id: effectiveNote.id, updated_time: effectiveNote.updated_time } : { id: '', updated_time: 0 }, + isProvisional: props.isProvisional, titleInputRef: titleInputRef, editorRef: editorRef, @@ -120,12 +127,32 @@ function NoteEditor(props: NoteEditorProps) { return async function() { const note = await formNoteToNote(formNote); reg.logger().debug('Saving note...', note); - const savedNote: any = await Note.save(note); + const noteUpdatedTime = Date.now(); + + // First we set the formNote object, then we save the note. We + // do it in that order, otherwise `useFormNote` will be rendered + // with the newly saved note and the timestamp of that note will + // be more recent that the one in the editor, which will trigger + // an update. We do not want this since we already have the + // latest changes. + // + // It also means that we manually set the timestamp, so that we + // have it before the note is saved. setFormNote((prev: FormNote) => { - return { ...prev, user_updated_time: savedNote.user_updated_time, hasChanged: false }; + return { + ...prev, + user_updated_time: noteUpdatedTime, + updated_time: noteUpdatedTime, + hasChanged: false, + }; }); + const savedNote = await Note.save({ + ...note, + updated_time: noteUpdatedTime, + }, { autoTimestamp: false }); + void ExternalEditWatcher.instance().updateNoteFile(savedNote); props.dispatch({ diff --git a/packages/app-desktop/gui/NoteEditor/utils/types.ts b/packages/app-desktop/gui/NoteEditor/utils/types.ts index 0c0c6a60c5..5f3ed39d5e 100644 --- a/packages/app-desktop/gui/NoteEditor/utils/types.ts +++ b/packages/app-desktop/gui/NoteEditor/utils/types.ts @@ -5,6 +5,7 @@ import { MarkupLanguage } from '@joplin/renderer'; import { RenderResult, RenderResultPluginAsset } from '@joplin/renderer/MarkupToHtml'; import { MarkupToHtmlOptions } from './useMarkupToHtml'; import { Dispatch } from 'redux'; +import { NoteEntity } from '@joplin/lib/services/database/types'; export interface AllAssetsOptions { contentMaxWidthTarget?: string; @@ -20,7 +21,7 @@ export interface NoteEditorProps { dispatch: Dispatch; selectedNoteIds: string[]; selectedFolderId: string; - notes: any[]; + notes: NoteEntity[]; watchedNoteFiles: string[]; isProvisional: boolean; editorNoteStatuses: any; @@ -104,6 +105,7 @@ export interface FormNote { markup_language: number; user_updated_time: number; encryption_applied: number; + updated_time: number; hasChanged: boolean; @@ -154,6 +156,7 @@ export function defaultFormNote(): FormNote { hasChanged: false, user_updated_time: 0, encryption_applied: 0, + updated_time: 0, }; } diff --git a/packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.ts b/packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.ts index bb62209392..a14cc9792a 100644 --- a/packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.ts +++ b/packages/app-desktop/gui/NoteEditor/utils/useFormNote.test.ts @@ -1,8 +1,19 @@ import Note from '@joplin/lib/models/Note'; import { setupDatabaseAndSynchronizer, switchClient } from '@joplin/lib/testing/test-utils'; import { renderHook } from '@testing-library/react-hooks'; -import useFormNote, { HookDependencies } from './useFormNote'; +import useFormNote, { DbNote, HookDependencies } from './useFormNote'; +const defaultFormNoteProps: HookDependencies = { + syncStarted: false, + decryptionStarted: false, + noteId: '', + isProvisional: false, + titleInputRef: null, + editorRef: null, + onBeforeLoad: ()=>{}, + onAfterLoad: ()=>{}, + dbNote: { id: '', updated_time: 0 }, +}; describe('useFormNote', () => { beforeEach(async () => { @@ -15,14 +26,10 @@ describe('useFormNote', () => { const makeFormNoteProps = (syncStarted: boolean, decryptionStarted: boolean): HookDependencies => { return { + ...defaultFormNoteProps, syncStarted, decryptionStarted, noteId: testNote.id, - isProvisional: false, - titleInputRef: null, - editorRef: null, - onBeforeLoad: ()=>{}, - onAfterLoad: ()=>{}, }; }; @@ -70,4 +77,35 @@ describe('useFormNote', () => { }); }); }); + + it('should reload the note when it is changed outside of the editor', async () => { + const note = await Note.save({ title: 'Test Note!' }); + + const makeFormNoteProps = (dbNote: DbNote): HookDependencies => { + return { + ...defaultFormNoteProps, + noteId: note.id, + dbNote, + }; + }; + + const formNote = renderHook(props => useFormNote(props), { + initialProps: makeFormNoteProps({ id: note.id, updated_time: note.updated_time }), + }); + + await formNote.waitFor(() => { + expect(formNote.result.current.formNote.title).toBe('Test Note!'); + }); + + // Simulate the note being modified outside the editor + const modifiedNote = await Note.save({ id: note.id, title: 'Modified' }); + + // NoteEditor then would update `dbNote` + formNote.rerender(makeFormNoteProps({ id: note.id, updated_time: modifiedNote.updated_time })); + + await formNote.waitFor(() => { + expect(formNote.result.current.formNote.title).toBe('Modified'); + }); + }); + }); diff --git a/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts b/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts index 96a330dc84..a8c335aa56 100644 --- a/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts +++ b/packages/app-desktop/gui/NoteEditor/utils/useFormNote.ts @@ -7,21 +7,30 @@ import { splitHtml } from '@joplin/renderer/HtmlToHtml'; import Setting from '@joplin/lib/models/Setting'; import usePrevious from '../../hooks/usePrevious'; import ResourceEditWatcher from '@joplin/lib/services/ResourceEditWatcher/index'; - -const { MarkupToHtml } = require('@joplin/renderer'); +import { MarkupToHtml } from '@joplin/renderer'; import Note from '@joplin/lib/models/Note'; -import { reg } from '@joplin/lib/registry'; import ResourceFetcher from '@joplin/lib/services/ResourceFetcher'; import DecryptionWorker from '@joplin/lib/services/DecryptionWorker'; +import { NoteEntity } from '@joplin/lib/services/database/types'; +import Logger from '@joplin/utils/Logger'; + +const logger = Logger.create('useFormNote'); export interface OnLoadEvent { formNote: FormNote; + updated_time: number; +} + +export interface DbNote { + id: string; + updated_time: number; } export interface HookDependencies { syncStarted: boolean; decryptionStarted: boolean; noteId: string; + dbNote: DbNote; isProvisional: boolean; titleInputRef: any; editorRef: any; @@ -63,7 +72,7 @@ function resourceInfosChanged(a: ResourceInfos, b: ResourceInfos): boolean { export default function useFormNote(dependencies: HookDependencies) { const { - syncStarted, decryptionStarted, noteId, isProvisional, titleInputRef, editorRef, onBeforeLoad, onAfterLoad, + syncStarted, decryptionStarted, noteId, isProvisional, titleInputRef, editorRef, onBeforeLoad, onAfterLoad, dbNote, } = dependencies; const [formNote, setFormNote] = useState(defaultFormNote()); @@ -77,7 +86,7 @@ export default function useFormNote(dependencies: HookDependencies) { // a new refresh. const [formNoteRefeshScheduled, setFormNoteRefreshScheduled] = useState(0); - async function initNoteState(n: any) { + async function initNoteState(n: NoteEntity) { let originalCss = ''; if (n.markup_language === MarkupToHtml.MARKUP_LANGUAGE_HTML) { @@ -85,7 +94,7 @@ export default function useFormNote(dependencies: HookDependencies) { originalCss = splitted.css; } - const newFormNote = { + const newFormNote: FormNote = { id: n.id, title: n.title, body: n.body, @@ -99,6 +108,7 @@ export default function useFormNote(dependencies: HookDependencies) { hasChanged: false, user_updated_time: n.user_updated_time, encryption_applied: n.encryption_applied, + updated_time: n.updated_time, }; // Note that for performance reason,the call to setResourceInfos should @@ -116,7 +126,7 @@ export default function useFormNote(dependencies: HookDependencies) { useEffect(() => { if (formNoteRefeshScheduled <= 0) return () => {}; - reg.logger().info('Sync has finished and note has never been changed - reloading it'); + logger.info('Sync has finished and note has never been changed - reloading it'); let cancelled = false; @@ -128,7 +138,7 @@ export default function useFormNote(dependencies: HookDependencies) { // it would not have been loaded in the editor (due to note selection changing // on delete) if (!n) { - reg.logger().warn('Trying to reload note that has been deleted:', noteId); + logger.warn('Trying to reload note that has been deleted:', noteId); return; } @@ -150,19 +160,19 @@ export default function useFormNote(dependencies: HookDependencies) { }, [formNoteRefeshScheduled]); useEffect(() => { - // Check that synchronisation has just finished - and - // if the note has never been changed, we reload it. - // If the note has already been changed, it's a conflict - // that's already been handled by the synchronizer. + // Check that synchronisation has just finished - and if the note has + // never been changed, we reload it. If the note has already been + // changed, it's a conflict that's already been handled by the + // synchronizer. const decryptionJustEnded = prevDecryptionStarted && !decryptionStarted; const syncJustEnded = prevSyncStarted && !syncStarted; if (!decryptionJustEnded && !syncJustEnded) return; if (formNote.hasChanged) return; - // Refresh the form note. - // This is kept separate from the above logic so that when prevSyncStarted is changed - // from true to false, it doesn't cancel the note from loading. + // Refresh the form note. This is kept separate from the above logic so + // that when prevSyncStarted is changed from true to false, it doesn't + // cancel the note from loading. refreshFormNote(); }, [ prevSyncStarted, syncStarted, @@ -170,6 +180,18 @@ export default function useFormNote(dependencies: HookDependencies) { formNote.hasChanged, refreshFormNote, ]); + useEffect(() => { + // Something's not fully initialised - we skip the check + if (!dbNote.id || !dbNote.updated_time || !formNote.updated_time) return; + + // If the note in the database is more recent that the note in editor, + // it was modified outside the editor, so we refresh it. + if (dbNote.id === formNote.id && dbNote.updated_time > formNote.updated_time) { + logger.info('Note has been changed outside the editor - reloading it'); + refreshFormNote(); + } + }, [dbNote.id, dbNote.updated_time, formNote.updated_time, formNote.id, refreshFormNote]); + useEffect(() => { if (!noteId) { if (formNote.id) setFormNote(defaultFormNote()); @@ -180,7 +202,7 @@ export default function useFormNote(dependencies: HookDependencies) { let cancelled = false; - reg.logger().debug('Loading existing note', noteId); + logger.debug('Loading existing note', noteId); function handleAutoFocus(noteIsTodo: boolean) { if (!isProvisional) return; @@ -200,15 +222,15 @@ export default function useFormNote(dependencies: HookDependencies) { const n = await Note.load(noteId); if (cancelled) return; if (!n) throw new Error(`Cannot find note with ID: ${noteId}`); - reg.logger().debug('Loaded note:', n); + logger.debug('Loaded note:', n); - await onBeforeLoad({ formNote }); + await onBeforeLoad({ formNote, updated_time: 0 }); const newFormNote = await initNoteState(n); setIsNewNote(isProvisional); - await onAfterLoad({ formNote: newFormNote }); + await onAfterLoad({ formNote: newFormNote, updated_time: n.updated_time }); handleAutoFocus(!!n.is_todo); } diff --git a/packages/app-desktop/testPluginDemo.sh b/packages/app-desktop/testPluginDemo.sh index bfcac942b1..ce960cd718 100755 --- a/packages/app-desktop/testPluginDemo.sh +++ b/packages/app-desktop/testPluginDemo.sh @@ -5,17 +5,22 @@ SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" TEMP_PATH=~/src/plugin-tests -PLUGIN_PATH=~/src/joplin/packages/app-cli/tests/support/plugins/note_list_renderer +NEED_COMPILING=0 +PLUGIN_PATH=~/src/joplin/packages/app-cli/tests/support/plugins/simple -mkdir -p "$TEMP_PATH" -PLUGIN_NAME=$(echo "$PLUGIN_PATH" | awk -F/ '{print $NF}') -TEMP_PLUGIN_PATH="$TEMP_PATH/$PLUGIN_NAME" +if [[ $NEED_COMPILING == 1 ]]; then + mkdir -p "$TEMP_PATH" + PLUGIN_NAME=$(echo "$PLUGIN_PATH" | awk -F/ '{print $NF}') + TEMP_PLUGIN_PATH="$TEMP_PATH/$PLUGIN_NAME" -echo "Copying from: $PLUGIN_PATH" -echo "To: $TEMP_PLUGIN_PATH" + echo "Copying from: $PLUGIN_PATH" + echo "To: $TEMP_PLUGIN_PATH" -rsync -a --delete "$PLUGIN_PATH/" "$TEMP_PLUGIN_PATH/" + rsync -a --delete "$PLUGIN_PATH/" "$TEMP_PLUGIN_PATH/" -npm install --prefix="$TEMP_PLUGIN_PATH" && yarn start --dev-plugins "$TEMP_PLUGIN_PATH" + npm install --prefix="$TEMP_PLUGIN_PATH" && yarn start --dev-plugins "$TEMP_PLUGIN_PATH" +else + yarn start --dev-plugins "$PLUGIN_PATH" +fi # Add eg "--profile $HOME/.config/joplindev-desktop-1" to test with a different profile \ No newline at end of file