From 64ebe0f4372b789e79033b7307d0259360d96ed1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Thu, 6 Jul 2017 02:16:01 -0400 Subject: [PATCH 1/2] Support multiple previews per message - Load up to 5 previews per message (to avoid abuse) - Do not load multiple times the same URL - Prepare preview containers per message instead of appending (to maintain correct order) - Store an array of previews instead of a single preview in `Msg` objects - Consolidate preview rendering for new messages and upon refresh/load history (when rendering entire channels) - Update `parse` tests to reflect previous point - Add test for multiple URLs - Switch preview tests from `assert` API to `expect` API --- client/js/libs/handlebars/parse.js | 3 ++ client/js/render.js | 15 ++++-- client/js/renderPreview.js | 57 +++++++++++++++++++++ client/js/socket-events/msg_preview.js | 46 ++--------------- client/views/msg.tpl | 7 +-- src/models/msg.js | 1 + src/plugins/irc-events/link.js | 21 +++++--- test/client/js/libs/handlebars/parse.js | 56 +++++++++++++++------ test/plugins/link.js | 67 +++++++++++++++++++------ test/util.js | 3 +- 10 files changed, 185 insertions(+), 91 deletions(-) create mode 100644 client/js/renderPreview.js diff --git a/client/js/libs/handlebars/parse.js b/client/js/libs/handlebars/parse.js index b5e9e5d7..2d65c69e 100644 --- a/client/js/libs/handlebars/parse.js +++ b/client/js/libs/handlebars/parse.js @@ -81,5 +81,8 @@ module.exports = function parse(text) { } return fragments; + }).join("") + linkParts.map((part) => { + const escapedLink = Handlebars.Utils.escapeExpression(part.link); + return `
`; }).join(""); }; diff --git a/client/js/render.js b/client/js/render.js index 88c1e311..4381313e 100644 --- a/client/js/render.js +++ b/client/js/render.js @@ -3,6 +3,7 @@ const $ = require("jquery"); const templates = require("../views"); const options = require("./options"); +const renderPreview = require("./renderPreview"); const utils = require("./utils"); const sorting = require("./sorting"); @@ -15,7 +16,7 @@ module.exports = { renderChannel, renderChannelMessages, renderChannelUsers, - renderNetworks + renderNetworks, }; function buildChannelMessages(channel, messages) { @@ -35,9 +36,9 @@ function buildChatMessage(data) { target = "#chan-" + chat.find(".active").data("id"); } - if (data.msg.preview) { - data.msg.preview.shown = options.shouldOpenMessagePreview(data.msg.preview.type); - } + data.msg.previews.forEach((preview) => { + preview.shown = options.shouldOpenMessagePreview(preview.type); + }); const chan = chat.find(target); let template = "msg"; @@ -72,6 +73,12 @@ function buildChatMessage(data) { const msg = $(templates[template](data.msg)); const text = msg.find(".text"); + if (data.msg.previews.length) { + data.msg.previews.forEach((preview) => { + renderPreview(preview, msg); + }); + } + if (template === "msg_action") { text.html(templates.actions[type](data.msg)); } diff --git a/client/js/renderPreview.js b/client/js/renderPreview.js new file mode 100644 index 00000000..a1f1ce54 --- /dev/null +++ b/client/js/renderPreview.js @@ -0,0 +1,57 @@ +"use strict"; + +const $ = require("jquery"); + +const options = require("./options"); +const templates = require("../views"); + +module.exports = renderPreview; + +function renderPreview(preview, msg) { + preview.shown = options.shouldOpenMessagePreview(preview.type); + + const container = msg.closest(".chat"); + let bottom = false; + if (container.length) { + bottom = container.isScrollBottom(); + } + + msg.find(`[data-url="${preview.link}"]`) + .first() + .append(templates.msg_preview({preview: preview})); + + if (preview.shown && bottom) { + handleImageInPreview(msg.find(".toggle-content"), container); + } + + container.trigger("keepToBottom"); +} + +$("#chat").on("click", ".toggle-button", function() { + const self = $(this); + const container = self.closest(".chat"); + const content = self.parent().next(".toggle-content"); + const bottom = container.isScrollBottom(); + + if (bottom && !content.hasClass("show")) { + handleImageInPreview(content, container); + } + + content.toggleClass("show"); + + // If scrollbar was at the bottom before toggling the preview, keep it at the bottom + if (bottom) { + container.scrollBottom(); + } +}); + +function handleImageInPreview(content, container) { + const img = content.find("img"); + + // Trigger scroll logic after the image loads + if (img.length && !img.width()) { + img.on("load", function() { + container.trigger("keepToBottom"); + }); + } +} diff --git a/client/js/socket-events/msg_preview.js b/client/js/socket-events/msg_preview.js index 7f90784d..81a3d617 100644 --- a/client/js/socket-events/msg_preview.js +++ b/client/js/socket-events/msg_preview.js @@ -1,51 +1,13 @@ "use strict"; const $ = require("jquery"); +const Handlebars = require("handlebars/runtime"); +const renderPreview = require("../renderPreview"); const socket = require("../socket"); -const templates = require("../../views"); -const options = require("../options"); socket.on("msg:preview", function(data) { - data.preview.shown = options.shouldOpenMessagePreview(data.preview.type); - const msg = $("#msg-" + data.id); - const container = msg.closest(".chat"); - const bottom = container.isScrollBottom(); - msg.find(".text").append(templates.msg_preview({preview: data.preview})); - - if (data.preview.shown && bottom) { - handleImageInPreview(msg.find(".toggle-content"), container); - } - - container.trigger("keepToBottom"); + data.link = Handlebars.Utils.escapeExpression(data.link); + renderPreview(data.preview, msg); }); - -$("#chat").on("click", ".toggle-button", function() { - const self = $(this); - const container = self.closest(".chat"); - const content = self.parent().next(".toggle-content"); - const bottom = container.isScrollBottom(); - - if (bottom && !content.hasClass("show")) { - handleImageInPreview(content, container); - } - - content.toggleClass("show"); - - // If scrollbar was at the bottom before toggling the preview, keep it at the bottom - if (bottom) { - container.scrollBottom(); - } -}); - -function handleImageInPreview(content, container) { - const img = content.find("img"); - - // Trigger scroll logic after the image loads - if (img.length && !img.width()) { - img.on("load", function() { - container.trigger("keepToBottom"); - }); - } -} diff --git a/client/views/msg.tpl b/client/views/msg.tpl index 5dfaf86a..6b5eaf7e 100644 --- a/client/views/msg.tpl +++ b/client/views/msg.tpl @@ -7,10 +7,5 @@ {{> user_name nick=from}} {{/if}} - - {{~{parse text}~}} - {{#if preview}} - {{> msg_preview}} - {{/if}} - + {{{parse text}}} diff --git a/src/models/msg.js b/src/models/msg.js index c71aa7d6..4421b5de 100644 --- a/src/models/msg.js +++ b/src/models/msg.js @@ -31,6 +31,7 @@ function Msg(attr) { _.defaults(this, attr, { from: "", id: id++, + previews: [], text: "", type: Msg.Type.MESSAGE, self: false diff --git a/src/plugins/irc-events/link.js b/src/plugins/irc-events/link.js index 7dd593b9..e0f0120f 100644 --- a/src/plugins/irc-events/link.js +++ b/src/plugins/irc-events/link.js @@ -23,14 +23,19 @@ module.exports = function(client, chan, msg) { return; } - const link = escapeHeader(links[0].link); - fetch(link, function(res) { - if (res === null) { - return; - } + Array.from(new Set( // Remove duplicate links + links.map((link) => escapeHeader(link.link)) + )) + .slice(0, 5) // Only preview the first 5 URLs in message to avoid abuse + .forEach((link) => { + fetch(link, function(res) { + if (res === null) { + return; + } - parse(msg, link, res, client); - }); + parse(msg, link, res, client); + }); + }); }; function parse(msg, url, res, client) { @@ -110,7 +115,7 @@ function emitPreview(client, msg, preview) { } } - msg.preview = preview; + msg.previews.push(preview); client.emit("msg:preview", { id: msg.id, diff --git a/test/client/js/libs/handlebars/parse.js b/test/client/js/libs/handlebars/parse.js index f8d5a091..5a011e77 100644 --- a/test/client/js/libs/handlebars/parse.js +++ b/test/client/js/libs/handlebars/parse.js @@ -37,13 +37,15 @@ describe("parse Handlebars helper", () => { expected: "" + "irc://freenode.net/thelounge" + - "" + "" + + "
" }, { input: "www.nooooooooooooooo.com", expected: "" + "www.nooooooooooooooo.com" + - "" + "" + + "
" }, { input: "look at https://thelounge.github.io/ for more information", expected: @@ -51,7 +53,8 @@ describe("parse Handlebars helper", () => { "" + "https://thelounge.github.io/" + "" + - " for more information", + " for more information" + + "
" }, { input: "use www.duckduckgo.com for privacy reasons", expected: @@ -59,13 +62,26 @@ describe("parse Handlebars helper", () => { "" + "www.duckduckgo.com" + "" + - " for privacy reasons" + " for privacy reasons" + + "
" }, { input: "svn+ssh://example.org", expected: "" + "svn+ssh://example.org" + - "" + "" + + "
" + }, { + input: "https://example.com https://example.org", + expected: + "" + + "https://example.com" + + " " + + "" + + "https://example.org" + + "" + + "
" + + "
" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -81,7 +97,8 @@ describe("parse Handlebars helper", () => { "bonuspunkt: your URL parser misparses this URL: " + "" + "https://msdn.microsoft.com/en-us/library/windows/desktop/ms644989(v=vs.85).aspx" + - ""; + "" + + "
"; const actual = parse(input); @@ -96,7 +113,8 @@ describe("parse Handlebars helper", () => { "" + "https://theos.kyriasis.com/~kyrias/stats/archlinux.html" + "" + - ">" + ">" + + "
" }, { input: "abc (www.example.com)", expected: @@ -104,19 +122,22 @@ describe("parse Handlebars helper", () => { "" + "www.example.com" + "" + - ")" + ")" + + "
" }, { input: "http://example.com/Test_(Page)", expected: "" + "http://example.com/Test_(Page)" + - "" + "" + + "
" }, { input: "www.example.com/Test_(Page)", expected: "" + "www.example.com/Test_(Page)" + - "" + "" + + "
" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -253,7 +274,8 @@ describe("parse Handlebars helper", () => { "freenode.net" + "/" + "thelounge" + - "" + "" + + "
" }, { input: "\x02#\x038,9thelounge", expected: @@ -292,14 +314,16 @@ describe("parse Handlebars helper", () => { "like.." + "" + "http://example.com" + - "" + "" + + "
" }, { input: "like..HTTP://example.com", expected: "like.." + "" + "HTTP://example.com" + - "" + "" + + "
" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -315,7 +339,8 @@ describe("parse Handlebars helper", () => { "" + "" + "http://example.com/#hash" + - "" + "" + + "
" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -330,7 +355,8 @@ describe("parse Handlebars helper", () => { expect(actual).to.equal( "Url: http://example.com/path " + - "Channel: ##channel" + "Channel: ##channel" + + "
" ); }); }); diff --git a/test/plugins/link.js b/test/plugins/link.js index 2a7acba5..e0a0b0ea 100644 --- a/test/plugins/link.js +++ b/test/plugins/link.js @@ -1,6 +1,6 @@ "use strict"; -var assert = require("assert"); +const expect = require("chai").expect; var util = require("../util"); var link = require("../../src/plugins/irc-events/link.js"); @@ -36,9 +36,10 @@ describe("Link plugin", function() { }); this.irc.once("msg:preview", function(data) { - assert.equal(data.preview.type, "link"); - assert.equal(data.preview.head, "test title"); - assert.equal(data.preview.body, "simple description"); + expect(data.preview.type).to.equal("link"); + expect(data.preview.head).to.equal("test title"); + expect(data.preview.body).to.equal("simple description"); + expect(message.previews.length).to.equal(1); done(); }); }); @@ -55,7 +56,7 @@ describe("Link plugin", function() { }); this.irc.once("msg:preview", function(data) { - assert.equal(data.preview.head, "opengraph test"); + expect(data.preview.head, "opengraph test"); done(); }); }); @@ -72,7 +73,7 @@ describe("Link plugin", function() { }); this.irc.once("msg:preview", function(data) { - assert.equal(data.preview.body, "opengraph description"); + expect(data.preview.body).to.equal("opengraph description"); done(); }); }); @@ -89,8 +90,8 @@ describe("Link plugin", function() { }); this.irc.once("msg:preview", function(data) { - assert.equal(data.preview.head, "Google"); - assert.equal(data.preview.thumb, "http://localhost:9002/real-test-image.png"); + expect(data.preview.head).to.equal("Google"); + expect(data.preview.thumb).to.equal("http://localhost:9002/real-test-image.png"); done(); }); }); @@ -107,7 +108,7 @@ describe("Link plugin", function() { }); this.irc.once("msg:preview", function(data) { - assert.equal(data.preview.thumb, ""); + expect(data.preview.thumb).to.be.empty; done(); }); }); @@ -124,8 +125,8 @@ describe("Link plugin", function() { }); this.irc.once("msg:preview", function(data) { - assert.equal(data.preview.head, "Untitled page"); - assert.equal(data.preview.thumb, "http://localhost:9002/real-test-image.png"); + expect(data.preview.head).to.equal("Untitled page"); + expect(data.preview.thumb).to.equal("http://localhost:9002/real-test-image.png"); done(); }); }); @@ -142,8 +143,8 @@ describe("Link plugin", function() { }); this.irc.once("msg:preview", function(data) { - assert.equal(data.preview.head, "404 image"); - assert.equal(data.preview.thumb, ""); + expect(data.preview.head).to.equal("404 image"); + expect(data.preview.thumb).to.be.empty; done(); }); }); @@ -156,9 +157,45 @@ describe("Link plugin", function() { link(this.irc, this.network.channels[0], message); this.irc.once("msg:preview", function(data) { - assert.equal(data.preview.type, "image"); - assert.equal(data.preview.link, "http://localhost:9002/real-test-image.png"); + expect(data.preview.type).to.equal("image"); + expect(data.preview.link).to.equal("http://localhost:9002/real-test-image.png"); done(); }); }); + + it("should load multiple URLs found in messages", function(done) { + const message = this.irc.createMessage({ + text: "http://localhost:9002/one http://localhost:9002/two" + }); + + link(this.irc, this.network.channels[0], message); + + this.app.get("/one", function(req, res) { + res.send("first title"); + }); + + this.app.get("/two", function(req, res) { + res.send("second title"); + }); + + const loaded = { + one: false, + two: false + }; + + this.irc.on("msg:preview", function(data) { + if (data.preview.link === "http://localhost:9002/one") { + expect(data.preview.head).to.equal("first title"); + loaded.one = true; + } else if (data.preview.link === "http://localhost:9002/two") { + expect(data.preview.head).to.equal("second title"); + loaded.two = true; + } + + if (loaded.one && loaded.two) { + expect(message.previews.length).to.equal(2); + done(); + } + }); + }); }); diff --git a/test/util.js b/test/util.js index 88e3b409..bee4c3af 100644 --- a/test/util.js +++ b/test/util.js @@ -20,7 +20,8 @@ MockClient.prototype.createMessage = function(opts) { var message = _.extend({ text: "dummy message", nick: "test-user", - target: "#test-channel" + target: "#test-channel", + previews: [], }, opts); return message; From 28200830ed3bad73bdddc3f6df31ef65ee3e468d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Thu, 6 Jul 2017 03:35:54 -0400 Subject: [PATCH 2/2] Move preview toggle buttons next to their respective URLs and switch from ellipsis to caret --- client/css/style.css | 28 ++++++++++++++------ client/js/libs/handlebars/parse.js | 2 +- client/js/renderPreview.js | 10 ++++++-- client/views/index.js | 1 + client/views/msg_preview.tpl | 3 --- client/views/msg_preview_toggle.tpl | 10 ++++++++ test/client/js/libs/handlebars/parse.js | 34 ++++++++++++------------- 7 files changed, 57 insertions(+), 31 deletions(-) create mode 100644 client/views/msg_preview_toggle.tpl diff --git a/client/css/style.css b/client/css/style.css index 4082cff3..f100305e 100644 --- a/client/css/style.css +++ b/client/css/style.css @@ -205,6 +205,7 @@ kbd { #chat .whois .from:before, #chat .nick .from:before, #chat .action .from:before, +#chat .toggle-button:after, .context-menu-item:before, #nick button:before { font: normal normal normal 14px/1 FontAwesome; @@ -293,6 +294,16 @@ kbd { content: "\f005"; /* http://fontawesome.io/icon/star/ */ } +#chat .toggle-button { + /* These 2 directives are loosely taken from .fa-fw */ + width: 1.35em; + text-align: center; +} + +#chat .toggle-button:after { + content: "\f0da"; /* http://fontawesome.io/icon/caret-right/ */ +} + #chat .count:before { color: #cfcfcf; content: "\f002"; /* http://fontawesome.io/icon/search/ */ @@ -1097,17 +1108,18 @@ kbd { } #chat .toggle-button { - background: #f5f5f5; - border-radius: 2px; display: inline-block; color: #666; - height: 1em; - line-height: 0; - padding: 0 6px; + transition: color .2s, transform .2s; } -#chat .toggle-button:after { - content: "ยทยทยท"; +#chat .toggle-button.opened { + transform: rotate(90deg); +} + +#chat .toggle-button:hover { + /* transform and opacity together glitch, so need to use RGBA transition */ + color: rgba(102, 102, 102, .8); /* #666 x .8 opacity */ } #chat .toggle-content { @@ -1118,7 +1130,7 @@ kbd { font-size: 12px; max-width: 100%; padding: 6px; - margin-top: 2px; + margin: 2px 0; overflow: hidden; } diff --git a/client/js/libs/handlebars/parse.js b/client/js/libs/handlebars/parse.js index 2d65c69e..1a581904 100644 --- a/client/js/libs/handlebars/parse.js +++ b/client/js/libs/handlebars/parse.js @@ -83,6 +83,6 @@ module.exports = function parse(text) { return fragments; }).join("") + linkParts.map((part) => { const escapedLink = Handlebars.Utils.escapeExpression(part.link); - return `
`; + return `
`; }).join(""); }; diff --git a/client/js/renderPreview.js b/client/js/renderPreview.js index a1f1ce54..86fb58c0 100644 --- a/client/js/renderPreview.js +++ b/client/js/renderPreview.js @@ -16,7 +16,11 @@ function renderPreview(preview, msg) { bottom = container.isScrollBottom(); } - msg.find(`[data-url="${preview.link}"]`) + msg.find(`.text a[href="${preview.link}"]`) + .first() + .after(templates.msg_preview_toggle({preview: preview})); + + msg.find(`.preview[data-url="${preview.link}"]`) .first() .append(templates.msg_preview({preview: preview})); @@ -30,13 +34,15 @@ function renderPreview(preview, msg) { $("#chat").on("click", ".toggle-button", function() { const self = $(this); const container = self.closest(".chat"); - const content = self.parent().next(".toggle-content"); + const content = self.closest(".text") + .find(`.preview[data-url="${self.data("url")}"] .toggle-content`); const bottom = container.isScrollBottom(); if (bottom && !content.hasClass("show")) { handleImageInPreview(content, container); } + self.toggleClass("opened"); content.toggleClass("show"); // If scrollbar was at the bottom before toggling the preview, keep it at the bottom diff --git a/client/views/index.js b/client/views/index.js index 9de19307..ca571bec 100644 --- a/client/views/index.js +++ b/client/views/index.js @@ -26,6 +26,7 @@ module.exports = { msg: require("./msg.tpl"), msg_action: require("./msg_action.tpl"), msg_preview: require("./msg_preview.tpl"), + msg_preview_toggle: require("./msg_preview_toggle.tpl"), msg_unhandled: require("./msg_unhandled.tpl"), network: require("./network.tpl"), unread_marker: require("./unread_marker.tpl"), diff --git a/client/views/msg_preview.tpl b/client/views/msg_preview.tpl index 2ebaa023..5f56529c 100644 --- a/client/views/msg_preview.tpl +++ b/client/views/msg_preview.tpl @@ -1,7 +1,4 @@ {{#preview}} -
- -
{{#equal type "image"}} diff --git a/client/views/msg_preview_toggle.tpl b/client/views/msg_preview_toggle.tpl new file mode 100644 index 00000000..386282cc --- /dev/null +++ b/client/views/msg_preview_toggle.tpl @@ -0,0 +1,10 @@ +{{#preview}} + +{{/preview}} diff --git a/test/client/js/libs/handlebars/parse.js b/test/client/js/libs/handlebars/parse.js index 5a011e77..aa0e5050 100644 --- a/test/client/js/libs/handlebars/parse.js +++ b/test/client/js/libs/handlebars/parse.js @@ -38,14 +38,14 @@ describe("parse Handlebars helper", () => { "" + "irc://freenode.net/thelounge" + "" + - "
" + "
" }, { input: "www.nooooooooooooooo.com", expected: "" + "www.nooooooooooooooo.com" + "" + - "
" + "
" }, { input: "look at https://thelounge.github.io/ for more information", expected: @@ -54,7 +54,7 @@ describe("parse Handlebars helper", () => { "https://thelounge.github.io/" + "" + " for more information" + - "
" + "
" }, { input: "use www.duckduckgo.com for privacy reasons", expected: @@ -63,14 +63,14 @@ describe("parse Handlebars helper", () => { "www.duckduckgo.com" + "" + " for privacy reasons" + - "
" + "
" }, { input: "svn+ssh://example.org", expected: "" + "svn+ssh://example.org" + "" + - "
" + "
" }, { input: "https://example.com https://example.org", expected: @@ -80,8 +80,8 @@ describe("parse Handlebars helper", () => { "" + "https://example.org" + "" + - "
" + - "
" + "
" + + "
" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -98,7 +98,7 @@ describe("parse Handlebars helper", () => { "" + "https://msdn.microsoft.com/en-us/library/windows/desktop/ms644989(v=vs.85).aspx" + "" + - "
"; + "
"; const actual = parse(input); @@ -114,7 +114,7 @@ describe("parse Handlebars helper", () => { "https://theos.kyriasis.com/~kyrias/stats/archlinux.html" + "" + ">" + - "
" + "
" }, { input: "abc (www.example.com)", expected: @@ -123,21 +123,21 @@ describe("parse Handlebars helper", () => { "www.example.com" + "" + ")" + - "
" + "
" }, { input: "http://example.com/Test_(Page)", expected: "" + "http://example.com/Test_(Page)" + "" + - "
" + "
" }, { input: "www.example.com/Test_(Page)", expected: "" + "www.example.com/Test_(Page)" + "" + - "
" + "
" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -275,7 +275,7 @@ describe("parse Handlebars helper", () => { "/" + "thelounge" + "" + - "
" + "
" }, { input: "\x02#\x038,9thelounge", expected: @@ -315,7 +315,7 @@ describe("parse Handlebars helper", () => { "" + "http://example.com" + "" + - "
" + "
" }, { input: "like..HTTP://example.com", expected: @@ -323,7 +323,7 @@ describe("parse Handlebars helper", () => { "" + "HTTP://example.com" + "" + - "
" + "
" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -340,7 +340,7 @@ describe("parse Handlebars helper", () => { "" + "http://example.com/#hash" + "" + - "
" + "
" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -356,7 +356,7 @@ describe("parse Handlebars helper", () => { expect(actual).to.equal( "Url: http://example.com/path " + "Channel: ##channel" + - "
" + "
" ); }); });