fix(rotate): write one turn at a time and keep the picture still

__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<ArrayBuffer> | null>(() => new Uint8Array([0xFF, 0xD8, 0x99])))
+const setOrientation = vi.hoisted(() => vi.fn<(bytes: Uint8Array<ArrayBuffer>, orientation: number) => Uint8Array<ArrayBuffer> | 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<unknown>()
+			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

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<IFile | undefined>) {
+export function useRotation(file: Ref<IFile | undefined>, 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<IFile | undefined>) {
 	 */
 	const versions = new Map<string, string>()
 
-	/** 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<ArrayBuffer> } | 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<void> = Promise.resolve()
+
+	/** How many writes are queued or in flight, so the last one out clears the flag */
 	let writing = 0
 
 	/**
@@ -103,43 +120,65 @@ export function useRotation(file: Ref<IFile | undefined>) {
 	 * 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<void> {
+	function save(): Promise<void> {
 		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<void> {
 		try {
-			const response = await axios.get(node.encodedSource, { responseType: 'arraybuffer' })
-			const bytes = new Uint8Array(response.data as ArrayBuffer)
+			let bytes: Uint8Array<ArrayBuffer>
+			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(/&quot;|"/g, '')}"` } : undefined,
 			})
 
 			const saved = result.headers?.['oc-etag'] ?? result.headers?.etag
 			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 })

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