Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Take into account wordboundaries for custom highlighting #1358

Merged
merged 1 commit into from Aug 22, 2017
Merged

Take into account wordboundaries for custom highlighting #1358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions client/js/options.js
View file
Open in desktop
@@ -1,6 +1,7 @@
"use strict";

const $ = require("jquery");
const escapeRegExp = require("lodash/escapeRegExp");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const settings = $("#settings");
const userStyles = $("#user-specified-css");
const storage = require("./localStorage");
Expand Down Expand Up @@ -98,6 +99,15 @@ settings.on("change", "input, select, textarea", function() {
// otherwise, users get notifications for everything
return h !== "";
});
// Construct regex with wordboundary for every highlight item
const highlightsTokens = options.highlights.map(function(h) {
return escapeRegExp(h);
});
if (highlightsTokens && highlightsTokens.length) {
module.exports.highlightsRE = new RegExp("\\b(?:" + highlightsTokens.join("|") + ")\\b", "i");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
module.exports.highlightsRE = null;
}
} else if (name === "showSeconds") {
chat.find(".msg > .time").each(function() {
$(this).text(tz($(this).parent().data("time")));
Expand Down
8 changes: 5 additions & 3 deletions client/js/render.js
View file
Open in desktop
Expand Up @@ -65,9 +65,11 @@ function buildChatMessage(data) {
const chan = chat.find(target);
let template = "msg";

if (!data.msg.highlight && !data.msg.self && (type === "message" || type === "notice") && options.highlights.some(function(h) {
return data.msg.text.toLocaleLowerCase().indexOf(h.toLocaleLowerCase()) > -1;
})) {
// See if any of the custom highlight regexes match
if (!data.msg.highlight && !data.msg.self
&& options.highlightsRE
&& (type === "message" || type === "notice")
&& options.highlightsRE.exec(data.msg.text)) {
data.msg.highlight = true;
}

Expand Down