Refactor link previews #1276

xPaw · 2017-06-26T09:02:25Z

Moves preview content as part of the original message
Renders toggle button text with CSS so it's not selectable and not copyable
Toggle button only appears after getting preview content, not instantly
- This will fix Link preview button shown on invalid links #1173
Fixes auto opening previews in history and page reloads
Fix image previews broken by one of the previous PRs
Add more tests

xPaw · 2017-06-26T20:59:07Z

client/views/msg.tpl

+		{{#if preview}}
+			{{> toggle}}
+		{{/if}}
+	</span>


extra wrong span?

Why is that?

astorije · 2017-06-27T18:18:47Z

Fix image previews broken by one of the previous PRs

Putting this here for the record: it would be really really really great if you could make this its own PR so we could merge that ASAP. I don't think I'll have time to review this this week or for v2.3.3.

Thanks!

astorije · 2017-06-30T03:29:05Z

client/views/toggle.tpl

@@ -1,19 +1,18 @@
-{{#toggle}}


Did you mean to rename this file into msg_preview as well?

astorije · 2017-06-30T03:30:28Z

client/css/style.css

@@ -1110,6 +1106,10 @@ kbd {
 	padding: 0 6px;
 }

+#chat .toggle-button:after {
+	content: "···";


Probably for another PR, but I'd love to change this to a right (closed) / down (opened) caret...

astorije · 2017-06-30T03:31:10Z

client/js/libs/jquery/stickyscroll.js

@@ -37,7 +37,7 @@ import jQuery from "jquery";
 				lastStick = Date.now();
 				this.scrollTop = this.scrollHeight;
 			})
-			.on("msg.sticky", keepToBottom)
+			.on("keepToBottom.sticky", keepToBottom)


I'm definitely missing something here, why does this need to be changed?

Gotta trigger "keep to bottom" logic, but can't call the already used msg event.

astorije · 2017-06-30T03:32:30Z

client/js/render.js

+			(options.links && data.msg.preview.type === "link") ||
+			(options.thumbnails && data.msg.preview.type === "image");
+	}
+


Any chance we can avoid the repetition between this and msg_preview.js?

astorije · 2017-06-30T03:33:25Z

client/js/socket-events/msg.js

@@ -35,7 +35,8 @@ socket.on("msg", function(data) {
 		.trigger("msg", [
 			target,
 			data
-		]);
+		])
+		.trigger("keepToBottom");


Does this mean that anytime a message is received, the channel is jumped to the bottom automatically?

Testing this, it doesn't seem so. But then I'm not 100% why this is for.

Because keepToBottom has logic inside to only "keep to bottom" if it's already at the bottom. Thats's the point of the plugin.

Okay, this goes along with the rename from msg to keepToBottom and I missed that.

astorije · 2017-06-30T03:39:03Z

src/plugins/irc-events/link.js

-		.replace(/\x02|\x1D|\x1F|\x16|\x0F|\x03(?:[0-9]{1,2}(?:,[0-9]{1,2})?)?/g, "")
-		.split(" ")
-		.filter((w) => /^https?:\/\//.test(w));
+	const cleanText = msg.text.replace(/\x02|\x1D|\x1F|\x16|\x0F|\x03(?:[0-9]{1,2}(?:,[0-9]{1,2})?)?/g, "");


Could you add a comment to explain what this regex does, please?

astorije · 2017-06-30T03:39:23Z

src/plugins/irc-events/link.js

 const request = require("request");
 const Helper = require("../../helper");
+const findLinks = require("../../../client/js/libs/handlebars/ircmessageparser/findLinks");


Oooh I like where this is going!

Actually, I just realized (:man_facepalming:) that this server file is loading a client file. Any way we can make this a bit cleaner? Not sure what's the best way, I'm just wondering.

astorije · 2017-06-30T03:42:08Z

src/plugins/irc-events/link.js

-		.split(" ")
-		.filter((w) => /^https?:\/\//.test(w));
+	const cleanText = msg.text.replace(/\x02|\x1D|\x1F|\x16|\x0F|\x03(?:[0-9]{1,2}(?:,[0-9]{1,2})?)?/g, "");
+	const links = findLinks(cleanText).filter((w) => /^https?:\/\//.test(w.link));


I think it would be slick to add a scheme parameters to the findLinks function. Only a single usage for this right now but I can see where this could be useful.

astorije · 2017-06-30T04:00:38Z

src/plugins/irc-events/link.js

-	if (!toggle.head.length) {
-		if (toggle.thumb.length || toggle.body.length) {
-			toggle.head = "Untitled page";
+	if (!preview.head.length && preview.type === "link") {


For the record, this is the fix for that bug introduced by a previous PR: image previews were broken.

astorije · 2017-06-30T04:05:51Z

test/plugins/link.js


 describe("Link plugin", function() {
 	before(function(done) {
 		this.app = util.createWebserver();
+		this.app.get("/favicon.png", function(req, res) {
+			res.sendFile(path.resolve(__dirname, "../../client/img/apple-touch-icon-120x120.png"));


Our prefetcher fetches favicons as a thumbnail?! At https://github.com/thelounge/lounge/pull/1276/files#diff-4805d608cbc31851a7bee1bf4c7e247eR54 I'm only seeing og:image and twitter:image, what am I missing?

This is for image previews, not thumbnails. This is just a test URL that serves an existing image so the image preview passes.

Ah right. I know this is nitpicky, but could you rename to test-image or something? It's kind of confusing at first glance, you think the prefetcher makes use of the favicon or something (which is not so far fetched, hence the confusion).

Eh, do we want to add an unnecessary image to the repo?

No no I meant rename favicon.png to test-image.png or something. I'm guessing the name is just a random placeholder, no?

Ah yea, will do

astorije · 2017-06-30T04:11:03Z

src/plugins/irc-events/link.js

-		toggle.type = "link";
-		toggle.head =
+		preview.type = "link";
+		preview.head =
 			$("meta[property=\"og:title\"]").attr("content")


Is it worth making use of og:site_name too? Like site_name - title

I find that unnecessary noise.

Fair enough, we'll see at usage if that's something that would make sense or not

xPaw · 2017-07-02T15:39:21Z

@astorije Addressed your comments.

EDIT: I need to still call click event handler, as that has a fix for image load and scroll to bottom.

xPaw · 2017-07-02T20:36:03Z

Okay fixed, should be good to go.

astorije · 2017-07-03T05:04:03Z

For the record, when it comes to code coverage, what this PR does:

Before:

Statements   : 20.65% ( 741/3589 )
Branches     : 11.67% ( 196/1680 )
Functions    : 22.42% ( 115/513 )
Lines        : 23.95% ( 735/3069 )

After:

Statements   : 20.93% ( 752/3593 )
Branches     : 12.75% ( 215/1686 )
Functions    : 22.52% ( 116/515 )
Lines        : 24.25% ( 745/3072 )

I was actually expecting more based on the tests in this PR, but I'll take it!

astorije

This works really well, 👏

Merging, with a single review, I know, but it should actually be straightforward, plus we have limited maintainers' availability right now.
We'll find out soon enough if there are hidden bugs or not.

#1276 refactored previews, and in particular [reused our link-in-messages detection](https://github.com/thelounge/lounge/pull/1276/files#diff-4805d608cbc31851a7bee1bf4c7e247eR6) instead of [having a separate logic](https://github.com/thelounge/lounge/pull/1276/files#diff-4805d608cbc31851a7bee1bf4c7e247eL19). Unfortunately, this loads a client library, which requires `urijs`. Since these are built at release time, we were not including this package in the production dependencies, and it is now breaking at install time. This might happen again if we add a client dependency in this file and forget it is also loaded by the server. We could in the future either extract this logic into a shared location, or we could move this logic entirely on the server (or maybe many other options), but in the meantime this will fix the issue in `v2.4.0-rc.1`.

thelounge#1276 refactored previews, and in particular [reused our link-in-messages detection](https://github.com/thelounge/lounge/pull/1276/files#diff-4805d608cbc31851a7bee1bf4c7e247eR6) instead of [having a separate logic](https://github.com/thelounge/lounge/pull/1276/files#diff-4805d608cbc31851a7bee1bf4c7e247eL19). Unfortunately, this loads a client library, which requires `urijs`. Since these are built at release time, we were not including this package in the production dependencies, and it is now breaking at install time. This might happen again if we add a client dependency in this file and forget it is also loaded by the server. We could in the future either extract this logic into a shared location, or we could move this logic entirely on the server (or maybe many other options), but in the meantime this will fix the issue in `v2.4.0-rc.1`.

Refactor link previews

thelounge#1276 refactored previews, and in particular [reused our link-in-messages detection](https://github.com/thelounge/lounge/pull/1276/files#diff-4805d608cbc31851a7bee1bf4c7e247eR6) instead of [having a separate logic](https://github.com/thelounge/lounge/pull/1276/files#diff-4805d608cbc31851a7bee1bf4c7e247eL19). Unfortunately, this loads a client library, which requires `urijs`. Since these are built at release time, we were not including this package in the production dependencies, and it is now breaking at install time. This might happen again if we add a client dependency in this file and forget it is also loaded by the server. We could in the future either extract this logic into a shared location, or we could move this logic entirely on the server (or maybe many other options), but in the meantime this will fix the issue in `v2.4.0-rc.1`.

xPaw added Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. Type: Feature Tickets that describe a desired feature or PRs that add them to the project. labels Jun 26, 2017

xPaw added this to the 2.3.3 milestone Jun 26, 2017

xPaw mentioned this pull request Jun 26, 2017

Move link prefetcher button to toggle template and warn on the client when image is too big #1275

Closed

xPaw force-pushed the xpaw/refactor-toggle branch from 617e1b2 to cd157bd Compare June 26, 2017 09:27

xPaw commented Jun 26, 2017

View reviewed changes

xPaw force-pushed the xpaw/refactor-toggle branch from cd157bd to 8a9e72c Compare June 27, 2017 07:05

astorije reviewed Jun 30, 2017

View reviewed changes

xPaw force-pushed the xpaw/refactor-toggle branch from b62a855 to 9f84611 Compare July 2, 2017 15:37

xPaw force-pushed the xpaw/refactor-toggle branch from 9f84611 to 718b87e Compare July 2, 2017 20:35

Refactor link previews

14b2ad7

xPaw force-pushed the xpaw/refactor-toggle branch from 718b87e to 14b2ad7 Compare July 2, 2017 21:41

astorije approved these changes Jul 3, 2017

View reviewed changes

astorije merged commit 622c91b into master Jul 3, 2017

astorije deleted the xpaw/refactor-toggle branch July 3, 2017 06:33

astorije mentioned this pull request Jul 27, 2017

Move urijs from devDependencies to dependencies #1378

Merged

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

Merge pull request thelounge#1276 from thelounge/xpaw/refactor-toggle

8ae6b0c

Refactor link previews

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor link previews #1276

Refactor link previews #1276

xPaw commented Jun 26, 2017 •

edited

xPaw Jun 26, 2017

astorije Jun 27, 2017

astorije commented Jun 27, 2017

astorije Jun 30, 2017

xPaw Jul 2, 2017

astorije Jun 30, 2017

astorije Jun 30, 2017

xPaw Jun 30, 2017

astorije Jun 30, 2017

astorije Jun 30, 2017

astorije Jun 30, 2017

xPaw Jun 30, 2017

astorije Jul 2, 2017

astorije Jun 30, 2017

astorije Jun 30, 2017

astorije Jul 6, 2017

astorije Jun 30, 2017

astorije Jun 30, 2017

astorije Jun 30, 2017

xPaw Jun 30, 2017

astorije Jul 2, 2017

xPaw Jul 2, 2017

astorije Jul 2, 2017

xPaw Jul 2, 2017

astorije Jun 30, 2017

xPaw Jun 30, 2017

astorije Jul 2, 2017

xPaw commented Jul 2, 2017 •

edited

xPaw commented Jul 2, 2017

astorije commented Jul 3, 2017

astorije left a comment

Refactor link previews #1276

Refactor link previews #1276

Conversation

xPaw commented Jun 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Jun 27, 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

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

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 2, 2017 • edited

xPaw commented Jul 2, 2017

astorije commented Jul 3, 2017

astorije left a comment

Choose a reason for hiding this comment

xPaw commented Jun 26, 2017 •

edited

xPaw commented Jul 2, 2017 •

edited