No prefetch URLs unless are messages #812
Conversation
|
What made you open this? I just tried with a |
|
I've found that URIs in notice are parsed on server window |
|
Gotcha @xPaw. What about allowing |
|
@astorije Dunno, IRC rfc tends to say that actions should not be acted upon anyhow. |
|
Hmm, odd. In terms of UX, as a user, I would expect that "I love that picture: URL" and "/me loves this picture: URL" are being expanded the same in The Lounge. |
| @@ -91,6 +91,9 @@ module.exports = function(irc, network) { | |||
| }); | |||
| chan.pushMessage(client, msg, !self); | |||
|
|
|||
| LinkPrefetch(client, chan, msg); | |||
| // No prefetch URLs unless are simple messages. | |||
| if (data.type === Msg.Type.MESSAGE) { | |||
There was a problem hiding this comment.
I would add Msg.Type.ACTION here. If strong opinion against this, I'll change my mind, but as a user I'd expect that.
There was a problem hiding this comment.
Action is "/me", yeah? I agree that prefetch should work with those as well.
There was a problem hiding this comment.
if (data.type === Msg.Type.MESSAGE || data.type === Msg.Type.ACTION) {would probably do the trick.
There was a problem hiding this comment.
@xPaw, I'd rather not. I'd prefer a whitelist than a blacklist, at least at this stage and for v2.2.
That would mean filtering out topics as well probably, parts, quits, aways, etc.
|
BTW, this is a regression introduced after 2.1.0 so I'm tagging this one as required for 2.2.0 😢 |
astorije
added
the
Type: Bug
|
Hey guys. Sorry for delay. In my opinion, I think it is irrelevant to the user to have notices prefetched on Lobby window (just my 2 cents) that's why i've request this merge |
|
@birkof, I believe we all agree on this :)
Could you add this change here please? |
|
@birkof If you won't have time in the next few weeks, can you let us know, so we can either push it to the next release or update it ourselves. I don't think this is something we want to hold up the release for. |
|
I've also added the ACTION type to prefetching condition. |
|
Woops, looks like you missed your rebase seeing the massive commit list and changes. If you make sure that the "Allow maintainers to make changes" checkbox is checked on the top right section of this page and agree with it, I can fix your branch. |
|
Look at what that file handles, it only affects messages.
…On Sat, 14 Jan 2017, 03:23 Jérémie Astori, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/plugins/irc-events/message.js
<#812>:
> @@ -91,6 +91,9 @@ module.exports = function(irc, network) {
});
chan.pushMessage(client, msg, !self);
- LinkPrefetch(client, chan, msg);
+ // No prefetch URLs unless are simple messages.
+ if (data.type === Msg.Type.MESSAGE) {
@xPaw <https://github.com/xPaw>, I'd rather not. I'd prefer a whitelist
than a blacklist, at least at this stage and for v2.2.
That would mean filtering out topics as well probably, parts, quits,
aways, etc.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#812>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAlb0-bBWvLLiCyL27gFC9Eow92ByaIgks5rSCOngaJpZM4LQA1X>
.
|
|
@xPaw look at message.js file. it handles messages and actions. |
|
@astorije I allowed edits from maintainers. My mistake was to not make a new branch with this fix, and put it into master. |
ACTION & MESSAGE type messages should be prefetched by default
There was a problem hiding this comment.
I have fixed your branch (there is nothing a git rebase cannot clean up :P) as well as squashed the commits. I've also took the liberty to one-line your if statement, 6 lines was too much for me :D
I'm good with it now, what do @xPaw and @YaManicKill say?
|
Peeeeerfect! Thanks |
No prefetch URLs unless are messages
|
Hey @birkof, we have sticker packs for our contributors now! |
No description provided.