Improve image attachment management

[julien-nc]

refs #2411

Implementation of the behaviour suggested by @max-nextcloud in #2411 (comment) with which I completely agree ❤️ :

• 1. If the image url matches .attachments.MD_FILE_ID/FILENAME use FILENAME to get the image with the attachment API.
• 2. If the image url matches .attachments.OTHER_FILE_ID/FILENAME
  • a. try to retrieve the image with the WebDav API.
  • b. If that fails try to retrieve FILENAME with the attachment API.
  • c. Maybe rewrite the url with the own file id if b. succeeds.
• 3. If not, use the path with the WebDav API.
• 4. If that fails check if the url has a fileID param and try to get the file from that. (So we keep the old behavior for old images)

So this PR brings back the relative path in the markdown content's image target. We now write:

![IMAGE_FILE_NAME](.attachments.MARKDOWN_FILE_ID/ENCODED_IMAGE_FILE_NAME)

The problem with 2.a. and 2.b. is that imageUrl is a computed prop in which we would need a non-async (sync?) image loading check. But we can only make async network calls in a computed prop 😁.
There already is a check which depends on imageUrl.
In short, we would need to make deeper changes to ImageView to allow 2.a. and 2.b.

We could discuss that and in the meantime I might come up with a first solution.

[julien-nc]

@max-nextcloud I found a reasonable solution for 2.a. and 2.b. : move the content of the imageUrl computed prop in a method (called init) which is called in beforeMount (which is the only time and place we loaded the image anyway). This way we can happily call async stuff.

imageUrl is now a data attribute. It is set if the image has been successfully loaded.

The image loading has been moved in a method as well (loadImage). It has an optional error callback that we use for 2.

The init method now handles all the different cases and is more flexible than the old imageUrl computed prop.

[julien-nc]

/compile

[julien-nc]

/compile amend

[julien-nc]

@vinicius73 Thanks for the review. If you have any idea about how to fix the Jest tests. It seems the checks are done before the async stuff is finished.

[julien-nc]

Let's commit the compiled code in the end to make our lives easier.

So don't forget to compile locally 😉.

[vinicius73]

You can simplify this code.

Suggested change

	this.loadImage(this.getTextApiUrl(imageFileName)).catch((e) => {
	this.onImageLoadFailure()
	})
	this.loadImage(this.getTextApiUrl(imageFileName))
	.catch(this.onImageLoadFailure)

[vinicius73]

Also, we can use async/await and wrap this.init() and handle error from this.init()

Suggested change

	this.loadImage(this.getTextApiUrl(imageFileName)).catch((e) => {
	this.onImageLoadFailure()
	})
	await this.loadImage(this.getTextApiUrl(imageFileName))

or

Suggested change

	this.loadImage(this.getTextApiUrl(imageFileName)).catch((e) => {
	this.onImageLoadFailure()
	})
	return this.loadImage(this.getTextApiUrl(imageFileName))

and, change this.init() call.

this.init().catch(this.onImageLoadFailure)

[max-nextcloud]

Thanks a lot for picking up the idea. 😍
I only had a quick look at the code and checked if the text:// urls are still parsed and will still display nicely. Looks good to me in that regard.

[julien-nc]

/compile

[julien-nc]

/compile

[julien-nc]

/compile

[julien-nc]

@vinicius73 @max-nextcloud Ready to merge!

[juliusknorr]

Something for a separate PR but we should move those tests into tests/unit/Service/ImageServiceTest.php

[juliusknorr]

Minor comments, but feel free to address them separately :)

[julien-nc]

@vinicius73 Thanks
@juliushaertl Better now than forgetting it, thanks.

[julien-nc]

/compile

[julien-nc]

Should we backport this to stable24? I think we should.

[julien-nc]

/backport to stable24

[backportbot-nextcloud]

The backport to stable24 failed. Please do this backport manually.

[mejo-]

Thanks so much for implementing this @eneiluj ❤️

I think that we can close #2411 then, no?