Correctly format notification text for reply in a thread #2568

[bmarty]

Type of change

• Feature
• Bugfix
• Technical
• Other :

Content

Ensure PlainTextNodeVisitor correctly format mx-reply content.

Motivation and context

Fixes #2568

Screenshots / GIFs

Tests

• See Text in notification for reply in thread is not well formatted #2568

Tested devices

• Physical
• Emulator
• OS version(s):

Checklist

• Changes have been tested on an Android device or Android emulator with API 23
• UI change has been tested on both light and dark themes
• Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
• Pull request is based on the develop branch
• Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
• Pull request includes screenshots or videos if containing UI changes
• Pull request includes a sign off
• You've made a self review of your PR

[github-actions]

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/po4Qa5

[codecov]

Codecov Report

Attention: Patch coverage is 94.59459% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 73.15%. Comparing base (5cc5a0b) to head (c696726).

Files	Patch %	Lines
...ndroid/libraries/matrix/ui/messages/ToPlainText.kt	94.44%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2569      +/-   ##
===========================================
+ Coverage    73.14%   73.15%   +0.01%     
===========================================
  Files         1408     1408              
  Lines        34095    34112      +17     
  Branches      6617     6623       +6     
===========================================
+ Hits         24939    24955      +16     
  Misses        5699     5699              
- Partials      3457     3458       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jmartinesp]

Wouldn't it be better to always pass a non-null StringProvider?

[bmarty]

I tested it, but it's quite painful since this method is used in data class constructor, and other place where we do not have a stringProvider.

[bmarty]

Actually my bad, it's only he case for Document.toPlainText. I will give it a try.

[bmarty]

So I have tried it and it's adding lots of noise (Let me know if you want to see the code). Since there is no added value, I think we can keep the parameter optional. Do you agree @jmartinesp ?

[jmartinesp]

I'm not too sure about the current implementation: wouldn't it be better to add the in reply to part to the notification builder? If we change it here, it will also be changed for the text in the 'replied to' timeline events, in the room list for the last event, etc.

[bmarty]

I'm not too sure about the current implementation: wouldn't it be better to add the in reply to part to the notification builder? If we change it here, it will also be changed for the text in the 'replied to' timeline events, in the room list for the last event, etc.

I believe the reply part is handle differently on the timeline Event. The change in the code should only impact how event from notification are built (with type NotificationContent.MessageLike.RoomMessage). See some screenshots below, there is not regression:

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[jmartinesp]

I believe the reply part is handle differently on the timeline Event. The change in the code should only impact how event from notification are built (with type NotificationContent.MessageLike.RoomMessage). See some screenshots below, there is not regression:

Oh, it's not happening because the HTML somehow doesn't contain the mx-reply there, weird!

i.e. this is what I see in the debugger:

<html>
 <head></head>
 <body>
  Some text
 </body>
</html>

And this is the formatted body from the view source code screen:

<mx-reply><blockquote><a href=\"https://matrix.to/#/!some-link">In reply to</a> <a href=\"https://matrix.to/#/@some-user\">@some-user:matrix.org</a><br><a href=\"https://matrix.to/#/@some-other-user:matrix.org\">some-user-uesr</a>: original text.</blockquote></mx-reply>Some text

[bmarty]

Yes, I guess the SDK is doing some cleanup for us.

[jmartinesp]

So how about we do it in some different way? We could create something to detect if a message contains this mx-reply block and then extract both the message we replied to and the new one, and format them in some class related to the notifications, where we can access the StringProvider. It's a bit more work, but at least we wouldn't have to make the ToPlainText functions aware of these kind of replies.

Alternatively, if we do want to have this as part of the toPlainText() result everywhere, I think the second best thing we could do is create some PlainTextFormatter that can be injected instead of these functions, and use it in TimelineItemContentMessageFactory and other parts of the app.

[jmartinesp]

Also, the current rendering seems a bit broken:

The first one is a reply from another user to one of their messages in a thread.
The second one is a reply from that same user to one of my messages in the same thread.

[bmarty]

Yes the current rendering is broken, see #2568

I did not test with the mention though.

[jmartinesp]

Yes the current rendering is broken, see #2568

I did not test with the mention though.

To be clear, by 'current rendering' I meant with this branch, not with the code in develop. But I think I might have been looking at EX release installed in the phone and not EX debug 🤦 .

EDIT: yes, I can confirm it looks ok and was looking at the notifications from the wrong app...