deltachat / deltachat-core-rust

Delta Chat Rust Core library, used by Android/iOS/desktop apps, bindings and bots 📧
https://delta.chat/en/contribute
Other
659 stars 85 forks source link

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

Closed adbenitez closed 2 months ago

adbenitez commented 2 months ago

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

Simon-Laux commented 2 months ago

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

r10s commented 2 months ago

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 commented 2 months ago

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

iequidoo commented 2 months ago

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,: https://github.com/deltachat/deltachat-core-rust/blob/d6d90db957ee171365ca5dbd1f14b4440d843985/src/summary.rs#L64-L84

Simon-Laux commented 2 months ago

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 commented 2 months ago

i retried that:

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

meganoahj commented 2 months ago

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: https://github.com/deltachat/deltachat-desktop/pull/4064

Simon-Laux commented 2 months ago

fixed by :