From 35976637c2a0a365b7044f89a0eae1475e5c8261 Mon Sep 17 00:00:00 2001 From: skjnldsv Date: Wed, 23 Sep 2026 20:32:04 +0200 Subject: [PATCH] fix(rotate): write one turn at a time and keep the picture still A second turn made while the first was being written read the file before that write landed, then wrote the old orientation back or failed on the stale etag. Writes now queue behind each other, and a turn of the picture just written builds on the bytes we sent instead of a fresh read. The files:node:updated emit made the viewer remount the picture it was already showing turned, so it flashed. The emit stays for the rest of the app, and the viewer now marks the write as its own first, the way it already does for the editor's saves. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: skjnldsv --- __tests__/useRotation.spec.ts | 80 +++++++++++++++++++++++++++++++++- lib/composables/useRotation.ts | 57 ++++++++++++++++++++---- lib/views/Viewer.vue | 10 ++++- 3 files changed, 135 insertions(+), 12 deletions(-) diff --git a/__tests__/useRotation.spec.ts b/__tests__/useRotation.spec.ts index 18fbde4..48c8547 100644 --- a/__tests__/useRotation.spec.ts +++ b/__tests__/useRotation.spec.ts @@ -21,7 +21,7 @@ vi.mock('@nextcloud/dialogs', () => ({ showError: vi.fn(), showSuccess: vi.fn() // here is the orchestration around it: when a turn is written, which file // it is written to, and what is sent with it. The stub composes turns the // way the real one does so the count can be asserted. -const setOrientation = vi.hoisted(() => vi.fn<() => Uint8Array | null>(() => new Uint8Array([0xFF, 0xD8, 0x99]))) +const setOrientation = vi.hoisted(() => vi.fn<(bytes: Uint8Array, orientation: number) => Uint8Array | null>(() => new Uint8Array([0xFF, 0xD8, 0x99]))) vi.mock('@nextcloud/image-editor/jpeg', () => ({ readJpegOrientation: () => 1, // Honours the direction, so a composable that turned the picture the @@ -241,6 +241,84 @@ describe('useRotation', () => { expect(emitBus).toHaveBeenCalledWith('files:node:updated', expect.anything()) }) + it('hands the written file over before announcing it', async () => { + // The viewer marks the update as its own here, or it reloads the + // picture it already shows turned, and the picture flashes + const onWritten = vi.fn(() => expect(emitBus).not.toHaveBeenCalled()) + const node = makeFile() + file = ref(node) + scope = effectScope() + rotation = scope.run(() => useRotation(file, onWritten))! + rotation.rotateLeft() + await settle() + + expect(onWritten).toHaveBeenCalledWith(node) + expect(emitBus).toHaveBeenCalledOnce() + }) + + it('hands nothing over when the write fails', async () => { + axiosPut.mockRejectedValue(new Error('nope')) + const onWritten = vi.fn() + file = ref(makeFile()) + scope = effectScope() + rotation = scope.run(() => useRotation(file, onWritten))! + rotation.rotateLeft() + await settle() + + expect(onWritten).not.toHaveBeenCalled() + expect(emitBus).not.toHaveBeenCalled() + }) + + it('builds a second turn on the bytes it wrote, not on a fresh read', async () => { + // A read can be answered before the previous write lands, and a + // turn computed from it writes the old orientation back + const first = new Uint8Array([0xFF, 0xD8, 0x01]) + setOrientation.mockReturnValueOnce(first) + start(makeFile()) + rotation.rotateLeft() + await settle() + rotation.rotateLeft() + await settle() + + expect(axiosGet).toHaveBeenCalledTimes(1) + expect(setOrientation.mock.calls[1]![0]).toBe(first) + }) + + it('reads the file again once it has moved on to another', async () => { + start(makeFile({ basename: 'first.jpg' })) + rotation.rotateLeft() + await settle() + file.value = makeFile({ basename: 'second.jpg' }) + rotation.rotateLeft() + await settle() + + expect(axiosGet).toHaveBeenCalledTimes(2) + expect(axiosGet.mock.calls[1]![0]).toContain('second.jpg') + }) + + it('holds a turn made during a write until that write lands', async () => { + // Two writes racing each other each start from the file as it was, + // so the second undoes the first, or fails on the stale etag + const { promise, resolve } = Promise.withResolvers() + axiosPut.mockReturnValueOnce(promise) + start(makeFile({ attributes: { etag: 'opened-as' } })) + rotation.rotateLeft() + await settle() + rotation.rotateLeft() + await settle() + + expect(axiosPut).toHaveBeenCalledTimes(1) + expect(rotation.saving.value).toBe(true) + + resolve({ headers: { 'oc-etag': '"written"' } }) + await flushPromises() + + expect(axiosPut).toHaveBeenCalledTimes(2) + expect(axiosPut.mock.calls[1]![2].headers).toEqual({ 'If-Match': '"written"' }) + expect(axiosGet).toHaveBeenCalledTimes(1) + expect(rotation.saving.value).toBe(false) + }) + it('leaves the preview where it is, so the turn on screen holds', async () => { // The preview URL carries the etag: moving it reloads the element // to a freshly turned preview while the turn is still shown on top diff --git a/lib/composables/useRotation.ts b/lib/composables/useRotation.ts index 78a3625..7f4d02d 100644 --- a/lib/composables/useRotation.ts +++ b/lib/composables/useRotation.ts @@ -40,8 +40,10 @@ const ROTATABLE_MIME = 'image/jpeg' * Photos, the mobile clients. * * @param file the file on screen + * @param onWritten called with a file just before its update is announced, + * so the viewer can tell its own write from a change made elsewhere */ -export function useRotation(file: Ref) { +export function useRotation(file: Ref, onWritten?: (node: IFile) => void) { /** Quarter turns anticlockwise the viewer is showing, beyond the file's own */ const turns = ref(0) @@ -65,7 +67,22 @@ export function useRotation(file: Ref) { */ const versions = new Map() - /** How many writes are in flight, so the last one out clears the flag */ + /** + * The bytes last written, and the file they were written to. The next + * turn of that picture builds on them rather than on a fresh read, + * which could overtake the write before it and undo it. Only the last + * file's are kept, since a photo runs to megabytes. + */ + let written: { source: string, bytes: Uint8Array } | undefined + + /** + * The writes, one after another. A turn made while the previous one + * is still on its way waits for it, so each write starts from the + * orientation and the version the previous one left. + */ + let queue: Promise = Promise.resolve() + + /** How many writes are queued or in flight, so the last one out clears the flag */ let writing = 0 /** @@ -103,35 +120,55 @@ export function useRotation(file: Ref) { * quarters leave the file as it was, and a version of a file that did * not change is worse than no version at all. */ - async function save(): Promise { + function save(): Promise { clearTimeout(timer) const owed = pending % 4 const node = target pending = 0 target = undefined if (node === undefined || owed === 0) { - return + return queue } writing++ saving.value = true + queue = queue.then(() => write(node, owed)) + return queue + } + + /** + * Turn the file a number of quarters anticlockwise and write it back. + * + * The rest of the app is told the file changed once it lands, and + * `onWritten` runs first so the viewer, which already shows the turn, + * does not reload the picture for it. + * + * @param node the file to write + * @param owed quarter turns to add, 1 to 3 + */ + async function write(node: IFile, owed: number): Promise { try { - const response = await axios.get(node.encodedSource, { responseType: 'arraybuffer' }) - const bytes = new Uint8Array(response.data as ArrayBuffer) + let bytes: Uint8Array + if (written?.source === node.source) { + bytes = written.bytes + } else { + const response = await axios.get(node.encodedSource, { responseType: 'arraybuffer' }) + bytes = new Uint8Array(response.data as ArrayBuffer) + } let orientation = readJpegOrientation(bytes) for (let turn = 0; turn < owed; turn++) { orientation = rotateOrientation(orientation, 'left') } - const written = setJpegOrientation(bytes, orientation) - if (written === null) { + const turned = setJpegOrientation(bytes, orientation) + if (turned === null) { logger.error('Could not write the orientation of this JPEG', { source: node.source }) showError(t('This image could not be rotated')) return } const known = versions.get(node.source) ?? node.attributes?.etag as string | undefined - const result = await axios.put(node.encodedSource, new Blob([written], { type: ROTATABLE_MIME }), { + const result = await axios.put(node.encodedSource, new Blob([turned], { type: ROTATABLE_MIME }), { headers: known ? { 'If-Match': `"${String(known).replace(/"|"/g, '')}"` } : undefined, }) @@ -139,7 +176,9 @@ export function useRotation(file: Ref) { if (saved) { versions.set(node.source, String(saved).replace(/"/g, '')) } + written = { source: node.source, bytes: turned } + onWritten?.(node) emitBus('files:node:updated', node) } catch (error) { logger.error('Failed to rotate the image', { error }) diff --git a/lib/views/Viewer.vue b/lib/views/Viewer.vue index 97deddd..dbc1318 100644 --- a/lib/views/Viewer.vue +++ b/lib/views/Viewer.vue @@ -326,8 +326,14 @@ function handlerFor(file: IFile): IHandler | undefined { const canEdit = computed(() => currentHandler.value?.canEdit === true && ((currentFile.value?.permissions ?? Permission.NONE) & Permission.UPDATE) !== 0) -// Turning the picture on screen, written back to the file shortly after -const { canRotate, rotateLeft, turns } = useRotation(currentFile) +// Turning the picture on screen, written back to the file shortly after. +// The update a write announces is ours, and the turn is already on screen, +// unless the user has moved on: then nothing reloads for it anyway +const { canRotate, rotateLeft, turns } = useRotation(currentFile, (node) => { + if (node.fileid !== undefined && node.fileid === currentFile.value?.fileid) { + ownSaves.add(node.fileid) + } +}) // What the opener asked for, or nothing at all: every read of this falls // back to the default of that one option, and the service fills in the rest // for a caller that passes no options (see defaultViewerOptions).