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

Add captions to m.file, m.image, m.video, and m.audio #2059

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

nexy7574
Copy link

@nexy7574 nexy7574 commented Nov 17, 2024

Description

This PR will introduce both rendering & sending file captions.
These were introduced in spec v1.10, and have recently seen wide adoption amongst Element and other clients. It is time that cinny had this too.

Fixes #1646

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

Copy link

github-actions bot commented Nov 17, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@nexy7574
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

ajbura added a commit to cinnyapp/cla that referenced this pull request Nov 17, 2024
Copy link

github-actions bot commented Nov 17, 2024

Preview: https://2059--pr-cinny.netlify.app
⚠️ Exercise caution. Use test accounts. ⚠️

@nexy7574 nexy7574 changed the title Add captions to m.image Add captions to m.file, m.image, m.video, and m.audio Nov 18, 2024
src/app/components/RenderMessageContent.tsx Outdated Show resolved Hide resolved
src/app/components/RenderMessageContent.tsx Outdated Show resolved Hide resolved
Comment on lines 227 to 230
const contentsPromises = uploads.map(async (upload) => {
const fileItem = selectedFiles.find((f) => f.file === upload.file);
if (!fileItem) throw new Error('Broken upload');
const caption = toPlainText(editor.children).trim() || undefined;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like following changes add same caption to multiple uploaded file. Do we really want that? Can't we just send it as separate in that case?

Copy link
Author

Choose a reason for hiding this comment

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

Would it not be better to attach the caption to the last uploaded item, rather than as an entirely separate message?

Copy link
Collaborator

Choose a reason for hiding this comment

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

image I think captions should be added here for each file and message input should not be used for caption as doing so will break the current functionality. For example someone wants to send an image right after writing their TWIM message.

Copy link
Author

@nexy7574 nexy7574 Dec 10, 2024

Choose a reason for hiding this comment

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

As nice as that would be, would it not require adding another message composer, especially since both html and plaintext are meant to be supported? How would that work on mobile?
Apologies for the delay, my hands are quite full until Febuary 😔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah adding full message composer won't be a good idea, but maybe we can add plaintext caption as file rename option, not sure about how that would go. So if we don't want to go with sending caption from main composer yet could we please limit this PR for display only support as that alone would be really helpful.

src/app/features/room/RoomInput.tsx Outdated Show resolved Hide resolved
Comment on lines -321 to +324
mx.sendMessage(roomId, content);
if (!uploadBoardHandlers.current) {
mx.sendMessage(roomId, content);
}
Copy link
Member

Choose a reason for hiding this comment

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

It will be a good idea if we can extract the content generation process in separate function so it can be used here and for caption.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: MSC2530 support for Media Captions
3 participants