Take into account wordboundaries for custom highlighting #1358

starquake · 2017-07-21T08:37:09Z

Fixes #719

When you have apple as a keyword is does highlight:

Take a look at this magnificent apple!
I wouldn't call that an "apple"
Apple>Microsoft

But it doesn't highlight:

apples applepie grappler

Or if it's sq in my case it does highlight:

Hey sq!

But it doesn't highlight:

That guy is rather grotesque

xPaw · 2017-07-21T08:40:31Z

client/js/options.js

@@ -1,6 +1,7 @@
 "use strict";

 const $ = require("jquery");
+const _ = require("lodash");


You just included a very huge library (we don't use lodash on client)

How do you suggest to fix that? Duplicate the functionality? Or can I require("lodash.escaperegexp") ?

If requiring lodash.escaperegexp does not bring half of lodash with it, then that can work, otherwise just copy the function (it's a oneliner anyway).

Well if I want to require lodash.escaperegexp I need to install it and add it to packages.json as a dependency. Still okay? Or do you prefer copy pasting?

Have you tried lodash/escaperegexp?

Ah I suspected something like that should exist, was looking for it. Thanks will do that.

xPaw · 2017-07-21T08:41:02Z

client/js/options.js

@@ -91,6 +93,9 @@ settings.on("change", "input, select, textarea", function() {
 			// otherwise, users get notifications for everything
 			return h !== "";
 		});
+		options.highlightsRE = options.highlights.map(function (h) {


This can be compiled into a single regex like \b(?:one|two|three)\b.

xPaw · 2017-07-23T08:37:53Z

client/js/options.js

@@ -26,6 +27,7 @@ const options = $.extend({
 	thumbnails: true,
 	userStyles: userStyles.text(),
 	highlights: [],
+	highlightsRE: [],


This should be an empty regex or something because options.highlightsRE.exec may fail.

xPaw · 2017-07-23T08:38:29Z

client/js/options.js

+		var highlightsTokens = options.highlights.map(function(h) {
+			return escapeRegExp(h);
+		});
+		options.highlightsRE = new RegExp("\\b(?:" + highlightsTokens.join("|") + ")\\b", "i");


What will this do if highlightsTokens is empty (e.g. when clearing out highlights in settings)

starquake · 2017-07-26T21:38:58Z

Thanks for the feedback. Shouldn't commit stuff on fridays... Will fix.

astorije · 2017-08-03T00:26:51Z

What's the status of this PR? @starquake, is it ready for review? @xPaw, what do you think of the feature change overall?

starquake · 2017-08-03T05:03:55Z

@astorije No, I need to fix the things you mentioned. But suddenly I've been swamped with some other stuff. Will pick this up as soon when it all dies down a bit.

starquake · 2017-08-04T16:25:24Z

I have incorporated your comments.

xPaw · 2017-08-08T06:58:53Z

client/js/options.js

@@ -1,6 +1,7 @@
 "use strict";

 const $ = require("jquery");
+const escapeRegExp = require("lodash/escapeRegExp");


Before we merge, gotta test if this actually only brings the relevant function, or half of lodash with it.

no require: ➜ lounge git:(highlight-wordboundary) ✗ npm run-script build ➜ lounge git:(highlight-wordboundary) ✗ du client/js/bundle*.js 360 client/js/bundle.js 2496 client/js/bundle.vendor.js const escapeRegExp = require("lodash/escapeRegExp"); ➜ lounge git:(highlight-wordboundary) ✗ npm run-script build ➜ lounge git:(highlight-wordboundary) ✗ du client/js/bundle*.js 360 client/js/bundle.js 2496 client/js/bundle.vendor.js const escapeRegExp = require("lodash").escapeRegExp; ➜ lounge git:(highlight-wordboundary) ✗ npm run-script build ➜ lounge git:(highlight-wordboundary) ✗ du client/js/bundle*.js 360 client/js/bundle.js 3552 client/js/bundle.vendor.js

It adds nothing to the size because client/js/libs/handlebars/ircmessageparser/findChannels.js already has a reference to it.

xPaw · 2017-08-08T06:59:47Z

client/js/options.js

@@ -91,6 +93,13 @@ settings.on("change", "input, select, textarea", function() {
 			// otherwise, users get notifications for everything
 			return h !== "";
 		});
+		// Construct regex with wordboundary for every highlight item
+		var highlightsTokens = options.highlights.map(function(h) {


There's already a map with trim in it, you could probably throw escapeRegExp into it instead of iterating twice.

You mean something like this?

options.highlights = highlightString.split(",").map(function(h) { return escapeRegExp(h.trim()); })

Cause that would mean the contents of options.highlight will be escaped. I was afraid that might break other stuff that expected it to not be escaped.

Oh you are right, nevermind!

Sorry to be a pain for nitpicks like this, but could you make this a bit more ES6-like, please?

const highlightsTokens = options.highlights.map((h) => escapeRegExp(h));

@astorije No problem, it's nice to be on the other side for once

xPaw · 2017-08-08T07:00:35Z

client/js/options.js

+		});
+		if (highlightsTokens && highlightsTokens.length) {
+			options.highlightsRE = new RegExp("\\b(?:" + highlightsTokens.join("|") + ")\\b", "i");
+		}


This needs an else statement to reset highlightsRE to null.

xPaw · 2017-08-08T07:00:59Z

client/js/render.js

+	// See if any of the custom highlight regexes match
+	if (!data.msg.highlight && !data.msg.self
+		&& (type === "message" || type === "notice")
+		&& options.highlightsRE


I would move this higher to avoid type string checks.

Do you mean the order of the checks, or the entire if statement?

Order of checks.

Like this?

if (!data.msg.highlight && !data.msg.self && options.highlightsRE && (type === "message" || type === "notice") && options.highlightsRE.exec(data.msg.text)) { data.msg.highlight = true; }

Are string checks more expensive then checking if something is true? Or do you have some other reason?

It sure is cheaper to check a boolean. In this particular case it's an easy change, so no reason not to do it.

But options.highlightsRE is not a boolean. It's a regex. Or is that irrelevant?

It's only checked against not being undefined, so that's not a problem.

starquake · 2017-08-08T14:06:08Z

Incorporated your comments!

astorije · 2017-08-17T04:47:20Z

client/js/options.js

@@ -15,6 +16,7 @@ const options = {
 	coloredNicks: true,
 	desktopNotifications: false,
 	highlights: [],
+	highlightsRE: null,


Do we actually want to store this as part of localStorage?

Yeah, I guess it has to be assigned directly to module.exports.

@starquake, would you mind doing that? :) Thanks!

@astorije Will do!

astorije · 2017-08-17T04:49:42Z

@starquake, could you add a few examples to your PR description to mention what this detect / doesn't detect, compared to before your PR? Thanks!

starquake · 2017-08-17T06:26:38Z

@astorije Added some examples.

starquake · 2017-08-18T12:30:56Z

@astorije Incorporated your comments. I'm not that familiar with module.exports so I hope this is okay like this. Let me know!

starquake · 2017-08-18T14:21:26Z

client/js/options.js

+			return escapeRegExp(h);
+		});
+		if (highlightsTokens && highlightsTokens.length) {
+			module.exports.highlightsRE = new RegExp("\\b(?:" + highlightsTokens.join("|") + ")\\b", "i");


Not sure if referencing module.exports here is okay. If not, let me know!

I wonder if doing module.exports puts it into localStorage regardless, as it may be referencing the same object..

That's a great point, it probably does. I was wondering why shouldOpenMessagePreview would not be stored somehow, but it turns out functions are not serialized by JSON.stringify:

> JSON.stringify({ foo: 'bar', test() { return 'foo'; }, regex: new RegExp("test") }) '{"foo":"bar","regex":{}}'

Kinda sucks that we do module.exports = options; but then also use this file to do other things than just dealing with options directly.

@xPaw, what do you think?
I can think of a few ideas: wrapping options in a property module.exports.whatever = options; (requires editing of every place that has require("[...]/options"), creating a optionsHelper.js file, making highlightsRE a function instead as they're not serialized (that would only delay the issue instead of fixing it though), but none of those things are ideal. Open to ideas.

Exporting under a separate object is probably the cleanest way of doing it. Or making a getOption function.

I have also found other unintentional variable that would be stored: https://github.com/thelounge/lounge/blob/0b85582744be6671c96c185866171798af4eead9/client/js/sorting.js#L33

starquake · 2017-08-19T16:45:24Z

This pull request fixes a bug. Shouldn't the other problem be handled in a separate pull request. This problem existed before this PR and could maybe be allowed to still exist after it?

astorije · 2017-08-19T21:05:17Z

@starquake, sure, but that means this PR is blocked until this other bug gets fixed. We don't want to pollute people's localStorage :/
@xPaw, do you want to take a look at it? Or someone else?

starquake · 2017-08-21T10:21:26Z

@astorije Well in that case I think this PR should be blocked until the issue is fixed. I mean, I would be fine with that.

xPaw · 2017-08-22T08:11:47Z

@astorije @starquake I don't think we need to block this PR for the fix, as the regex won't end up being saved as stringifying a regex into json does nothing.

starquake · 2017-08-22T08:15:26Z

Oooh so I got lucky!

astorije · 2017-08-22T13:35:32Z

Okay, but let's not pollute localStorage any further then. As in since this PR is ready, that's fine, but let's fix the underlying issue before potentially adding another one :)

astorije

Great job, @starquake!

Take into account wordboundaries for custom highlighting

astorije · 2018-04-28T02:06:53Z

Hey @starquake, we have sticker packs for our contributors now!
If you're interested, go fill the form at https://goo.gl/forms/f5usqAEp5DWoeXC92 🙂

xPaw reviewed Jul 21, 2017

View reviewed changes

xPaw added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Jul 21, 2017

xPaw reviewed Jul 23, 2017

View reviewed changes

xPaw reviewed Aug 8, 2017

View reviewed changes

xPaw approved these changes Aug 8, 2017

View reviewed changes

astorije reviewed Aug 17, 2017

View reviewed changes

Take into account wordboundaries for custom highlighting

43cc279

starquake commented Aug 18, 2017

View reviewed changes

astorije approved these changes Aug 22, 2017

View reviewed changes

astorije merged commit 3b79a3d into thelounge:master Aug 22, 2017

AlMcKinlay added this to the 2.5.0 milestone Aug 22, 2017

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

Merge pull request thelounge#1358 from starquake/highlight-wordboundary

3ab6630

Take into account wordboundaries for custom highlighting

Take into account wordboundaries for custom highlighting #1358

Take into account wordboundaries for custom highlighting #1358

Conversation

starquake commented Jul 21, 2017 • edited

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

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

starquake commented Jul 26, 2017

astorije commented Aug 3, 2017

starquake commented Aug 3, 2017

starquake commented Aug 4, 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

xPaw Aug 8, 2017 • edited

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

starquake commented Aug 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

astorije commented Aug 17, 2017

starquake commented Aug 17, 2017

starquake commented Aug 18, 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

starquake commented Aug 19, 2017

astorije commented Aug 19, 2017

starquake commented Aug 21, 2017

xPaw commented Aug 22, 2017

starquake commented Aug 22, 2017

astorije commented Aug 22, 2017

astorije left a comment

Choose a reason for hiding this comment

astorije commented Apr 28, 2018

starquake commented Jul 21, 2017 •

edited

xPaw Jul 21, 2017 •

edited

xPaw Aug 8, 2017 •

edited