Skip to content

Conversation

@Waradu
Copy link
Contributor

@Waradu Waradu commented Nov 16, 2024

Image support. Other attachments are filtered out for now.
image
Sending is also not implemented.

@alii
Copy link
Contributor

alii commented Nov 16, 2024

Thanks very much for the pr!

Rose already had some code which just came out last night (as it was just testing only for now) for an image gallery and seemed to have a clear idea of what that should look like. @roobscoob could you review/leave a note if you think we should merge this 🙂

@alii
Copy link
Contributor

alii commented Nov 16, 2024

Worth noting: currently rendering for the message is done with the get_content method (plan was that would include the entire message UI). Thinking about it now, it could make sense for image attachments to render separately.

But maybe it might make even more sense if instead of the backend deciding what message to render, we actually had an interim shape that was like RenderableMessage and the backend's job is just responsible for giving us these RenderableMessage shapes. That way all the rendering could still be done in the ui crate and be unrelated to the backend.

Still worth discussion with @roobscoob

@roobscoob
Copy link
Contributor

My take here is that images should be a part of "Content", as should any further message-body related data (including reactions, embeds, further.) #11 Supports message grouping via (effectively) a Vec<Content>, and I think that's the way to go. Don't want to have to deal with it being like, a Vec<(Content, Attachment, Reactions, Embeds, ...)>

Not to mention I think a "Content" is more abstract for non-discord platforms, while not losing any functionality on discord itself.

@alii a little curious about this:

Worth noting: currently rendering for the message is done with the get_content method (plan was that would include the entire message UI). Thinking about it now, it could make sense for image attachments to render separately.

Why do you think it would make more sense? It's not intuitive for me but I'm curious what you have in mind

@alii
Copy link
Contributor

alii commented Nov 16, 2024

@roobscoob Why do you think it would make more sense? It's not intuitive for me but I'm curious what you have in mind

Because then message UI would be consistent between backends, and instead of passing Elements we just pass an Scope-specific struct. Each backend just basically needs to implement a translation layer but the rendering is always done the same. Can discuss further in Scope discord

Copy link
Contributor

@roobscoob roobscoob left a comment

Choose a reason for hiding this comment

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

Discussed in discord, just formalizing here. Ideally chat::Message does not have a get_images(..) and instead Images become a part of DiscordMessageContent (maybe images: DiscordImageContent? to keep your struct?)

.p_2()
.child(img(message.get_author().get_icon()).object_fit(gpui::ObjectFit::Fill).bg(rgb(0xFFFFFF)).rounded_full().w_12().h_12())
.child(div().flex().flex_col().child(message.get_author().get_display_name()).child(message.get_content()))
.child(div().flex().flex_col().child(message.get_author().get_display_name()).child(message.get_content()).child(message.get_images()))
Copy link
Contributor

Choose a reason for hiding this comment

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

one other thing: this will conflict now that #11 is merged. Changing content should fix this though.

@Waradu
Copy link
Contributor Author

Waradu commented Nov 16, 2024

I'm closing this for now. I'll create a new PR when I have time to reimplement it (or someone else can do it).
To summarize things for later:
Everything message related (text, images, embeds...) should be part of DiscordMessageContent and included in get_content().

@Waradu Waradu closed this Nov 16, 2024
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.

4 participants