damus icon indicating copy to clipboard operation
damus copied to clipboard

Fix note rendering for those that contain previewable items or leading and trailing whitespaces

Open tyiu opened this issue 10 months ago • 11 comments

Summary

There are several issues with rendering notes:

  • URLs, note references, and invoices can be removed mid-sentence which makes the content of the note no longer make sense
  • Leading and trailing whitespaces cause unnecessary visual noise that make it harder to read the note and can be a spam attack vector.
  • Newlines are removed when they shouldn't be and alter the original formatting of the note

This PR fixes it so that:

  • The only time the text of URLs, note references, or invoices can be hidden are if they are sequenced at the end of the content and are the only previewable items that exist in the content.
  • Leading and trailing whitespaces are now trimmed. This also fixes an issue where if there are trailing whitespaces after the last previewable item, the text would not be hidden.
  • Newlines are preserved if they precede a previewable block but aren't at the end of the note
  • If invoices are shown as text, we now abbreviate the middle and show only the first and last few characters of the invoice string. Tapping on inlined invoice text will bring up the wallet selector.

Closes: https://github.com/damus-io/damus/issues/2187

Checklist

  • [x] I have read (or I am familiar with) the Contribution Guidelines
  • [x] I have tested the changes in this PR
  • [x] I have opened or referred to an existing github issue related to this change.
  • [x] My PR is either small, or I have split it into smaller logical commits that are easier to review
  • [x] I have added the signoff line to all my commits. See Signing off your work
  • [x] I have added appropriate changelog entries for the changes in this PR. See Adding changelog entries
  • [x] I have added appropriate Closes: or Fixes: tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patches

Test report

Please provide a test report for the changes in this PR. You can use the template below, but feel free to modify it as needed.

Device: iPhone 16 Pro Simulator

iOS: iOS 18.3.1

Damus: 2ccd3d38b4e69beed8233134a4ea32eeddff7ec5

Steps:

  1. Build and run Damus.
  2. Open the following notes to observe that they render properly (and compare with the behavior without this change where rendering is broken)

nevent1qqsgv35vzvyl4eesqm8quk33jgxkddgvvnxntj3r75nk5jkwljgr4vgyx2pse Simulator Screenshot - iPhone 16 Pro - 2025-03-02 at 00 23 28 Medium

nevent1qqs04gq030x3ew3mgdumy2j324lpww08enx6q06yldmqmv43vr05x9gpz3mhxue69uhhyetvv9ujumn0wd68ytnzvuq36amnwvaz7tmwdaehgu3dwp6kytnhv4kxcmmjv3jhytnwv46qzxrhwden5te0wfjkccte9ehx7um5wfshg6fwvdhk6qg4waehxw309ajkgetw9ehx7um5wghxcctwvsx36csg Simulator Screenshot - iPhone 16 Pro - 2025-03-01 at 17 18 29 Medium

nevent1qqsv9z59d39e47j86577sun65a5a7pqac0hmqjxfzzvh8mqzz0m7facpz3mhxue69uhhyetvv9ujuerpd46hxtnfduq3vamnwvaz7tmjv4kxz7fwdehhxarj9e3xzmnyqyvhwumn8ghj7mn0wd68ytnsd3jkycmgv95kutn0wfnszrthwden5te0dehhxtnvdakqra33mf Simulator Screenshot - iPhone 16 Pro - 2025-03-02 at 00 24 02 Medium

nevent1qqs2qk90sjcwqmn87kclvf5hr23qurafksplsmf30ster4pn7f9rxgqppamhxue69uhkummnw3ezumt0d5q3samnwvaz7tmjv4kxz7fwdehhxarj9e3k7mfwv96szxthwden5te0vfhhxarj9ehx76m0w3shymewwahhy6cpzemhxue69uhkzarvv9ejumn0wd68ytnvv9hxgxl0wqu Simulator Screenshot - iPhone 16 Pro - 2025-03-01 at 17 19 21 Medium

nevent1qqsfzsfgv4qcp2pmrkxjrpxygzjhvpx723qsr0mpy6hxcxcej26u0lgpz3mhxue69uhhyetvv9ujuerpd46hxtnfduq3vamnwvaz7tmjv4kxz7fwdehhxarj9e3xzmnyqgstvj22wngc5t0687qvak06mt34spm3dl8pqu0ymcv7946xkmv8vps95mdqg Simulator Screenshot - iPhone 16 Pro - 2025-03-02 at 00 24 21 Medium

nevent1qqsxxhax8uk9dvguj6mks4awukyvhpazudnmazgs9hjkg365yrchehcpz4mhxue69uhhwmm59eek7anzd96zu6r0wd6qz9thwden5te0vaex2etwwdhh2mpwwdcxzcm9qythwumn8ghj7cmjv4shgu3wdehhxarj9emkjmn9qyd8wumn8ghj7argv4nx7un9wd6zumn0wd68yvfwvdhk6ckaxv2 Simulator Screenshot - iPhone 16 Pro - 2025-03-02 at 00 22 50 Medium

nevent1qqs2nflv99gw3sqe7jk4vcl2j69r078anwxdl5ls9hhyh4jetmyj22cpzemhxue69uhhyetvv9ujumn0wd68ytnzv9hxgqgswaehxw309ahx7um5wgh8w6twv5qkvamnwvaz7tmxd9k8getj9ehx7um5wgh8w6twv5hkuur4vgchjct4dsuxkvp48yenwdm489k8xafkxajx2dmexcenwae5df6xwet4wa3k66p4dcmnwwpcdsm8smnvdeexwuenw3mx5mtx8a38ymmpv33kzum58468yat9qy28wumn8ghj7un9d3shjtnyv9kh2uewd9hsv67rrj Simulator Screenshot - iPhone 16 Pro - 2025-03-02 at 00 31 15 Medium

nevent1qqs8f47tv4mdjkj78a96vg4sh8d2q0nvpwkzggjw7gz4flmhfk6vy4spzpmhxue69uhkummnw3ezuamfdejsz9thwden5te0v4jx2m3wdehhxarj9ekxzmnyqyxhwumn8ghj7mn0wvhxcmmvqy28wumn8ghj7un9d3shjtnyv9kh2uewd9hs8rusn4

https://github.com/user-attachments/assets/411816cf-5892-481d-adf7-b4f1b77b8f99

Results:

  • [x] PASS

tyiu avatar Mar 01 '25 22:03 tyiu

Hmm, there's a bug rendering invoice strings in the selectable view.

tyiu avatar Mar 02 '25 00:03 tyiu

OK, fixed it in my second commit.

https://damus.io/nevent1qqs8f47tv4mdjkj78a96vg4sh8d2q0nvpwkzggjw7gz4flmhfk6vy4spzpmhxue69uhkummnw3ezuamfdejsz9thwden5te0v4jx2m3wdehhxarj9ekxzmnyqyxhwumn8ghj7mn0wvhxcmmvqy28wumn8ghj7un9d3shjtnyv9kh2uewd9hs8rusn4

https://github.com/user-attachments/assets/411816cf-5892-481d-adf7-b4f1b77b8f99

tyiu avatar Mar 02 '25 01:03 tyiu

Oh, there's another issue that I didn't account for. If there are multiple consecutive previewable blocks at the end of the note content, their text should probably be hidden. I'll work on that.

tyiu avatar Mar 02 '25 03:03 tyiu

Oh, there's another issue that I didn't account for. If there are multiple consecutive previewable blocks at the end of the note content, their text should probably be hidden. I'll work on that.

OK, fixed. PR is ready for review again.

tyiu avatar Mar 02 '25 05:03 tyiu

Thanks @tyiu, I will take a close look at this PR soon (probably in a few days), I just need to get through 2 other items first! I added this to my list on this cycle.

danieldaquino avatar Mar 03 '25 20:03 danieldaquino

I'm going to rebase on master and also amend 82fcf123d485cf016ab64a1bea8299b022d44711 into 9883a92ae943ae6948da50ef2c23bc9ab785ea9e for a cleaner branch history.

tyiu avatar Mar 04 '25 00:03 tyiu

we really should have some tests for these

jb55 avatar Mar 05 '25 20:03 jb55

we really should have some tests for these

Agreed. I can do it as part of this PR if you feel that we need it before merging. It's always harder when there's no tests to begin with, but better now than never!

tyiu avatar Mar 05 '25 20:03 tyiu

can just get ai to write them if we're lazy

jb55 avatar Mar 08 '25 00:03 jb55

can just get ai to write them if we're lazy

I have them written locally. Just trying to figure out the event mention carousel edge case. Right now, they can be completely hidden if there are multiple events mentioned.

tyiu avatar Mar 08 '25 04:03 tyiu

Just trying to figure out the event mention carousel edge case.

Thank you for letting us know @tyiu! Please let me know once you resolve those, and I can proceed with the review! Thanks!

danieldaquino avatar Mar 12 '25 21:03 danieldaquino

@danieldaquino Let's merge it and tackle that refactor separately. I don't want to delay or add more complexity in this one.

tyiu avatar May 07 '25 21:05 tyiu

@danieldaquino Let's merge it and tackle that refactor separately. I don't want to delay or add more complexity in this one.

Sounds good, merged!

Opened https://github.com/damus-io/damus/issues/3023 that can be addressed any time in the future.

danieldaquino avatar May 07 '25 22:05 danieldaquino