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).