Repository navigation
IDN link previews - #14707
IDN link previews#14707arnt wants to merge 3 commits into
Conversation
| { "grå.org", true }, // å is Latin script | ||
| { "münchen.de", true }, // ü is Latin script | ||
| { "慕田峪长城.网址", true }, // Great Wall site | ||
| // Мышкин is the idiot in Dostoyevsky's book. |
There was a problem hiding this comment.
I particularly like this test.
|
@alex-signal you did the change before mine on this code… Can you suggest what I ought to do next to get this reviewed/merged? |
|
Hi, any comments on this? |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions! |
ef34d3c to
509a350
Compare
|
I tried a rebase now, to see whether the bot added wontfix due to the conflicts or whether that's due to someone's decision. |
|
I would appreciate knowing whether this is actively wontfix or simply not reviewed for lack of time. |
|
Thanks for the work and rebasing. We'll take a look but won't have an answer for you immediately on which way this'll go. |
|
@cody-signal Thanks for your forthright answer. Forthright is the best kind of nice. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions! |
The previous code mishandled extended Latin such as münchen.de and also did not detect combinations such as Greek/Cyrillic. This solves both the false negatives and the false positives. As of this commit, links to grå.org get the underline treatment on API 24 and later. Character.UnicodeScript does not exist on API 23, so Android 6.0 retains the old behaviour.
As of this commit, a link shared using the share sheet of a browser, or cut and pasted, is displayed in the human-readable form, provided that the domain name doesn't mix scripts. Note that there are two flows for cut and paste, one paste and one that looks as if one key press delivers a long string. This handles both.
509a350 to
c375d80
Compare
|
Hi, I rebased this to the current head now. I also noticed that it was broken on API 23, which Signal still supports, so I fixed that, and added a new test case. (The domain in the new test is one I heard a complaint about recently. Someone from that company didn't like the xn.) I'll keep rebasing very occasionally until you decide whether to merge or reject the PR. |
|
Oh, btw, the people who run my daughter's dance school would be happy if you decide in favour of merging ;) München is a perfectly sensible word. |
First time contributor checklist
Contributor checklist
Fixes #1234syntax — ie. I found none.Description
At present, when you share a site such as https://grå.org from e.g. Chrome on Android via either cut-and-paste or the share sheet, Signal shows a lot of xn--, which is user-unfriendly and IMO a problem waiting to happen. I saw that there's some attempt to make it work, so I improved, tested and tested it:
After this change, the link preview uses a lot of the letters/digits people learn in school and many fewer hex blobs. Basically, I tried to make it so that the link you share is one you can read.
I had to wait a little with the CLA. I rebased my branch and made this PR once I could sign the CLA. So this is properly tested, but I've rebased since, and frankly I expect to rebase more often than I carry out four-device tests.
(FYI, I work with this. This kind of problem is my day job. "Bringing internet to people who can read and write, but aren't so great with A-Z" if you see what I mean. I'll be happy to look at any Signal issue or PR that concerns unicode URLs/domains/addresses or the use of non-ASCII scripts, whether Norwegian or Nepali.)