search

nextcloud / talk-android

📱😀 Video & audio calls through Nextcloud on Android
Other
530 stars 238 forks source link

followup for #1935: fix notification subject for reaction #1945

Open mahibi opened 2 years ago

mahibi commented 2 years ago

align text for #1935 to align with ios

https://github.com/nextcloud/talk-android/pull/1936#issuecomment-1106848465

mahibi commented 2 years ago

regarding https://github.com/nextcloud/talk-android/pull/1936#issuecomment-1106848465

Btw the expected fixed version from my POV would be:

Subject as delivered Message that was reacted on

So in your sample

marcel hat mit smiling_face_with_three_hearts auf ihre private Nachricht reagiert Hello

@nickvergessen @AndyScherzinger

As far as i see this is against android's concept of notifications for chat apps. We need to set a person object which name is used to set the contentTitle.

androids documentation: https://developer.android.com/reference/androidx/core/app/NotificationCompat.MessagingStyle where we set it right now: https://github.com/nextcloud/talk-android/blob/30d57fda4f616a4b77b794a977deb651887fd585/app/src/main/java/com/nextcloud/talk/jobs/NotificationWorker.java#L449

So i guess that's why signal and telegram mix the reaction and the referred message in one text: grafik

So this could be an option for us too, but still would be different to iOS then.. (+ it would require own translation...) Should we do this for 14.0.0 or keep it like it is for now?

PS: From my POV the display of notifications (NotificationsWorker.java) should get more attention after 14.0.0, right now it's a bit complicated..

mahibi commented 2 years ago

comment from @AndyScherzinger :

I'd prefer consistency as a start so the questions would be

maybe @jancborchardt also has a take on this?

mahibi commented 2 years ago

@Ivansss i guess for iOS the concept is different so it's more flexible to to set the contents? or is it similar to android?

Ivansss commented 2 years ago

I guess it's similar to Android.

At the moment reactions notifications are displayed on iOS like:

Subject as delivered Message that was reacted on

We can think about another format for a next version but I would stick to the format above for 14.0 :)

jancborchardt commented 2 years ago

I'm fine with whatever is feasible for this version. For a future version either how Signal does it, or how Telegram does it (bit shorter, nice) would be good. :)

nickvergessen commented 2 years ago

How long should the text be before it's trimmed in case someone hearts my 4k character messages and not only "zzzZ" Very short? 32/64? Or medium like 128? Or untrimmed?

starypatyk commented 2 years ago

@mahibi

PS: From my POV the display of notifications (NotificationsWorker.java) should get more attention after 14.0.0, right now it's a bit complicated..

Fully agree! :wink:

I plan some refactoring in #1923 (after 14.0.0 is released).

AndyScherzinger commented 2 years ago

@starypatyk I think you are good to go now, last RC4 for 14.0.0 has been tagged today which should then be the stable release next week, at least all changes are in, so there shouldn't be any changes anymore except critical fixes but that shouldn't impact the notification code (too much).

timkrueger commented 2 years ago

Very short? 32/64? Or medium like 128? Or untrimmed? @nickvergessen

I would say untrimmed. Then it can be handled in the client. But then it must be separated so that the client can trim it independently from the reaction part. Or really short with 32 characters.

# Longer
Reacted 😎 to: 'Lorem ipsum dolor sit amet, consectetur ...'

# Shorter
😎 to 'Lorem ipsum dolor sit amet, consectetur ...'
Compact Expanded
Longer
Shorter

I faked the screenshots. Would this the way to go?

cc @jancborchardt @Ivansss

I edited the screenshots after a short offside discussion with @nickvergessen. Now the 'Longer' example in the table above is Signal style.

timkrueger commented 2 years ago

Here some examples from Signal:

Short text:

Long text:

jancborchardt commented 2 years ago

Very cool @timkrueger! :) I’d say including the "Reacted" is good as it makes abundantly clear what it is, and usually just seeing the very start of the message (in compacted) is enough.

timkrueger commented 2 years ago

So then this issue can be transferred to the spreed repository? I think that changes must only be done on the server.

@nickvergessen @Ivansss

nickvergessen commented 2 years ago

Aren't you replacing the message already in the client? Because what you display as "message" is the subject of the notification. So you can also just translate it there and add the parameters from the subject and the parsed message into the new string.

We can't really do it on the notification directly as if the subject contains the message we have to heavily cut the content as we can only encrypt around 150 characters.

timkrueger commented 2 years ago

Aren't you replacing the message already in the client? Because what you display as "message" is the subject of the notification. So you can also just translate it there and add the parameters from the subject and the parsed message into the new string.

No. I faked the screenshots.

We use the subject to set the conversation title. In the case of a 1-to-1 conversation it's an empty string: https://github.com/nextcloud/talk-android/blob/387bc3ca4f4c4e33b53ecd1d54283e510a872eb9/app/src/main/java/com/nextcloud/talk/jobs/NotificationWorker.java#L517

The text which is actually shown is directly set from the encrypted message: https://github.com/nextcloud/talk-android/blob/387bc3ca4f4c4e33b53ecd1d54283e510a872eb9/app/src/main/java/com/nextcloud/talk/jobs/NotificationWorker.java#L527

Because of that I have German text on an English client:

Tim Zwei hat mit ☺️ auf deine private Nachricht reagiert

We can't really do it on the notification directly as if the subject contains the message we have to heavily cut the content as we can only encrypt around 150 characters.

You're right. The complete problem can not be fixed on the server. But we need somehow only the reaction emoji without the text around.

Or I'm completely wrong?

nickvergessen commented 2 years ago

But we need somehow only the reaction emoji without the text around.

The decrypted push only contains the parsed subject. In case you still get the full notification from the server, you should get the emoji there as a type=highlight parameter with the placeholder reaction https://github.com/nextcloud/spreed/blob/486b89806f4185b01e61009ee486df90cbd5fa22/lib/Notification/Notifier.php#L456-L460