fix: open non-image attachment

[luka-nextcloud]

📝 Summary

• Resolves: Opening non-image attachments is broken #3849

🖼️ Screenshots

🚧 TODO

• ...

🏁 Checklist

• Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
• Sign-off message is added to all commits
• Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
• Documentation (README or documentation) has been updated or is not required

[cypress]

5 failed and 2 flaky tests on run #9272 ↗︎

5

137

1

0

2

Details:

fix: open non-image attachment
Project: Text	Commit: 7e1e84adb9
Status: Failed	Duration: 05:59 💡
Started: Mar 29, 2023 10:17 AM	Ended: Mar 29, 2023 10:23 AM

  cypress/e2e/attachments.spec.js • 5 failed tests

View Output Video

Test		Artifacts
Test all attachment insertion methods > Insert an image file from Files		Output Screenshots
Test all attachment insertion methods > Upload a local image file (table.png)		Output Screenshots
Test all attachment insertion methods > Upload a local media file (file.txt.gz)		Output Screenshots
Test all attachment insertion methods > Upload image files with the same name		Output Screenshots
Test all attachment insertion methods > test if attachment files are in the attachment folder		Output Screenshots

  cypress/e2e/sync.spec.js • 2 flaky tests

View Output Video

Test		Artifacts
Sync > saves on close		Output Screenshots
Sync > recovers from a lost connection		Output Screenshots

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[mejo-]

Thanks for looking into this @luka-nextcloud. From the screenshot I wonder why you changed the layout of non-image attachments? I think we should preserve the way they were rendered before, no?

[mejo-]

@luka-nextcloud I would have envisioned a simple <a href="/f/<fileId>">[...]</a> around the non-image attachment node here:

text/src/nodes/ImageView.vue

Lines 36 to 48 in 25f2f5f

	<div v-if="isMediaAttachment"
	class="media"
	@click="handleImageClick(src)">
	<div class="media__wrapper">
	<img v-show="loaded"
	:src="imageUrl"
	class="image__main"
	@load="onLoaded">
	<div class="metadata">
	<span class="name">{{ alt }}</span>
	<span class="size">{{ attachmentMetadata.size }}</span>
	</div>
	</div>

Additionally, the registered @click handler in this block would need to be removed. That would allow Text/Collectives to treat the non-media attachment node as simple link with their registered linkHandlers. What do you think about that approach?

[luka-nextcloud]

Thanks for looking into this @luka-nextcloud. From the screenshot I wonder why you changed the layout of non-image attachments? I think we should preserve the way they were rendered before, no?

Actually, I didn't change the layout, this layout for non-image was already there at the beginning. It wasn't shown before because of some missing data (mime type of file was missing before). I just fixed it.

[luka-nextcloud]

@luka-nextcloud I would have envisioned a simple <a href="/f/<fileId>">[...]</a> around the non-image attachment node here:

text/src/nodes/ImageView.vue

Lines 36 to 48 in 25f2f5f

	<div v-if="isMediaAttachment"
	class="media"
	@click="handleImageClick(src)">
	<div class="media__wrapper">
	<img v-show="loaded"
	:src="imageUrl"
	class="image__main"
	@load="onLoaded">
	<div class="metadata">
	<span class="name">{{ alt }}</span>
	<span class="size">{{ attachmentMetadata.size }}</span>
	</div>
	</div>

Additionally, the registered @click handler in this block would need to be removed. That would allow Text/Collectives to treat the non-media attachment node as simple link with their registered linkHandlers. What do you think about that approach?

I think it's already done here. Please check: https://github.com/nextcloud/text/blob/main/src/nodes/ImageView.vue#L108

[mejo-]

@luka-nextcloud I think there's a missunderstanding here.

To my understanding, the following block is used for attachments that have a set, but unsupported mimetype:

text/src/nodes/ImageView.vue

Lines 105 to 123 in dd0f826

	<div v-else class="image-view__cant_display">
	<transition name="fade">
	<div v-show="loaded">
	<a :href="internalLinkOrImage" target="_blank">
	<span v-if="!isSupportedImage">{{ alt }}</span>
	</a>
	</div>
	</transition>
	<transition v-if="isSupportedImage" name="fade">
	<div v-show="loaded" class="image__caption">
	<input ref="altInput"
	type="text"
	:value="alt"
	:disabled="!editable"
	@blur="updateAlt"
	@keyup.enter="updateAlt">
	</div>
	</transition>
	</div>

I would like to continue using the following block for media attachments (non-image attachments) instead (where it's rendered properly as attachment node), just making this node a link to the files URL of the attachment:

text/src/nodes/ImageView.vue

Lines 36 to 59 in dd0f826

	<div v-if="isMediaAttachment"
	class="media"
	@click="handleImageClick(src)">
	<div class="media__wrapper">
	<img v-show="loaded"
	:src="imageUrl"
	class="image__main"
	@load="onLoaded">
	<div class="metadata">
	<span class="name">{{ alt }}</span>
	<span class="size">{{ attachmentMetadata.size }}</span>
	</div>
	</div>
	<div v-if="showDeleteIcon"
	class="buttons">
	<NcButton :aria-label="t('text', 'Delete this attachment')"
	:title="t('text', 'Delete this attachment')"
	@click="deleteNode">
	<template #icon>
	<DeleteIcon />
	</template>
	</NcButton>
	</div>
	</div>

[luka-nextcloud]

@mejo- Could you please check again?

[mejo-]

Thanks, looks better already but still I think most of your changes are not necessary 😉

Also, could you squash all commits in the PR into one?

[mejo-]

Thanks @luka-nextcloud. I think that the PR has to wait until #3974 got resolved.

As said, the option to inject fileId as query param to the markdown in insertAttachment() of MediaHandler.vue brings several problems and should be avoided in my eyes:

• It doesn't work for attachments that were uploaded prior to this PR.
• It will break whenever the text document gets copied - fileIds of the attachments will change in that case.
• It will not work for attachments that got inserted manually (arguably neglectable though)

[luka-nextcloud]

@mejo-

It doesn't work for attachments that were uploaded prior to this

For attachments already saved before, there is no way to detect it's fileId. The saved content before will look like this.

It will break whenever the text document gets copied - fileIds of the attachments will change in that case.

Not sure what it means...

It will not work for attachments that got inserted manually (arguably neglectable though)

Do you mean using "Upload from computer"?

[mejo-]

Hey @luka-nextcloud,

For attachments already saved before, there is no way to detect it's fileId. The saved content before will look like this.

Exactly, and that's why I suggested to update the logic of AttachmentResolver as described in #3974 😉

It will break whenever the text document gets copied - fileIds of the attachments will change in that case.

Not sure what it means...

I mean the following: create a Text document and add an attachment. Then you will have the attachment fileId hardcoded in its markdown. Afterwards, copy the document in the Nextcloud Files app ("Move or copy" in three-dot-menu, then "Copy"). The resulting copy will have the attachment copied over as well, but the fileId in markdown will be wrong.

[ralfrupf1976]

I just uploaded a PDF in Kollektive - still PDFs are not shown correctly. Any Updates on this?

[mejo-]

I just uploaded a PDF in Kollektive - still PDFs are not shown correctly. Any Updates on this?

@ralfrupf1976 we're working on a proper fix for this. It can take a bit though because it requires bigger internal code changes.

[juliushaertl]

@mejo- I guess this will be covered by #5042 then so let's close this one for now I'd say.