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

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tyiu
Copy link
Collaborator

@tyiu tyiu commented Mar 1, 2025

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: #2187

Checklist

  • I have read (or I am familiar with) the Contribution Guidelines
  • I have tested the changes in this PR
  • I have opened or referred to an existing github issue related to this change.
  • My PR is either small, or I have split it into smaller logical commits that are easier to review
  • I have added the signoff line to all my commits. See Signing off your work
  • I have added appropriate changelog entries for the changes in this PR. See Adding changelog entries
  • 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: 2ccd3d3

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

Screen.Recording.2025-03-01.at.8.43.08.PM.mov

Results:

  • PASS

}

func abbrev_pubkey(_ pubkey: String, amount: Int = 8) -> String {
func abbrev_identifier(_ pubkey: String, amount: Int = 8) -> String {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed because it was already abbreviating more than only pubkeys.

@tyiu
Copy link
Collaborator Author

tyiu commented Mar 2, 2025

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

@tyiu
Copy link
Collaborator Author

tyiu commented Mar 2, 2025

@tyiu
Copy link
Collaborator Author

tyiu commented Mar 2, 2025

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 tyiu marked this pull request as draft March 2, 2025 03:07
@tyiu tyiu force-pushed the hide-last-only-previewable branch from d1a7961 to 2ccd3d3 Compare March 2, 2025 05:24
@tyiu tyiu marked this pull request as ready for review March 2, 2025 05:31
@tyiu
Copy link
Collaborator Author

tyiu commented Mar 2, 2025

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.

@danieldaquino
Copy link
Contributor

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.

@linear linear bot mentioned this pull request Mar 3, 2025
@tyiu
Copy link
Collaborator Author

tyiu commented Mar 4, 2025

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

tyiu added 2 commits March 3, 2025 19:56
…g and trailing whitespaces

Changelog-Fixed: Fixed note rendering for those that contain previewable items or leading and trailing whitespaces
Closes: damus-io#2187
Signed-off-by: Terry Yiu <[email protected]>
Changelog-Added: Added inline note rendering of invoices to pull up wallet selector sheet
Signed-off-by: Terry Yiu <[email protected]>
@tyiu tyiu force-pushed the hide-last-only-previewable branch from 82fcf12 to ab58a94 Compare March 4, 2025 00:56
@jb55
Copy link
Collaborator

jb55 commented Mar 5, 2025

we really should have some tests for these

@tyiu
Copy link
Collaborator Author

tyiu commented Mar 5, 2025

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 tyiu marked this pull request as draft March 6, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide external link URLs
3 participants