element-x-android icon indicating copy to clipboard operation
element-x-android copied to clipboard

Published 20 hours ago • verified publisher icon element-hq

Correctly format notification text for reply in a thread #2568

Open bmarty opened this issue 1 year ago • 11 comments

Type of change

  • [ ] Feature
  • [x] Bugfix
  • [ ] Technical
  • [ ] Other :

Content

Ensure PlainTextNodeVisitor correctly format mx-reply content.

Motivation and context

Fixes #2568

Screenshots / GIFs

Tests

  • See #2568

Tested devices

  • [ ] Physical
  • [x] 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

bmarty avatar Mar 19 '24 12:03 bmarty

:iphone: Scan the QR code below to install the build (arm64 only) for this PR. QR code If you can't scan the QR code you can install the build via this link: https://i.diawi.com/po4Qa5

github-actions[bot] avatar Mar 19 '24 12:03 github-actions[bot]

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 :warning:
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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 19 '24 13:03 codecov[bot]

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.

jmartinesp avatar Mar 19 '24 13:03 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.

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:

image image

bmarty avatar Mar 19 '24 13:03 bmarty

Quality Gate Passed 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

sonarqubecloud[bot] avatar Mar 19 '24 13:03 sonarqubecloud[bot]

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

jmartinesp avatar Mar 19 '24 13:03 jmartinesp

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

bmarty avatar Mar 19 '24 13:03 bmarty

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 avatar Mar 19 '24 14:03 jmartinesp

Also, the current rendering seems a bit broken:

share_994317422670100723

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.

jmartinesp avatar Mar 19 '24 14:03 jmartinesp

Yes the current rendering is broken, see https://github.com/element-hq/element-x-android/issues/2568

I did not test with the mention though.

bmarty avatar Mar 19 '24 14:03 bmarty

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...

jmartinesp avatar Mar 19 '24 14:03 jmartinesp

I cannot repro the issue anymore, something may have changed SDK side. I will also close the issue when I will know what has changed.

bmarty avatar Jun 12 '24 09:06 bmarty