feat: show reactions in summaries #5387
Conversation
r10s
force-pushed
the
r10s/reactions-in-summaries
branch
from
ab259d4 to
c018338
Compare
|
I think that a reaction can be used as a verb: |
r10s
force-pushed
the
r10s/reactions-in-summaries
branch
from
75fc14b to
20a7fbf
Compare
Also with "reacted" it may be difficult to translate. Verbs in software UI are generally difficult to translate, sometimes there is no good translation. @r10s what do you think about dropping "reacted" from strings? |
this is an interesting idea, we should add that definitely to the translators hints, to give translators options to keep the string short EDIT: and/or if "reacted" is difficult to translate. apart from, that, i'd give the strings as known from whatsapp a chance before thinking about rewording, adding sender to summary etc. in practise, this works pretty nicely and sounds natural. these strings are also pretty close to what we're doing wrt "ALICE changed group image" meanwhile. |
|
On Sun, Mar 31, 2024 at 06:32 -0700, bjoern wrote:
> I think that a reaction can be used as a verb [...]
apart from, that, i'd give the strings as known from whatsapp a chance before thinking about rewording adding sender to summary etc. in practise, this works pretty nicely and sounds natural. these strings are also pretty close to what we're doing wrt "ALICE changed group image" meanwhile.
moreover, many people use tons of 1:1 chats in their private lives.
With them there isn't ambiguity who reacted.
|
|
i made a PR to add translations to transifex and also added the translator's hints there: deltachat/deltachat-android#2985 |
src/summary.rs
Outdated
| return Ok(Summary { | ||
| prefix: None, | ||
| text: msg_reacted(context, reaction_contact_id, &reaction, &summary).await, | ||
| timestamp: msg.get_timestamp(), // message timestamp (not reaction) - otherwise sorting is wrong |
There was a problem hiding this comment.
Btw, sorting can be wrong anyway because we order chats in the chatlist by the sort timestamp, but display the "sent" timestamp. So, maybe we don't need to protect from this because it happens sometimes anyway, and just display the reaction time
There was a problem hiding this comment.
true. however, this "wrong message timestamp", messing up descending timestamp column, is the exception in case of errors.
if we would do that for magnitudes more frequent reactions, the timestamp column will appear unpredictable random instead of descending to the user. avoiding to get oriented there wrt newer/older up/down.
i think, this is also the main reason reason not to resort the list on reactions - it is just too much noise.
There was a problem hiding this comment.
Agree. Maybe only improve the comment -- "message timestamp (not reaction) - to make timestamps more consistent with chats ordering"
|
Is this only shown for reactions that are on the last message in the chat?
There needs to be at least some event to update the chatlist, or am I missing sth? |
|
the event emitted is DC_EVENT_REACTION_CHANGED, it was emitted before already, this is not part of this PR. for ios this just works, for android a one line change. #4476 might need adaption moreover, the UI need to set the two stock strings of deltachat/deltachat-android#2985. the summary is shown for any reaction of any message, if the reaction is newer than the last message. see initial post for more details |
r10s
force-pushed
the
r10s/reactions-in-summaries
branch
from
b342a98 to
26c57e6
Compare
|
/me force-pushed, resolved conflicts and tweaked the tests a bit. @iequidoo the new |
src/summary.rs
Outdated
| return Ok(Summary { | ||
| prefix: None, | ||
| text: msg_reacted(context, reaction_contact_id, &reaction, &summary).await, | ||
| timestamp: msg.get_timestamp(), // message timestamp (not reaction) - otherwise sorting is wrong |
There was a problem hiding this comment.
Agree. Maybe only improve the comment -- "message timestamp (not reaction) - to make timestamps more consistent with chats ordering"
src/reaction.rs
Outdated
| use crate::receive_imf::{receive_imf, receive_imf_from_inbox}; | ||
| use crate::test_utils::TestContext; | ||
| use crate::test_utils::TestContextManager; | ||
| use deltachat_time::SystemTimeTools; |
There was a problem hiding this comment.
You can even use crate::tools::SystemTime; and then write SystemTime::shift() like it's done in other tests
There was a problem hiding this comment.
thanks for the hint! indeed i was wondering why SystemTimeTools usage could not be found via grep.
i will adapt the tests in this PR to match the usage pattern of the other tests
this PR shows the last reaction in chatlist's summaries if there is no newer message.
the reason to show reactions in the summary, is to make them a little more visible when one is not in the chat. esp. in not-so-chatty or in one-to-ones chats this becomes handy: imaging a question and someone "answers" with "thumbs up" ... 🥚
otoh, reactions are still tuned down on purpose: no notifications, chats are opend as usual, the chatlist is not sorted by reactions and also the date in the summary refer to the last message - i thought quite a bit about that, this seems to be good compromise and will raise the fewest questions. it is somehow clear to the users that reactions are not the same as a real message. also, it is comparable easy to implement - no UI changes required :)
all that is very close to what whatsapp is doing (figured that out by quite some testing ... to cite @adbenitez: if in doubt, we can blame whatsapp :)
this is how it looks like: 🥚
technically, i first wanted to go for the "big solution" and add two more columns, chat_id and timestamp, however, it seemed a bit bloated if we really only need the last one. therefore, i just added the last reaction information to the chat's param, which seems more performant but also easier to code :)
btw: there is no index over the msg_id - no index at all in the reactions table? or did i miss sth?
🥚