Use localpart in plaintext so we get highlighted for pills #658
Conversation
lib/models/MatrixAction.js
Outdated
| @@ -64,10 +64,18 @@ MatrixAction.prototype.formatMentions = function(nickUserIdMap) { | |||
| this.htmlText = this.text; | |||
| } | |||
| userId = ircFormatting.escapeHtmlChars(userId); | |||
| /* Due to how Riot and friends do push notifications, we need the plain text to match something. | |||
| Modern client's will pill-ify and will show a displayname, so we can use the localpart so it | |||
| matches and doesn't degrade too badly on limited clients.*/ | |||
There was a problem hiding this comment.
minor: double space after matches
lib/models/MatrixAction.js
Outdated
| /* Due to how Riot and friends do push notifications, we need the plain text to match something. | ||
| Modern client's will pill-ify and will show a displayname, so we can use the localpart so it | ||
| matches and doesn't degrade too badly on limited clients.*/ | ||
| const localpart = userId.substr(1, userId.indexOf(":")-1); |
There was a problem hiding this comment.
we should be using the display name, as per matrix-org/matrix-spec-proposals#1547
There was a problem hiding this comment.
You really want to do a displayname lookup for every match? The localparts already match.
There was a problem hiding this comment.
I'd prefer it if the cached display name was used, given we already know about the member event.
There was a problem hiding this comment.
I don't think we do know about the mem...OH you mean the member cache thingatron?
|
Continuing on #658 (comment)
|
|
We already pull members out of the room for the purpose of figuring out mentions. Including partial profiles for those users doesn't seem terribly bad. If we cache miss on the display name, fall back to the complete user ID, not localpart. |
We don't? We could, but we don't currently. |
lib/models/MatrixAction.js
Outdated
|
|
||
| const regex = MentionRegex(escapeStringRegexp(matchName)); | ||
| this.htmlText = this.htmlText.replace(regex, | ||
| `$1<a href="https://matrix.to/#/${userId}">${ircFormatting.escapeHtmlChars(identifier)}</a>` |
spec/unit/MatrixAction.spec.js
Outdated
| }); | ||
| expect(action.text).toEqual("Go to http://JCDenton.com"); | ||
| expect(action.htmlText).toEqual( | ||
| "Go to <a href='http://JCDenton.com'>my website</a>" | ||
| ); |
There was a problem hiding this comment.
a test for making sure it also actually uses the profile where possible would be nice
This PR has a soft dependency on matrix-org/matrix-appservice-bridge#83, where we will only cache if the bridge-sdk is setup to do so. Otherwise, we won't.