Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-image media attachments #2378

Merged
merged 25 commits into from Sep 6, 2022
Merged

Conversation

julien-nc
Copy link
Member

refs #2339

Allow non-image media upload. Some details needs to be discussed before moving forward.

@julien-nc julien-nc added this to the Nextcloud 25 milestone May 9, 2022
@julien-nc julien-nc force-pushed the enh/2339/non-image-media-attachments branch from 556f914 to 7b13184 Compare May 9, 2022 22:30
@julien-nc julien-nc force-pushed the enh/2339/non-image-media-attachments branch 2 times, most recently from 0778311 to bd0bf69 Compare May 31, 2022 15:41
@julien-nc
Copy link
Member Author

julien-nc commented May 31, 2022

@mejo- @max-nextcloud @vinicius73 @juliushaertl

This is now based on latest master which includes the material icon PR 😁.
⚠️ compiled code is not included.

Before moving forward, do we agree on the following aspects?

  1. We could just rename image related entries in the menubar s/image/media/ as we want to let users insert any kind of file via: the menubar, the paste event, the drop event
  2. Images and Medias (any other mimetype) can be handled by the same controller/service on the server side?
    • We could rename ImageController => MediaController
    • We could rename ImageService => MediaService
  3. We have to choose how to write media files in the markdown content (and then make the magic happen in Tiptap's parsing mechanism)
  4. We will handle the media display in a dedicated node (pretty much like ImageView). It could look like the file list items in Files.

What's been implemented for now:

  • It's possible to upload any kind of file
    • If it's an image, it's done like before
    • If not, then it's almost inserted like an image except the link target ends with #media
  • ImageView calls different endpoints when the link target ends with #media
  • We now have 3 endpoints to serve image/media stuff:
    • one to get an image (it existed before), it provides a preview if possible, the real image content if not
    • one to get a media file, it provides the file content
    • one to get a media file preview, it provides a file preview if possible, the mimetype icon URL if not

What's weird but not definitive 😜 :

  • There's no styling when ImageView display a media file as it will be done somewhere else later
  • The attachment cleanup does not work anymore because the trailing #media breaks it. It will be adjusted when we decide how to write media files in the markdown content

TODO

  • Implement server side logic
  • Implement the parsing magic for media attachments
  • Implement a MediaFileView node to display media attachments
  • Adjust the attachment cleanup methods
  • Implement some PhpUnit tests
  • Implement some Cypress tests

Let's go!

@vinicius73
Copy link
Member

I like this approach, looks future-proof.
What do you think about change MediaHandler/ImageHandler to AttachmentHandler @eneiluj?

@julien-nc julien-nc force-pushed the enh/2339/non-image-media-attachments branch from bd0bf69 to 60dea34 Compare June 8, 2022 17:33
@julien-nc julien-nc force-pushed the enh/2339/non-image-media-attachments branch 3 times, most recently from 7f9e746 to 01e519c Compare July 14, 2022 12:46
@julien-nc julien-nc marked this pull request as ready for review July 14, 2022 12:49
@julien-nc julien-nc changed the title [WIP] Non-image media attachments Non-image media attachments Jul 14, 2022
@julien-nc
Copy link
Member Author

julien-nc commented Jul 14, 2022

Some news

  • Media attachments are inserted just like images in the Markdown content
  • There are 3 new API endpoints to load a media attachment file content, a preview and metadata
  • The image loading fallback mechanism can now handle any number of fallback targets
  • We attempt to load a media file if loading as an image has failed
  • Media attachments are rendered by <ImageView>. It is still possible to delete with the trash icon. It looks like that:
    image

Some problems

  • For now we can't point to a media file with a relative path using the dav API. It just works for files in the attachment folder.
  • The current implementation produces multiple requests for each media attachment. In the worst case:
    1. one checking the dav API (failing)
    2. one checking the image attachment API (failing)
    3. one checking the media attachment API (yay success)
    4. one last to get the metadata (yay again)

TODO

  • What do we do when clicking on a media attachment?
    • Download?
    • Show in Files?
    • Open in the Viewer if possible?
    • All that and more?
  • Rename ImageService, ImageResolver, MediaHandler etc...
  • Think about having a single attachment endpoint returning the mimetype, a preview and some metadata

src/nodes/ImageView.vue Outdated Show resolved Hide resolved
return new Promise((resolve, reject) => {
const img = new Image()
img.onload = () => {
this.imageUrl = imageUrl
this.imageLoaded = true
this.loaded = true
this.attachmentType = attachmentType
console.debug('SUCCESS type', attachmentType)
if (attachmentType === 'media') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it will be good to change it to a watcher.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to commits on top of mines 😁. It is not as simple as it seems because we need the file name to load the metadata.

src/services/ImageResolver.js Outdated Show resolved Hide resolved
appinfo/routes.php Outdated Show resolved Hide resolved
@juliushaertl juliushaertl mentioned this pull request Jul 28, 2022
20 tasks
@jancborchardt
Copy link
Member

@eneiluj the delete icon looks a bit off, it seems to be a community one from materialdesignicons.com (please try not to use that, only when the official site has no icon) – use this instead :) https://fonts.google.com/icons?selected=Material%20Icons%3Adelete%3A

Regarding the layout of the element itself, it would be a bit nicer if the element doesn’t go full width. The size info could go into the subline of the element, using color: var(--color-text-maxcontrast);, and the delete icon on the right of it immediately. Since the delete icon is only shown on hover/focus it is fine to have it at different positions.

Example mockup like it is in Mail:
image

@julien-nc
Copy link
Member Author

/compile

@julien-nc julien-nc force-pushed the enh/2339/non-image-media-attachments branch 2 times, most recently from 65618d7 to 7407a9d Compare August 23, 2022 11:41
Julien Veyssier added 19 commits September 6, 2022 14:44
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
…lving loadImage

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
…r when exceeding post_max_size, log upload auth error

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@julien-nc julien-nc force-pushed the enh/2339/non-image-media-attachments branch from 4cf9769 to 4b80cd5 Compare September 6, 2022 12:46
@julien-nc
Copy link
Member Author

/compile

@max-nextcloud
Copy link
Collaborator

I wonder if the cypress failures are due to this... maybe because the urls changed now that the parameters are in there...

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@julien-nc julien-nc force-pushed the enh/2339/non-image-media-attachments branch from 9eb7a5a to 277b8e0 Compare September 6, 2022 14:34
@julien-nc
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@julien-nc julien-nc merged commit 0849b55 into master Sep 6, 2022
@delete-merged-branch delete-merged-branch bot deleted the enh/2339/non-image-media-attachments branch September 6, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants