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

Don't mark room as unread due to reactions #5846

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

ofalvai
Copy link
Contributor

@ofalvai ofalvai commented Apr 26, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Fixes #2907

Motivation and context

I found it distracting and frustrating that my room list had a lot of unread indicators, showing unread reactions by other people. This is especially distracting in read-only announcement rooms where reactions are often added to old messages.

This change also aligns Element Android to Element Web in handling reactions.

Screenshots / GIFs

Before After
Screenshot_20220419-220448 Screenshot_20220419-220438

Tests

Manual testing with my own unread channels

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 11

Checklist

Signed-off-by: Olivér Falvai [email protected]

@@ -103,12 +100,6 @@ class DisplayableEventFormatter @Inject constructor(
EventType.STICKER -> {
simpleFormat(senderName, stringProvider.getString(R.string.send_a_sticker), appendAuthor)
}
EventType.REACTION -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be unused after my changes.

@@ -214,11 +205,6 @@ class DisplayableEventFormatter @Inject constructor(
EventType.STICKER -> {
stringProvider.getString(R.string.send_a_sticker)
}
EventType.REACTION -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the formatThreadSummary() pair of the above deletion, I deleted it for consistency. But let me know if this is a bad idea.

@ofalvai ofalvai marked this pull request as ready for review April 27, 2022 18:22
@ouchadam ouchadam added X-Needs-Product Issue needs input from Product team Z-Community-PR Issue is solved by a community member's PR labels May 6, 2022
@ouchadam
Copy link
Contributor

ouchadam commented May 6, 2022

looping in product to confirm the behaviour

  • Reactions will no longer mark rooms as unread
  • Reactions will no longer show as the last message in room list

@ofalvai
Copy link
Contributor Author

ofalvai commented Sep 7, 2022

Just a friendly ping to make sure it's not forgotten.

If it's okay from the product POV, I'll rebase this PR and resolve conflicts.

@daniellekirkwood
Copy link
Contributor

🎙 Product

The Web product has a setting that changes whether the reaction shows up in the message preview - is that something we have here?

Otherwise, looks good. Glad that we're matching Web!

@ofalvai
Copy link
Contributor Author

ofalvai commented Oct 5, 2022

@daniellekirkwood this change simply disables the unread reactions (as shown on the screenshots). But if you say a new setting is required for this, I'm happy to include that.

Just to clarify: in the web product this is under Labs and it's disabled by default (meaning that this PR will change the default behavior).

@daniellekirkwood
Copy link
Contributor

Good to know, thanks!

@@ -32,6 +32,5 @@ object RoomSummaryConstants {
EventType.CALL_ANSWER,
EventType.ENCRYPTED,
EventType.STICKER,
EventType.REACTION
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would merge this PR if this was the only change (with the changelog). Let's do not touch DisplayableEventFormatter.kt and strings.xml, just in case this is added again (in the project or in a fork).

@bmarty
Copy link
Member

bmarty commented Jan 10, 2023

@ofalvai can I ask you to handle #5846 (comment)? Then we could merge this PR. Thanks!

@bmarty bmarty removed the X-Needs-Product Issue needs input from Product team label Jan 10, 2023
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

don't mark room as unread due to emoji reactions
5 participants