manual backport of #2426, refs #2411, Improve image attachment management

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>

6 files changed, 139 insertions, 156 deletions

diff --git a/cypress/integration/images.spec.js b/cypress/integration/images.spec.js
index d1fe42170..74efe74d4 100644
--- a/cypress/integration/images.spec.js
+++ b/cypress/integration/images.spec.js
@@ -82,7 +82,7 @@ const checkImage = (documentId, imageName, imageId, index) => {
 	cy.log('Check the image is visible and well formed', { documentId, imageName, imageId, index, encodedName })
 	return new Cypress.Promise((resolve, reject) => {
 		cy.get('#editor [data-component="image-view"]')
-			.filter('[data-src="text://image?imageFileName=' + encodedName + '"]')
+			.filter('[data-src=".attachments.' + documentId + '/' + encodedName + '"]')
 			.find('.image__view') // wait for load finish
 			.within(($el) => {
 				// keep track that we have created this image in the attachment dir
diff --git a/lib/Service/ImageService.php b/lib/Service/ImageService.php
index 8c2bc1ee2..901575355 100644
--- a/lib/Service/ImageService.php
+++ b/lib/Service/ImageService.php
@@ -156,6 +156,7 @@ class ImageService {
 		$savedFile = $saveDir->newFile($fileName, $newFileResource);
 		return [
 			'name' => $fileName,
+			'dirname' => $saveDir->getName(),
 			'id' => $savedFile->getId(),
 			'documentId' => $textFile->getId(),
 		];
@@ -183,6 +184,7 @@ class ImageService {
 		$savedFile = $saveDir->newFile($fileName, $newFileResource);
 		return [
 			'name' => $fileName,
+			'dirname' => $saveDir->getName(),
 			'id' => $savedFile->getId(),
 			'documentId' => $textFile->getId(),
 		];
@@ -227,6 +229,7 @@ class ImageService {
 			// get file type and name
 			return [
 				'name' => $fileName,
+				'dirname' => $saveDir->getName(),
 				'id' => $targetFile->getId(),
 				'documentId' => $textFile->getId(),
 			];
@@ -457,7 +460,7 @@ class ImageService {
 					$attachmentsByName[$attNode->getName()] = $attNode;
 				}
 
-				$contentAttachmentNames = $this->getAttachmentNamesFromContent($textFile->getContent());
+				$contentAttachmentNames = $this->getAttachmentNamesFromContent($textFile->getContent(), $fileId);
 
 				$toDelete = array_diff(array_keys($attachmentsByName), $contentAttachmentNames);
 				foreach ($toDelete as $name) {
@@ -476,20 +479,35 @@ class ImageService {
 	 * @param string $content
 	 * @return array
 	 */
-	public static function getAttachmentNamesFromContent(string $content): array {
-		$matches = [];
+	public static function getAttachmentNamesFromContent(string $content, int $fileId): array {
+		$oldMatches = [];
 		preg_match_all(
 			// simple version with .+ between the brackets
 			// '/\!\[.+\]\(text:\/\/image\?[^)]*imageFileName=([^)&]+)\)/',
 			// complex version of php-markdown
+			// matches ![ANY_CONSIDERED_CORRECT_BY_PHP-MARKDOWN](text://image?ANYTHING&imageFileName=FILE_NAME) and captures FILE_NAME
 			'/\!\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[\])*\])*\])*\])*\])*\])*\]\(text:\/\/image\?[^)]*imageFileName=([^)&]+)\)/',
 			$content,
+			$oldMatches,
+			PREG_SET_ORDER
+		);
+		$oldNames = array_map(static function (array $match) {
+			return urldecode($match[1]);
+		}, $oldMatches);
+
+		$matches = [];
+		// matches ![ANY_CONSIDERED_CORRECT_BY_PHP-MARKDOWN](.attachments.DOCUMENT_ID/ANY_FILE_NAME) and captures FILE_NAME
+		preg_match_all(
+			'/\!\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[(?>[^\[\]]+|\[\])*\])*\])*\])*\])*\])*\]\(\.attachments\.'.$fileId.'\/([^)&]+)\)/',
+			$content,
 			$matches,
 			PREG_SET_ORDER
 		);
-		return array_map(static function (array $match) {
+		$names = array_map(static function (array $match) {
 			return urldecode($match[1]);
 		}, $matches);
+
+		return array_merge($names, $oldNames);
 	}
 
 	/**
diff --git a/src/components/EditorWrapper.vue b/src/components/EditorWrapper.vue
index 18b256cc7..f2cfaefa7 100644
--- a/src/components/EditorWrapper.vue
+++ b/src/components/EditorWrapper.vue
@@ -616,7 +616,7 @@ export default {
 			}
 
 			return this.$syncService.uploadImage(file).then((response) => {
-				this.insertAttachmentImage(response.data?.name, response.data?.id, position)
+				this.insertAttachmentImage(response.data?.name, response.data?.id, position, response.data?.dirname)
 			}).catch((error) => {
 				console.error(error)
 				showError(error?.response?.data?.error)
@@ -625,7 +625,7 @@ export default {
 		insertImagePath(imagePath) {
 			this.uploadingImages = true
 			this.$syncService.insertImageFile(imagePath).then((response) => {
-				this.insertAttachmentImage(response.data?.name, response.data?.id)
+				this.insertAttachmentImage(response.data?.name, response.data?.id, null, response.data?.dirname)
 			}).catch((error) => {
 				console.error(error)
 				showError(error?.response?.data?.error)
@@ -633,10 +633,10 @@ export default {
 				this.uploadingImages = false
 			})
 		},
-		insertAttachmentImage(name, fileId, position = null) {
+		insertAttachmentImage(name, fileId, position = null, dirname = '') {
 			// inspired by the fixedEncodeURIComponent function suggested in
 			// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent
-			const src = 'text://image?imageFileName='
+			const src = dirname + '/'
 				+ encodeURIComponent(name).replace(/[!'()*]/g, (c) => {
 					return '%' + c.charCodeAt(0).toString(16).toUpperCase()
 				})
diff --git a/src/nodes/ImageView.vue b/src/nodes/ImageView.vue
index 1c6ecd70f..595be8862 100644
--- a/src/nodes/ImageView.vue
+++ b/src/nodes/ImageView.vue
@@ -131,6 +131,7 @@ export default {
 			loaded: false,
 			failed: false,
 			showIcons: false,
+			imageUrl: null,
 		}
 	},
 	computed: {
@@ -151,39 +152,6 @@ export default {
 					})
 			}
 		},
-		imageUrl() {
-			if (this.src.startsWith('text://')) {
-				const documentId = this.currentSession?.documentId
-				const sessionId = this.currentSession?.id
-				const sessionToken = this.currentSession?.token
-				const imageFileName = getQueryVariable(this.src, 'imageFileName')
-				if (getCurrentUser() || !this.token) {
-					return generateUrl('/apps/text/image?documentId={documentId}&sessionId={sessionId}&sessionToken={sessionToken}&imageFileName={imageFileName}',
-						{
-							documentId,
-							sessionId,
-							sessionToken,
-							imageFileName,
-						})
-				} else {
-					return generateUrl('/apps/text/image?documentId={documentId}&sessionId={sessionId}&sessionToken={sessionToken}&imageFileName={imageFileName}&shareToken={shareToken}',
-						{
-							documentId,
-							sessionId,
-							sessionToken,
-							imageFileName,
-							shareToken: this.token,
-						})
-				}
-			}
-			if (this.isRemoteUrl || this.isPreviewUrl || this.isDataUrl) {
-				return this.src
-			}
-			if (this.hasPreview && this.mime !== 'image/gif') {
-				return this.previewUrl
-			}
-			return this.davUrl
-		},
 		isRemoteUrl() {
 			return this.src.startsWith('http://')
 				|| this.src.startsWith('https://')
@@ -195,6 +163,9 @@ export default {
 		isDataUrl() {
 			return this.src.startsWith('data:')
 		},
+		isDirectUrl() {
+			return (this.isRemoteUrl || this.isPreviewUrl || this.isDataUrl)
+		},
 		basename() {
 			return decodeURI(this.src.split('?')[0])
 		},
@@ -280,18 +251,59 @@ export default {
 			this.loaded = true
 			return
 		}
-		const img = new Image()
-		img.onload = () => {
-			this.imageLoaded = true
-		}
-		img.onerror = () => {
+		this.init().catch((e) => {
+			this.onImageLoadFailure()
+		})
+	},
+	methods: {
+		async init() {
+			if (this.src.startsWith('text://')) {
+				const imageFileName = getQueryVariable(this.src, 'imageFileName')
+				return this.loadImage(this.getTextApiUrl(imageFileName))
+			}
+			if (this.src.startsWith(`.attachments.${this.currentSession?.documentId}/`)) {
+				const imageFileName = decodeURIComponent(this.src.replace(`.attachments.${this.currentSession?.documentId}/`, '').split('?')[0])
+				return this.loadImage(this.getTextApiUrl(imageFileName))
+			}
+			if (this.isDirectUrl) {
+				return this.loadImage(this.src)
+			}
+			if (this.hasPreview && this.mime !== 'image/gif') {
+				return this.loadImage(this.previewUrl)
+			}
+			// if it starts with '.attachments.1234/'
+			if (this.src.match(/^\.attachments\.\d+\//)) {
+				// try the webdav url
+				return this.loadImage(this.davUrl).catch((e) => {
+					// try the attachment API
+					const imageFileName = decodeURIComponent(this.src.replace(/\.attachments\.\d+\//, '').split('?')[0])
+					const textApiUrl = this.getTextApiUrl(imageFileName)
+					return this.loadImage(textApiUrl).then(() => {
+						// TODO if attachment works, rewrite the url with correct document ID
+					})
+				})
+			}
+			this.loadImage(this.davUrl)
+		},
+		async loadImage(imageUrl) {
+			return new Promise((resolve, reject) => {
+				const img = new Image()
+				img.onload = () => {
+					this.imageUrl = imageUrl
+					this.imageLoaded = true
+					resolve()
+				}
+				img.onerror = (e) => {
+					reject(e)
+				}
+				img.src = imageUrl
+			})
+		},
+		onImageLoadFailure() {
 			this.failed = true
 			this.imageLoaded = false
 			this.loaded = true
-		}
-		img.src = this.imageUrl
-	},
-	methods: {
+		},
 		updateAlt() {
 			this.alt = this.$refs.altInput.value
 		},
@@ -301,6 +313,28 @@ export default {
 				this.editor.commands.scrollIntoView()
 			})
 		},
+		getTextApiUrl(imageFileName) {
+			const documentId = this.currentSession?.documentId
+			const sessionId = this.currentSession?.id
+			const sessionToken = this.currentSession?.token
+			if (getCurrentUser() || !this.token) {
+				return generateUrl('/apps/text/image?documentId={documentId}&sessionId={sessionId}&sessionToken={sessionToken}&imageFileName={imageFileName}',
+					{
+						documentId,
+						sessionId,
+						sessionToken,
+						imageFileName,
+					})
+			}
+			return generateUrl('/apps/text/image?documentId={documentId}&sessionId={sessionId}&sessionToken={sessionToken}&imageFileName={imageFileName}&shareToken={shareToken}',
+				{
+					documentId,
+					sessionId,
+					sessionToken,
+					imageFileName,
+					shareToken: this.token,
+				})
+		},
 	},
 }
 </script>
diff --git a/src/tests/nodes/ImageView.spec.js b/src/tests/nodes/ImageView.spec.js
deleted file mode 100644
index 773e4f14c..000000000
--- a/src/tests/nodes/ImageView.spec.js
+++ /dev/null
@@ -1,87 +0,0 @@
-import { shallowMount } from '@vue/test-utils'
-import ImageView from '../../nodes/ImageView.vue'
-
-global.OC = {
-	requestToken: '123',
-	config: {modRewriteWorking: true},
-	MimeType: {getIconUrl: mime => mime},
-	webroot: ''
-}
-
-describe('Image View src attribute based on markdown', () => {
-
-	const factory = attrs => {
-		const propsData = {
-			extension: { options: {currentDirectory: '/current'} },
-			node: {attrs}
-		}
-		const data = () => ({
-			imageLoaded: true,
-			loaded: true,
-			failed: false,
-		})
-		return shallowMount(ImageView, {propsData, data})
-	}
-
-	test('old style is used as is', () => {
-		const src = '/core/preview?fileId=123#mimetype=image%2Fjpeg'
-		const wrapper = factory({src})
-		expect(wrapper.find('.image__main').attributes('src')).toBe(src)
-	})
-
-	test('old style with index.php is used as is', () => {
-		const src = '/index.php/core/preview?fileId=9&x=1024&y=1024&a=true#mimetype=image%2Fjpeg&hasPreview=true&fileId=9'
-		const wrapper = factory({src})
-		expect(wrapper.find('.image__main').attributes('src')).toBe(src)
-	})
-
-	test('fileId is used for preview url', () => {
-		const src = '/Media/photo.jpeg?fileId=7#mimetype=image%2Fjpeg&hasPreview=true'
-		const wrapper = factory({src})
-		expect(wrapper.vm.fileId).toBe('7')
-		expect(wrapper.find('.image__main').attributes('src'))
-			.toContain('/core/preview?fileId=7')
-	})
-
-	test('use dav paths for gifs so they are animated', () => {
-		const src = '/Media/giffy.gif?fileId=7#mimetype=image%2Fgif&hasPreview=true'
-		const wrapper = factory({src})
-		expect(wrapper.vm.extension.options.currentDirectory).toBe('/current')
-		expect(wrapper.find('.image__main').attributes('src'))
-			.toContain("remote.php/dav/files/user1/current/Media/giffy.gif")
-	})
-
-	test('without fileId relative path is used in file based preview url', () => {
-		const wrapper = factory({src: 'sub/asdf.jpg?hasPreview=true'})
-		expect(wrapper.vm.isSupportedImage).toBe(true)
-		expect(wrapper.find('.image__main').attributes('src'))
-			.toBe('/core/preview?file=%2Fcurrent%2Fsub%2Fasdf.jpg&x=1024&y=1024&a=true')
-	})
-
-	test('public share link previews are just used as they are', () => {
-		const wrapper = factory({src: 'https://nextcloud/index.php/apps/files_sharing/publicpreview/CSYoWifBzrsMWeA?file=/deck11-calendar.png&x=1760&y=990&a=true'})
-		expect(wrapper.vm.isSupportedImage).toBe(true)
-		expect(wrapper.find('.image__main').attributes('src'))
-			.toBe('https://nextcloud/index.php/apps/files_sharing/publicpreview/CSYoWifBzrsMWeA?file=/deck11-calendar.png&x=1760&y=990&a=true')
-	})
-
-	test('data urls are used as is', () => {
-		const src = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEUAAACnej3aAAAAAXRSTlMAQObYZgAAAApJREFUCNdjYAAAAAIAAeIhvDMAAAAASUVORK5CYII='
-		const wrapper = factory({src})
-		expect(wrapper.find('.image__main').attributes('src')).toBe(src)
-	})
-
-	test('image served by the Text app API', () => {
-		const wrapper = factory({src: 'text://image?imageFileName=1640709467-a%60a%60a.png'})
-		expect(wrapper.vm.isSupportedImage).toBe(true)
-		expect(wrapper.find('.image__main').attributes('src'))
-			.toContain('apps/text/image?documentId=')
-	})
-
-	test('image served by the Text app API', () => {
-		const wrapper = factory({src: 'text://image?imageFileName=1640709467-a%60a%60a.png'})
-		expect(wrapper.vm.isSupportedImage).toBe(true)
-		expect(wrapper.find('.image__main').attributes('src'))
-			.toContain('imageFileName=1640709467-a%60a%60a.png')
-	})
-})
diff --git a/tests/TextTest.php b/tests/unit/Service/ImageServiceTest.php
index bfa4251fb..ffbba391d 100644
--- a/tests/TextTest.php
+++ b/tests/unit/Service/ImageServiceTest.php
@@ -6,28 +6,29 @@ use OCA\Text\AppInfo\Application;
 use OCA\Text\Service\ImageService;
 use OCP\Files\Folder;
 
-class TextTest extends \PHPUnit\Framework\TestCase {
+class ImageServiceTest extends \PHPUnit\Framework\TestCase {
+	private static $attachmentNames = [
+		'aaa.png',
+		'aaa (2).png',
+		'aaa 2).png',
+		'aaa (2.png',
+		'aaa ((2.png',
+		'aaa 2)).png',
+		'a[a]a.png',
+		'a(a)a.png',
+		'a](a.png',
+		',;:!?.§-_a_',
+		'a`a`.png',
+	];
+
 	public function testDummy() {
 		$app = new Application();
 		$this->assertEquals('text', $app::APP_NAME);
 	}
 
-	public function testGetAttachmentNamesFromContent() {
-		$contentNames = [
-			'aaa.png',
-			'aaa (2).png',
-			'aaa 2).png',
-			'aaa (2.png',
-			'aaa ((2.png',
-			'aaa 2)).png',
-			'a[a]a.png',
-			'a(a)a.png',
-			'a](a.png',
-			',;:!?.§-_a_',
-			'a`a`.png',
-		];
+	public function testGetOldAttachmentNamesFromContent() {
 		$content = "some content\n";
-		foreach ($contentNames as $name) {
+		foreach (self::$attachmentNames as $name) {
 			// this is how it's generated in MenuBar.vue
 			$linkText = preg_replace('/[[\]]/', '', $name);
 			$encodedName = urlencode($name);
@@ -35,8 +36,25 @@ class TextTest extends \PHPUnit\Framework\TestCase {
 		}
 		$content .= 'some content';
 
-		$computedNames = ImageService::getAttachmentNamesFromContent($content);
-		foreach ($contentNames as $contentName) {
+		$computedNames = ImageService::getAttachmentNamesFromContent($content, 33);
+		foreach (self::$attachmentNames as $contentName) {
+			$this->assertContains($contentName, $computedNames);
+		}
+	}
+
+
+	public function testGetAttachmentNamesFromContent() {
+		$content = "some content\n";
+		foreach (self::$attachmentNames as $name) {
+			// this is how it's generated in MenuBar.vue
+			$linkText = preg_replace('/[[\]]/', '', $name);
+			$encodedName = urlencode($name);
+			$content .= '![' . $linkText . '](.attachments.33/' . $encodedName . ")\n";
+		}
+		$content .= 'some content';
+
+		$computedNames = ImageService::getAttachmentNamesFromContent($content, 33);
+		foreach (self::$attachmentNames as $contentName) {
 			$this->assertContains($contentName, $computedNames);
 		}
 	}