Fix note rendering for those that contain previewable items or leading and trailing whitespaces
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:orFixes: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:
- Build and run Damus.
- Open the following notes to observe that they render properly (and compare with the behavior without this change where rendering is broken)
nevent1qqsgv35vzvyl4eesqm8quk33jgxkddgvvnxntj3r75nk5jkwljgr4vgyx2pse
https://github.com/user-attachments/assets/411816cf-5892-481d-adf7-b4f1b77b8f99
Results:
- [x] PASS
Hmm, there's a bug rendering invoice strings in the selectable view.
OK, fixed it in my second commit.
https://damus.io/nevent1qqs8f47tv4mdjkj78a96vg4sh8d2q0nvpwkzggjw7gz4flmhfk6vy4spzpmhxue69uhkummnw3ezuamfdejsz9thwden5te0v4jx2m3wdehhxarj9ekxzmnyqyxhwumn8ghj7mn0wvhxcmmvqy28wumn8ghj7un9d3shjtnyv9kh2uewd9hs8rusn4
https://github.com/user-attachments/assets/411816cf-5892-481d-adf7-b4f1b77b8f99
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.
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.
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.
I'm going to rebase on master and also amend 82fcf123d485cf016ab64a1bea8299b022d44711 into 9883a92ae943ae6948da50ef2c23bc9ab785ea9e for a cleaner branch history.
we really should have some tests for these
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!
can just get ai to write them if we're lazy
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.
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 Let's merge it and tackle that refactor separately. I don't want to delay or add more complexity in this one.
@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.