From a13c08a45b712245bfb9b1e8f10d9e3306b74e2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Wed, 19 Jul 2017 01:26:29 -0400 Subject: [PATCH 1/3] Enforce correct order for previews on server-side prefectch rather than at client parsing This has the benefit of not adding `.preview` divs everywhere, anytime we use `parse()`, and also to un-tie the position of the preview blocks from the result of the helper. This means that templates that call `parse` and have some extra markup after that are not constrained anymore. This is effectively an alternative, better way to fix https://github.com/thelounge/lounge/issues/1343, but the initial fix that was put in place (https://github.com/thelounge/lounge/pull/1347) is still relevant, for example to make sure a preview stays hidden (and does not add extra margin/padding/etc.) if the link does not prefetch. --- client/js/libs/handlebars/parse.js | 3 -- client/views/actions/action.tpl | 4 ++ client/views/msg.tpl | 8 +++- src/models/msg.js | 1 + src/plugins/irc-events/link.js | 20 ++++----- src/plugins/irc-events/message.js | 3 +- test/client/js/libs/handlebars/parse.js | 56 +++++++------------------ test/plugins/link.js | 26 +++++++----- test/util.js | 1 + 9 files changed, 55 insertions(+), 67 deletions(-) diff --git a/client/js/libs/handlebars/parse.js b/client/js/libs/handlebars/parse.js index 1a581904..b5e9e5d7 100644 --- a/client/js/libs/handlebars/parse.js +++ b/client/js/libs/handlebars/parse.js @@ -81,8 +81,5 @@ module.exports = function parse(text) { } return fragments; - }).join("") + linkParts.map((part) => { - const escapedLink = Handlebars.Utils.escapeExpression(part.link); - return `
`; }).join(""); }; diff --git a/client/views/actions/action.tpl b/client/views/actions/action.tpl index 2941459c..0f7c2133 100644 --- a/client/views/actions/action.tpl +++ b/client/views/actions/action.tpl @@ -1,2 +1,6 @@ {{> ../user_name nick=from}} {{{parse text}}} + +{{#each links}} +
+{{/each}} diff --git a/client/views/msg.tpl b/client/views/msg.tpl index 6b5eaf7e..b28c1ba0 100644 --- a/client/views/msg.tpl +++ b/client/views/msg.tpl @@ -7,5 +7,11 @@ {{> user_name nick=from}} {{/if}} - {{{parse text}}} + + {{{parse text}}} + + {{#each links}} +
+ {{/each}} + diff --git a/src/models/msg.js b/src/models/msg.js index 4421b5de..c0ede193 100644 --- a/src/models/msg.js +++ b/src/models/msg.js @@ -31,6 +31,7 @@ function Msg(attr) { _.defaults(this, attr, { from: "", id: id++, + links: [], previews: [], text: "", type: Msg.Type.MESSAGE, diff --git a/src/plugins/irc-events/link.js b/src/plugins/irc-events/link.js index fbf00479..c6f75a12 100644 --- a/src/plugins/irc-events/link.js +++ b/src/plugins/irc-events/link.js @@ -24,19 +24,19 @@ module.exports = function(client, chan, msg) { return; } - Array.from(new Set( // Remove duplicate links + msg.links = 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; - } + )).slice(0, 5); // Only preview the first 5 URLs in message to avoid abuse - parse(msg, link, res, client); - }); + msg.links.forEach((link) => { + fetch(link, function(res) { + if (res === null) { + return; + } + + parse(msg, link, res, client); }); + }); }; function parse(msg, url, res, client) { diff --git a/src/plugins/irc-events/message.js b/src/plugins/irc-events/message.js index f83f1b4a..039f5d62 100644 --- a/src/plugins/irc-events/message.js +++ b/src/plugins/irc-events/message.js @@ -89,11 +89,12 @@ module.exports = function(irc, network) { self: self, highlight: highlight }); - chan.pushMessage(client, msg, !self); // No prefetch URLs unless are simple MESSAGE or ACTION types if ([Msg.Type.MESSAGE, Msg.Type.ACTION].indexOf(data.type) !== -1) { LinkPrefetch(client, chan, msg); } + + chan.pushMessage(client, msg, !self); } }; diff --git a/test/client/js/libs/handlebars/parse.js b/test/client/js/libs/handlebars/parse.js index aa0e5050..3697a5ba 100644 --- a/test/client/js/libs/handlebars/parse.js +++ b/test/client/js/libs/handlebars/parse.js @@ -37,15 +37,13 @@ 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: @@ -53,8 +51,7 @@ describe("parse Handlebars helper", () => { "" + "https://thelounge.github.io/" + "" + - " for more information" + - "
" + " for more information" }, { input: "use www.duckduckgo.com for privacy reasons", expected: @@ -62,26 +59,13 @@ 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)); @@ -97,8 +81,7 @@ 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); @@ -113,8 +96,7 @@ describe("parse Handlebars helper", () => { "" + "https://theos.kyriasis.com/~kyrias/stats/archlinux.html" + "" + - ">" + - "
" + ">" }, { input: "abc (www.example.com)", expected: @@ -122,22 +104,19 @@ 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)); @@ -274,8 +253,7 @@ describe("parse Handlebars helper", () => { "freenode.net" + "/" + "thelounge" + - "" + - "
" + "" }, { input: "\x02#\x038,9thelounge", expected: @@ -314,16 +292,14 @@ 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)); @@ -339,8 +315,7 @@ describe("parse Handlebars helper", () => { "" + "" + "http://example.com/#hash" + - "" + - "
" + "" }]; const actual = testCases.map((testCase) => parse(testCase.input)); @@ -355,8 +330,7 @@ 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 3f2aa9bc..61e00b72 100644 --- a/test/plugins/link.js +++ b/test/plugins/link.js @@ -27,8 +27,9 @@ describe("Link plugin", function() { }); it("should be able to fetch basic information about URLs", function(done) { + const url = "http://localhost:9002/basic"; const message = this.irc.createMessage({ - text: "http://localhost:9002/basic" + text: url }); link(this.irc, this.network.channels[0], message); @@ -41,8 +42,10 @@ describe("Link plugin", function() { expect(data.preview.type).to.equal("link"); expect(data.preview.head).to.equal("test title"); expect(data.preview.body).to.equal("simple description"); - expect(data.preview.link).to.equal("http://localhost:9002/basic"); - expect(message.previews.length).to.equal(1); + expect(data.preview.link).to.equal(url); + + expect(message.links).to.deep.equal([url]); + expect(message.previews).to.deep.equal([data.preview]); done(); }); }); @@ -178,6 +181,11 @@ describe("Link plugin", function() { link(this.irc, this.network.channels[0], message); + expect(message.links).to.deep.equal([ + "http://localhost:9002/one", + "http://localhost:9002/two" + ]); + this.app.get("/one", function(req, res) { res.send("first title"); }); @@ -186,22 +194,18 @@ describe("Link plugin", function() { res.send("second title"); }); - const loaded = { - one: false, - two: false - }; + const previews = []; 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; } + previews.push(data.preview); - if (loaded.one && loaded.two) { - expect(message.previews.length).to.equal(2); + if (previews.length === 2) { + expect(message.previews).to.deep.equal(previews); done(); } }); diff --git a/test/util.js b/test/util.js index bee4c3af..69a84d60 100644 --- a/test/util.js +++ b/test/util.js @@ -21,6 +21,7 @@ MockClient.prototype.createMessage = function(opts) { text: "dummy message", nick: "test-user", target: "#test-channel", + links: [], previews: [], }, opts); From 1c8ea0b75c01b73ea1184fc07df18f6d744bbcd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Thu, 20 Jul 2017 01:58:04 -0400 Subject: [PATCH 2/3] Fix preserved whitespace-related issues for previews by separating them from main text --- client/css/style.css | 26 +++++++++++++------------- client/js/render.js | 4 ++-- client/js/renderPreview.js | 2 +- client/views/actions/action.tpl | 4 ++-- client/views/msg.tpl | 4 ++-- client/views/msg_action.tpl | 2 +- client/views/msg_unhandled.tpl | 2 +- 7 files changed, 22 insertions(+), 22 deletions(-) diff --git a/client/css/style.css b/client/css/style.css index b8f3c155..0f90db98 100644 --- a/client/css/style.css +++ b/client/css/style.css @@ -943,7 +943,7 @@ kbd { #chat .time, #chat .from, -#chat .text { +#chat .content { display: block; padding: 2px 0; flex: 0 0 auto; @@ -965,7 +965,7 @@ kbd { position: relative; } -#chat .text { +#chat .content { flex: 1 1 auto; overflow: hidden; } @@ -1024,7 +1024,7 @@ kbd { #chat.colored-nicks .user.color-31 { color: #ff4846; } #chat.colored-nicks .user.color-32 { color: #ff199b; } -#chat .text { +#chat .content { padding-left: 10px; padding-right: 6px; } @@ -1093,14 +1093,14 @@ kbd { display: none !important; } -#chat .join .text, -#chat .kick .text, -#chat .mode .text, -#chat .nick .text, -#chat .part .text, -#chat .quit .text, -#chat .topic .text, -#chat .topic_set_by .text { +#chat .join .content, +#chat .kick .content, +#chat .mode .content, +#chat .nick .content, +#chat .part .content, +#chat .quit .content, +#chat .topic .content, +#chat .topic_set_by .content { color: #999; } @@ -1978,7 +1978,7 @@ part/quit messages where we don't load previews (adds a blank line otherwise) */ #chat .time, #chat .from, - #chat .text { + #chat .content { border: 0; display: inline; padding: 0; @@ -2097,7 +2097,7 @@ part/quit messages where we don't load previews (adds a blank line otherwise) */ #chat .part-reason, #chat .quit-reason, #chat .new-topic, -#chat .action-text, +#chat .action .text, #chat table.channel-list .topic { white-space: pre-wrap; } diff --git a/client/js/render.js b/client/js/render.js index 721dd603..57f4da61 100644 --- a/client/js/render.js +++ b/client/js/render.js @@ -71,10 +71,10 @@ function buildChatMessage(data) { } const msg = $(templates[template](data.msg)); - const text = msg.find(".text"); + const content = msg.find(".content"); if (template === "msg_action") { - text.html(templates.actions[type](data.msg)); + content.html(templates.actions[type](data.msg)); } data.msg.previews.forEach((preview) => { diff --git a/client/js/renderPreview.js b/client/js/renderPreview.js index c6f6d21d..947ebd02 100644 --- a/client/js/renderPreview.js +++ b/client/js/renderPreview.js @@ -34,7 +34,7 @@ function renderPreview(preview, msg) { $("#chat").on("click", ".toggle-button", function() { const self = $(this); const container = self.closest(".chat"); - const content = self.closest(".text") + const content = self.closest(".content") .find(`.preview[data-url="${self.data("url")}"] .toggle-content`); const bottom = container.isScrollBottom(); diff --git a/client/views/actions/action.tpl b/client/views/actions/action.tpl index 0f7c2133..396d8be7 100644 --- a/client/views/actions/action.tpl +++ b/client/views/actions/action.tpl @@ -1,6 +1,6 @@ {{> ../user_name nick=from}} -{{{parse text}}} +{{{parse text}}} {{#each links}} -
+
{{/each}} diff --git a/client/views/msg.tpl b/client/views/msg.tpl index b28c1ba0..a3019185 100644 --- a/client/views/msg.tpl +++ b/client/views/msg.tpl @@ -7,8 +7,8 @@ {{> user_name nick=from}} {{/if}} - - {{{parse text}}} + + {{{parse text}}} {{#each links}}
diff --git a/client/views/msg_action.tpl b/client/views/msg_action.tpl index 02187a3f..c3d5c772 100644 --- a/client/views/msg_action.tpl +++ b/client/views/msg_action.tpl @@ -3,5 +3,5 @@ {{tz time}} - + diff --git a/client/views/msg_unhandled.tpl b/client/views/msg_unhandled.tpl index 69ad3fe5..0316676e 100644 --- a/client/views/msg_unhandled.tpl +++ b/client/views/msg_unhandled.tpl @@ -3,7 +3,7 @@ {{tz time}} [{{command}}] - + {{#each params}} {{this}} {{/each}} From 900d41bf477ef3f7d1469315b7e07d92d50aed9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9mie=20Astori?= Date: Fri, 21 Jul 2017 01:28:51 -0400 Subject: [PATCH 3/3] Re-use `.previews` to order incoming previews instead of extra `links` --- client/js/renderPreview.js | 4 ++++ client/views/actions/action.tpl | 4 ++-- client/views/msg.tpl | 4 ++-- src/models/msg.js | 1 - src/plugins/irc-events/link.js | 28 ++++++++++++---------------- test/plugins/link.js | 33 +++++++++++++++++++++++++-------- test/util.js | 1 - 7 files changed, 45 insertions(+), 30 deletions(-) diff --git a/client/js/renderPreview.js b/client/js/renderPreview.js index 947ebd02..e88c563e 100644 --- a/client/js/renderPreview.js +++ b/client/js/renderPreview.js @@ -8,6 +8,10 @@ const templates = require("../views"); module.exports = renderPreview; function renderPreview(preview, msg) { + if (preview.type === "loading") { + return; + } + preview.shown = options.shouldOpenMessagePreview(preview.type); const container = msg.closest(".chat"); diff --git a/client/views/actions/action.tpl b/client/views/actions/action.tpl index 396d8be7..f11a0d35 100644 --- a/client/views/actions/action.tpl +++ b/client/views/actions/action.tpl @@ -1,6 +1,6 @@ {{> ../user_name nick=from}} {{{parse text}}} -{{#each links}} -
+{{#each previews}} +
{{/each}} diff --git a/client/views/msg.tpl b/client/views/msg.tpl index a3019185..9b5b13f2 100644 --- a/client/views/msg.tpl +++ b/client/views/msg.tpl @@ -10,8 +10,8 @@ {{{parse text}}} - {{#each links}} -
+ {{#each previews}} +
{{/each}} diff --git a/src/models/msg.js b/src/models/msg.js index c0ede193..4421b5de 100644 --- a/src/models/msg.js +++ b/src/models/msg.js @@ -31,7 +31,6 @@ function Msg(attr) { _.defaults(this, attr, { from: "", id: id++, - links: [], previews: [], text: "", type: Msg.Type.MESSAGE, diff --git a/src/plugins/irc-events/link.js b/src/plugins/irc-events/link.js index c6f75a12..e15cf7a8 100644 --- a/src/plugins/irc-events/link.js +++ b/src/plugins/irc-events/link.js @@ -24,30 +24,28 @@ module.exports = function(client, chan, msg) { return; } - msg.links = Array.from(new Set( // Remove duplicate links + msg.previews = 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 + )).map((link) => ({ + type: "loading", + head: "", + body: "", + thumb: "", + link: link, + })).slice(0, 5); // Only preview the first 5 URLs in message to avoid abuse - msg.links.forEach((link) => { - fetch(link, function(res) { + msg.previews.forEach((preview) => { + fetch(preview.link, function(res) { if (res === null) { return; } - parse(msg, link, res, client); + parse(msg, preview, res, client); }); }); }; -function parse(msg, url, res, client) { - const preview = { - type: "", - head: "", - body: "", - thumb: "", - link: url, - }; - +function parse(msg, preview, res, client) { switch (res.type) { case "text/html": var $ = cheerio.load(res.data); @@ -130,8 +128,6 @@ function emitPreview(client, msg, preview) { } } - msg.previews.push(preview); - client.emit("msg:preview", { id: msg.id, preview: preview diff --git a/test/plugins/link.js b/test/plugins/link.js index 61e00b72..a0d9aff8 100644 --- a/test/plugins/link.js +++ b/test/plugins/link.js @@ -34,6 +34,14 @@ describe("Link plugin", function() { link(this.irc, this.network.channels[0], message); + expect(message.previews).to.deep.equal([{ + body: "", + head: "", + link: url, + thumb: "", + type: "loading" + }]); + this.app.get("/basic", function(req, res) { res.send("test title"); }); @@ -44,7 +52,6 @@ describe("Link plugin", function() { expect(data.preview.body).to.equal("simple description"); expect(data.preview.link).to.equal(url); - expect(message.links).to.deep.equal([url]); expect(message.previews).to.deep.equal([data.preview]); done(); }); @@ -181,10 +188,19 @@ describe("Link plugin", function() { link(this.irc, this.network.channels[0], message); - expect(message.links).to.deep.equal([ - "http://localhost:9002/one", - "http://localhost:9002/two" - ]); + expect(message.previews).to.eql([{ + body: "", + head: "", + link: "http://localhost:9002/one", + thumb: "", + type: "loading" + }, { + body: "", + head: "", + link: "http://localhost:9002/two", + thumb: "", + type: "loading" + }]); this.app.get("/one", function(req, res) { res.send("first title"); @@ -199,13 +215,14 @@ describe("Link plugin", function() { this.irc.on("msg:preview", function(data) { if (data.preview.link === "http://localhost:9002/one") { expect(data.preview.head).to.equal("first title"); + previews[0] = data.preview; } else if (data.preview.link === "http://localhost:9002/two") { expect(data.preview.head).to.equal("second title"); + previews[1] = data.preview; } - previews.push(data.preview); - if (previews.length === 2) { - expect(message.previews).to.deep.equal(previews); + if (previews[0] && previews[1]) { + expect(message.previews).to.eql(previews); done(); } }); diff --git a/test/util.js b/test/util.js index 69a84d60..bee4c3af 100644 --- a/test/util.js +++ b/test/util.js @@ -21,7 +21,6 @@ MockClient.prototype.createMessage = function(opts) { text: "dummy message", nick: "test-user", target: "#test-channel", - links: [], previews: [], }, opts);