isamert / scli

a simple terminal user interface for signal messenger (using signal-cli)
GNU General Public License v3.0
449 stars 39 forks source link

Show sender avatar in notifications #168

Closed maximbaz closed 2 years ago

maximbaz commented 2 years ago

Fixes #159.

Things to note:

... Why?

This allows for maximum flexibility. If we were to default to say mail-message-new, two things might happen:

Both cases are not really possible to configure via --notification-command, as this is just a fallback icon. We'd have to make an extra param or two to control this.

With the proposed default to scli however, everything above is achievable.

maximbaz commented 2 years ago

This generally works but has the same issue as #162, you fixed the name but now image is sometimes of a wrong sender, I need to study how you solved it and apply the same fix.

UPDATE: OK this was simpler than anticipated 😁

exquo commented 2 years ago

Nice, thanks!

I've added handling for the groups' avatars.

WRT the stock icons, I meant to use them as an additional step, if the scli.* icon has not been found. But I agree we'd probably be better doing without.

Let me know if you have feedback!

exquo commented 2 years ago

P.S. while testing this I've learned that after adding a new icon in /usr/share/icons, the icon cache needs to be rebuilt with

sudo update-icon-caches /usr/share/icons/*

before notify-send -i <new_icon> .. can be used.

https://askubuntu.com/questions/722708/how-do-i-refresh-the-icon-cache

maximbaz commented 2 years ago

I was just about to ask you today how we could distinguish notifications from a group, your idea with showing a group avatar is a very elegant solution, I really like this! I have no further comments, seems to work well for me! Thanks for the cleanup 😉

maximbaz commented 2 years ago

Hmm the problem with the wrong avatar on reaction messages has reappeared 🤔

UPDATE: saw only once (when I resumed my laptop after sleep) and still haven't found a reliable way to reproduce, so maybe I misremember things? Code looks reasonable to me...

exquo commented 2 years ago

I've tried the current version of the PR - seems to work fine with the reactions, with either contact, profile or group. But let me know if you spot any irregularities.

maximbaz commented 2 years ago

I think I found what confused me - it's when I send a reaction from phone to Alice, it will show a notification with my name and Alice's photo. I suppose if we could just make this show my photo, it would remove the confusion?

exquo commented 2 years ago

Oh, good catch! We shouldn't be sending any desktop notifications on receiving reactions in 'sync' messages (i.e. from linked devices). Same as with 'regular' (non-reaction) sync messages. I've pushed a commit to fix this.

maximbaz commented 2 years ago

I have no further comments, works well for me, thanks 👍