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

[Desktop] Icon preview of latest WebXDC displayed when summary is reaction event #5782

Closed
adbenitez opened this issue Jul 17, 2024 · 11 comments
Assignees
Labels
bug Something is not working

Comments

@adbenitez
Copy link
Member

image

this might be hard to fix without help from core, since these "foo reacted with X" summaries are kind of a minimal hack core-side

@adbenitez adbenitez added the bug Something is not working label Jul 17, 2024
@adbenitez adbenitez changed the title thumbnail preview displayed when summary is reaction event thumbnail preview of latest message displayed when summary is reaction event Jul 17, 2024
@Simon-Laux
Copy link
Member

needs to be fixed in core or in jsonrpc, this is summaryPreviewImage from ChatlistItem

@Simon-Laux Simon-Laux transferred this issue from deltachat/deltachat-desktop Jul 17, 2024
@r10s
Copy link
Member

r10s commented Jul 17, 2024

same for webxdc apps probably.

in case things get more complex than expected, we can also consider to drop the image completely. it is of limited use in that size with distorted aspect, often saying not much more than that there is an image. and we meanwhile have an emoji to indicate better than the old text that an image is present.

@Simon-Laux
Copy link
Member

I like the preview image, also I don't see why it should be complex

@iequidoo
Copy link
Collaborator

Can't reproduce this on my desktop, i see no image preview in case of a reaction. Also code-wise it looks correct, see thumbnail_path: None,:

pub async fn new_with_reaction_details(
context: &Context,
msg: &Message,
chat: &Chat,
contact: Option<&Contact>,
) -> Result<Summary> {
if let Some((reaction_msg, reaction_contact_id, reaction)) = chat
.get_last_reaction_if_newer_than(context, msg.timestamp_sort)
.await?
{
// there is a reaction newer than the latest message, show that.
// sorting and therefore date is still the one of the last message,
// the reaction is is more sth. that overlays temporarily.
let summary = reaction_msg.get_summary_text_without_prefix(context).await;
return Ok(Summary {
prefix: None,
text: msg_reacted(context, reaction_contact_id, &reaction, &summary).await,
timestamp: msg.get_timestamp(), // message timestamp (not reaction) to make timestamps more consistent with chats ordering
state: msg.state, // message state (not reaction) - indicating if it was me sending the last message
thumbnail_path: None,
});

@Simon-Laux
Copy link
Member

Simon-Laux commented Jul 19, 2024

I think it's about when the last message in the chat is an image or webxdc and there is a newer reaction to any other message in the chat that came in after the image. Strange that the code doesn't seem to have that possibility. what desktop/core version did you use @adbenitez ?

@r10s
Copy link
Member

r10s commented Jul 19, 2024

i retried that:

  • images are fine, as @iequidoo pointed out. indeed, that is solved "proper" (and is no "hack" :)

  • the issue seems to be about webxdc-icons, which seems to be added later by jsonrpc or desktop somehow - and does not always fit to the summary therefore

i think, desktop must use only thumbnail_path for the summary. core, additionally, can then set the path also for webxdc-icons as needed

@Simon-Laux
Copy link
Member

Simon-Laux commented Jul 20, 2024

the code in desktop: https://github.com/deltachat/deltachat-desktop/blob/83e44138e6d160f7cb5ae45c81f34bd85d0ba4d7/src/renderer/components/chat/ChatListItem.tsx#L104

webxdc icons are not set in code, because the path is inside of the zip files which is not accessible.
Maybe thumbnail path from jsonrpc/core should be set to sth like "webxdc-icon:last-msg-id" and then desktop should check for that string instead of checking for lastMessageType === 'Webxdc'.
we could also make core directly return the paths that desktop needs, but that custom scheme (webxdc-icon:${accountId}.${msgId}) is useless and maybe confusing for other platforms.

@iequidoo
Copy link
Collaborator

Maybe thumbnail path from jsonrpc/core should be set to sth like "webxdc-icon:last-msg-id" and then desktop should check for that string instead of checking for lastMessageType === 'Webxdc'.

Let's implement this approach. It looks as a minimal bugfix.

@iequidoo iequidoo changed the title thumbnail preview of latest message displayed when summary is reaction event [Desktop] Icon preview of latest WebXDC displayed when summary is reaction event Jul 21, 2024
@iequidoo iequidoo self-assigned this Jul 21, 2024
iequidoo added a commit that referenced this issue Jul 21, 2024
…id" (#5782)

This is a hint for apps that a WebXDC icon should be shown in the summary, e.g. in the
chatlist. Otherwise it's not clear when it should be shown, e.g. it shouldn't be shown in a reaction
summary.
iequidoo added a commit that referenced this issue Jul 21, 2024
…sg-id" (#5782)

This is a hint for apps that a WebXDC icon should be shown in the summary, e.g. in the
chatlist. Otherwise it's not clear when it should be shown, e.g. it shouldn't be shown in a reaction
summary.
iequidoo added a commit that referenced this issue Jul 22, 2024
…sg-id" (#5782)

This is a hint for apps that a WebXDC icon should be shown in the summary, e.g. in the
chatlist. Otherwise it's not clear when it should be shown, e.g. it shouldn't be shown in a reaction
summary.
@iequidoo
Copy link
Collaborator

#5789 is merged, this should be closed after the corresponding DC Desktop fix.

@meganoahj
Copy link
Contributor

same for webxdc apps probably.

in case things get more complex than expected, we can also consider to drop the image completely. it is of limited use in that size with distorted aspect, often saying not much more than that there is an image. and we meanwhile have an emoji to indicate better than the old text that an image is present.

Hey, this pr fixes the distortion:
deltachat/deltachat-desktop#4064

Simon-Laux pushed a commit to deltachat/deltachat-desktop that referenced this issue Aug 3, 2024
…n event (#4062)

* Fixes Icon preview of latest WebXDC displayed when summary is reaction event

deltachat/deltachat-core-rust#5782

* added changelog message

* fixed formatting

* fix lint

* fixed code

* put changelog entry to the right place
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
None yet
Development

No branches or pull requests

5 participants