Support multiple previews per message #1303

astorije · 2017-07-06T06:17:15Z

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

It's probably not the nicest code, but it works pretty nicely, and I'm sure reviews will highlight the required improvements.
Note that this does not improve the UI of displaying toggles/previews, this will be better left off to another PR and/or #1301.

Example of multiple URLs (screenshots show an outdated toggle button)

UI of toggle button

xPaw · 2017-07-06T06:20:04Z

client/js/render.js

@@ -90,6 +99,54 @@ function buildChatMessage(data) {
 	return msg;
 }

+function renderPreview(preview, msg) {


Can we keep all this preview code in msg_preview, otherwise we will end up having this render file huge again.

If we do that, then we need to do a require("client/js/socket-events/msg_preview") in this file, which I think we shouldn't do. The code in this function is very much related to the renderer, so I think we should leave it there, and modularize the renderer in a later PR if necessary.
For the click event, however, I'm less happy to move it here, but we have the same need for require otherwise :/

Why exactly do you need it? You already append the preview in template, and you seem to be duplicating it here.

You're probably scared of the socket-events folder, but at least all the related code will be in a single file, instead of being here in a somewhat generic render file.

Previews aren't appended in the template anymore.

I'm not scared of the socket-events folder lol. When refreshing the page, we're not getting the preview as a socket event, it doesn't make sense to load it from there. Everything gets loaded from the chain in render (build channels -> build messages -> etc.), and the rendering of previews is a part of that chain. So definitely makes sense to have it there.

Having it as part of render doesn't stop us from splitting the code.

xPaw · 2017-07-06T06:20:42Z

client/js/render.js

-		data.msg.preview.shown = options.shouldOpenMessagePreview(data.msg.preview.type);
+	if (data.msg.previews.length) {
+		data.msg.previews.forEach((preview) => {
+			preview.shown = options.shouldOpenMessagePreview(preview.type);


wtf, you want to open all previews at once?

What do you mean? I haven't improved/changed the UI one bit, so before that PR, if you were posting a URL and that would show as open, with this PR that same behavior applies for multiple URLs.
One reason that I didn't touch UI is because you are improving it in #1301 and I think we should keep those improvements there.

I'm still debating whether we want the caret right next to the URLs (à la Slack) or not, but to be honest the current UI works fairly well even with multiple URLs. Sure, it's taking a lot of space for nothing, but I'm not saying it's definitive :)

You're changing how many previews are rendered though. So with this PR I assume sending 5 links will render all 5 previews at once? Seems like quite the overkill.

xPaw · 2017-07-06T06:22:28Z

src/plugins/irc-events/link.js

-		if (res === null) {
-			return;
-		}
+	Array.from(new Set( // Remove duplicate links


Could you add proper caching instead of just deduping? As long as lounge process is running, it would keep X recent urls in memory (and block while one request is in flight, like await an existing promise).

I agree with the intent, but this goes way beyond the goal of this PR and the time I can allocate on this. I would rather keep the semi-naive solution I have right now (which is not worse than what we have in master anyway), and we can work on proper caching/deduping in a later PR, IMO.

xPaw · 2017-07-06T06:24:36Z

Is this going to append expand button per new line after the text? You added divs with urls, but I don't see any code changes to move buttons there.

xPaw · 2017-07-06T06:25:41Z

client/views/msg.tpl

@@ -9,8 +9,8 @@
 	</span>
 	<span class="text">
 		{{~{parse text}~}}
-		{{#if preview}}
+		{{#each previews}}
 			{{> msg_preview}}


This doesn't seem to match what you're doing in appends.

Correct, this should just all go, I just forgot.

xPaw · 2017-07-06T06:27:18Z

client/js/libs/handlebars/parse.js

@@ -81,5 +81,6 @@ module.exports = function parse(text) {
 		}

 		return fragments;
-	}).join("");
+	}).join("") +
+	linkParts.map((part) => `<div data-url="${part.link}"></div>`).join("");


part.link is not escaped.

Fixed. Is there something better I could do though?

xPaw · 2017-07-06T06:27:59Z

Add a test with multiple URLs, both for parse and link previews.

astorije · 2017-07-06T06:47:01Z

Is this going to append expand button per new line after the text? You added divs with urls, but I don't see any code changes to move buttons there.

Correct. Not ideal, but not a definitive solution, and I'm leaving UI improvements to your #1301 that we can build upon after this PR lands. I'm also not planning to release this PR without your work in #1301 :)

xPaw · 2017-07-06T06:48:01Z

I'm leaving UI improvements to your #1301 that we can build upon after this PR lands

That PR is unnecessary if buttons are going to appear after links.

astorije · 2017-07-06T06:48:30Z

Add a test with multiple URLs, both for parse and link previews.

What do you mean? I already did add/update tests, no?

xPaw · 2017-07-06T06:49:24Z

I already did add/update tests, no?

There's no test that has multiple <div data-url>

astorije · 2017-07-06T06:49:43Z

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 `<div data-url="${escapedLink}"></div>`;


Actually, escaping this is going to make finding stuff with jQuery difficult (it's matching based on preview.link, which is unescaped). Do we actually need to escape this, here? It's not for show, it's mostly a key.

Do you want a XSS? That's how you're going to get an XSS.

Fair enough 😅 What do you recommend? Because right now rendering of something that needs escaping will fail.

Actually, this is fixed now, yay

astorije · 2017-07-06T06:54:18Z

That PR is unnecessary if buttons are going to appear after links.

Are we set on this idea though? I think it's reasonable to settle on the actual capability of having multiple previews, and then play with different UIs, as yours is already an improvement.

There's no test that has multiple <div data-url>

Will add a test for parse, I believe that's the only missing one, no?

astorije · 2017-07-06T06:57:42Z

Test added.

xPaw · 2017-07-06T22:16:18Z

client/js/render.js

@@ -72,6 +75,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) => {


This can be oneliner

We can't because of this rule: http://eslint.org/docs/rules/brace-style.
Alternative is to do data.msg.previews.forEach((preview) => renderPreview(preview, msg));, which has an implicit return value that we shouldn't convey, so this is actually the best option IMO.

Essentially, the following:

data.msg.previews.forEach((preview) => renderPreview(preview, msg));

translates to:

data.msg.previews.forEach((preview) => { return renderPreview(preview, msg); });

and not:

data.msg.previews.forEach((preview) => { renderPreview(preview, msg); });

That extra return is unwanted.

That extra return is unwanted.

Yeah, but it doesn't cause any problems. I wouldn't have an issue with this, tbh.

xPaw · 2017-07-06T22:18:12Z

client/js/socket-events/msg_preview.js

-	if (bottom) {
-		container.scrollBottom();
-	}
+	data.link = Handlebars.Utils.escapeExpression(data.link);


Are you sure this isn't going to have side effects like double escapes?

In my understanding, it's coming back unescaped from the preview socket events. That's why we also escape when getting the msg event.

But then the msg_preview template is going to escape it again, is it not? Try testing it with & in url or something.

EDIT: I just tried it, works fine.

I just tried with the msdn URL given in test for parse, and yeah it does work fine :)

xPaw · 2017-07-06T22:18:44Z

client/views/msg_preview_toggle.tpl

@@ -0,0 +1 @@
+<button class="toggle-button opened" data-url={{url}}	aria-label="Toggle prefetched media"></button>


Was no quotes intentional?

xPaw · 2017-07-07T13:52:28Z

client/css/style.css

@@ -293,6 +294,16 @@ kbd {
 	content: "\f005"; /* http://fontawesome.io/icon/star/ */
 }

+#chat .toggle-button {
+	/* These 2 directives are directly from .fa-fw */
+	width: 1.28571429em;


I don't think we need such a precise width, the button can be wider to hit it easily.

I blindly copied it from font awesome for consistency sake, and then extra width for easy hit is at button level and not i level.

I'll round it up, double check if width is enough, and make sure height is as big as it can get.

Done. Button is now a bit bigger, and roughly square (takes all height).

xPaw · 2017-07-07T13:55:30Z

client/views/msg_preview_toggle.tpl

@@ -0,0 +1 @@
+<button class="toggle-button opened" data-url="{{url}}" aria-label="Toggle prefetched media"></button>


Pretty sure we are able to change text depending on type, so Toggle image and Toggle link preview?

I can do that most likely yeah.

- 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

…from ellipsis to caret

astorije · 2017-07-08T08:43:31Z

Re: concern of being blocking, I did some tests on the server, using:

───────────────────────────────────
modified: src/plugins/irc-events/link.js
───────────────────────────────────
@ link.js:31 @ module.exports = function(client, chan, msg) {
    ))
        .slice(0, 5) // Only preview the first 5 URLs in message to avoid abuse
        .forEach((link) => {
+           console.log(`Starting ${link}`);
            fetch(link, function(res) {
                if (res === null) {
                    return;
                }

+               console.log(`Finished ${link}`);
+
                parse(msg, link, res, client);
            });
        });

Posting this message (https://cloudhealthtech.com https://thelounge.github.io https://www.facebook.com https://www.google.com https://imgur.com/gallery/51haX) multiple times, I get different orders:

Starting https://cloudhealthtech.com
Starting https://thelounge.github.io
Starting https://www.facebook.com
Starting https://www.google.com
Starting https://imgur.com/gallery/51haX
Finished https://www.google.com
Finished https://www.facebook.com
Finished https://imgur.com/gallery/51haX
Finished https://thelounge.github.io
Finished https://cloudhealthtech.com

Starting https://cloudhealthtech.com
Starting https://thelounge.github.io
Starting https://www.facebook.com
Starting https://www.google.com
Starting https://imgur.com/gallery/51haX
Finished https://thelounge.github.io
Finished https://imgur.com/gallery/51haX
Finished https://www.google.com
Finished https://cloudhealthtech.com
Finished https://www.facebook.com

So unless I'm missing something, it looks fine.

astorije · 2017-07-08T08:44:23Z

@xPaw, I believe I have addressed all your comments, including splitting the rendering code into its own module :))

xPaw · 2017-07-08T08:44:43Z

client/css/style.css

@@ -293,6 +294,16 @@ kbd {
 	content: "\f005"; /* http://fontawesome.io/icon/star/ */
 }

+#chat .toggle-button {
+	/* These 2 directives are loosely taken from .fa-fw */


No longer valid comment

Nah it is, I updated it from directly from to loosely taken from :D Just a reminder to keep an eye on .fw-fa in the future.

xPaw · 2017-07-08T08:46:26Z

client/views/msg_preview_toggle.tpl

+<button class="toggle-button {{#if shown}} opened{{/if}}"
+  data-url="{{link}}"
+  {{#equal type "image"}}
+    aria-label="Toggle image preview"


Is this not going to show up on hover? I don't see tooltipped classes.

No, it was always aria-label, not tooltips 😅
I'm up for making these tooltips, but mind doing that later after merging? I'm going to be fairly busy this weekend, hence me addressing comments at 5am :)

See https://github.com/thelounge/lounge/pull/1303/files#diff-22e42391cd970f0b0b44672abb685a0aL3, and no tooltips in UI right now.

AlMcKinlay

Looks good, really nice change.

AlMcKinlay · 2017-07-09T14:35:34Z

client/js/render.js

@@ -72,6 +75,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) => {


That extra return is unwanted.

Yeah, but it doesn't cause any problems. I wouldn't have an issue with this, tbh.

AlMcKinlay · 2017-07-09T14:48:36Z

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 `<div class="preview" data-url="${escapedLink}"></div>`;


I'm not 100% sure about putting these right next to the link, but I can't think of a better option. It's probably the best we can do when there are multiple previews.

Yeah, alternatively it was messing with jQuery to insert at the right position depending on the order, etc. so this feels more "natural" but I completely feel you. Nothing we can't update if a better solution presents itself anyway

…eviews Support multiple previews per message

xPaw · 2018-10-03T11:56:33Z

For history: This PR broke storage de-referencing because it didn't update the relevant code. Fixed in #2824

astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jul 6, 2017

astorije requested a review from xPaw July 6, 2017 06:17

xPaw reviewed Jul 6, 2017

View reviewed changes

astorije force-pushed the astorije/multiple-previews branch 3 times, most recently from 52184eb to 007569e Compare July 6, 2017 06:40

astorije commented Jul 6, 2017

View reviewed changes

astorije force-pushed the astorije/multiple-previews branch from 007569e to 5c2fd6b Compare July 6, 2017 06:57

astorije force-pushed the astorije/multiple-previews branch 2 times, most recently from c87abd4 to 4ae1629 Compare July 6, 2017 14:20

xPaw added this to the 2.4.0 milestone Jul 6, 2017

xPaw reviewed Jul 6, 2017

View reviewed changes

astorije force-pushed the astorije/multiple-previews branch from 4ae1629 to 4f8114b Compare July 6, 2017 22:30

astorije mentioned this pull request Jul 7, 2017

Add collapse/expand commands to toggle all previews #1309

Merged

astorije force-pushed the astorije/multiple-previews branch from 4f8114b to 432eb62 Compare July 7, 2017 05:06

astorije mentioned this pull request Jul 7, 2017

Better previews #1130

Closed

3 tasks

xPaw reviewed Jul 7, 2017

View reviewed changes

astorije force-pushed the astorije/multiple-previews branch 3 times, most recently from aa5b6a4 to 6810a42 Compare July 8, 2017 08:18

astorije added 2 commits July 8, 2017 04:34

Move preview toggle buttons next to their respective URLs and switch …

2820083

…from ellipsis to caret

astorije force-pushed the astorije/multiple-previews branch from 6810a42 to 2820083 Compare July 8, 2017 08:43

xPaw reviewed Jul 8, 2017

View reviewed changes

xPaw approved these changes Jul 8, 2017

View reviewed changes

AlMcKinlay approved these changes Jul 9, 2017

View reviewed changes

xPaw merged commit 199bc45 into master Jul 9, 2017

xPaw deleted the astorije/multiple-previews branch July 9, 2017 15:29

This was referenced Jul 17, 2017

URL parser adds preview placeholder divs within the part/quit reason #1343

Closed

URLs passed in /me do not prefetch after refresh anymore #1346

Closed

Enforce correct order for previews on server-side prefetch rather than at client parsing #1353

Merged

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#1303 from thelounge/astorije/multiple-pr…

d73e6f7

…eviews Support multiple previews per message

xPaw mentioned this pull request Oct 3, 2018

Fix stored image previews not being dereferenced #2824

Merged

		@@ -0,0 +1 @@
		<button class="toggle-button opened" data-url={{url}} aria-label="Toggle prefetched media"></button>

Support multiple previews per message #1303

Support multiple previews per message #1303

Conversation

astorije commented Jul 6, 2017 • edited Loading

Example of multiple URLs (screenshots show an outdated toggle button)

UI of toggle button

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw commented Jul 6, 2017

astorije commented Jul 6, 2017

xPaw commented Jul 6, 2017

astorije commented Jul 6, 2017

xPaw commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Jul 6, 2017

astorije commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Jul 8, 2017

astorije commented Jul 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlMcKinlay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw commented Oct 3, 2018

astorije commented Jul 6, 2017 •

edited

Loading

xPaw Jul 6, 2017 •

edited

Loading

xPaw Jul 7, 2017 •

edited

Loading