improve client message parsing #699

Bonuspunkt · 2016-10-17T04:07:58Z

replaced regexps with parser and processing

fixes #654

xPaw · 2016-10-17T04:41:45Z

Nice! This needs heavy testing. Did you take a look at the previous PR trying to fix this to see why it wasn't merged?

astorije · 2016-10-17T04:44:28Z

This needs heavy testing.

@xPaw, what do you say we come up with a way to write tests for these? Every time we improve the message parsing, we fix 5 things and break 1. I remember @maxpoulin64 had a list of strings to test against once, we should be able to make this part of our mild test suite.
I can look into that when I have a minute but I wouldn't be against some inputs or opinions if you have any.

xPaw · 2016-10-17T04:45:41Z

We do need a test suite for it. Since it's exposed as a separate module, trivial to test.

Bonuspunkt · 2016-10-17T05:43:23Z

just realized, Object.assign is a ES2015 Feature - are you supporting IE11-?
Polyfill or rewrite?

Bonuspunkt · 2016-10-19T19:22:42Z

fixed, should also fix #15 and #199

xPaw · 2016-10-20T04:47:04Z

client/js/libs/handlebars/parse.js

+				});
+			}
+		} else if ((text[position] === "#" || text[position === "&"]) &&
+			// ^-- this is basically wrong, because we need the 005 CHANTYPES response from the connect


The front-end is actually aware of this, as I passed it into client when we moved to irc-fw. The network dom object has it in data.

Bonuspunkt · 2016-10-21T19:12:37Z

i would call it done.
imho there is no sensible way to pass RPL_ISUPPORT.CHANTYPES to the parse function

xPaw · 2016-10-21T19:14:31Z

client/js/libs/handlebars/parse.js

+Handlebars.registerHelper(
+	"parse", function(text) {
+		function createFragment(fragment) {
+			var className = "";


You could make this an array and then call .join on it, this would fix the leading space.

xPaw · 2016-10-21T19:42:29Z

Testing this, I see a bug in how it parses channels.

[color code]#123[reset color]: is being parsed as a channel #123:, but : should not be part of it.

EDIT: Another bug with the parser. #&">bug turns into #&">bug&">bug

Bonuspunkt · 2016-10-21T20:22:02Z

#123: is a valid channel name, you can join me on freenode :)

xPaw · 2016-10-21T20:23:39Z

@Bonuspunkt I understand it's valid, but it's ignoring the [reset color] char code to stop it from continuing channel parser (\x0f is not a valid channel character)

Bonuspunkt · 2016-10-21T20:32:36Z

the behavior is the same as mirc

i think i see the problem here, should

#[color]the[color]lounge

be #the or #thelounge ?

xPaw · 2016-10-21T20:35:02Z

Pretty sure that should not be parsed as a channel at all. I wouldn't really say mIRC is the ideal client to base our parser on.

Bonuspunkt · 2016-10-21T20:49:43Z

same behavior as mIRC in HexChat
AdiIRC seems to strip : but ignores styling chars (#[color]test[color]:! joins #test! )

what is your reference?
also good time to start a discussion if you want to support strip color/styling and how it should behave

xPaw · 2016-10-30T14:09:35Z

Okay for now it might be fine to parse channels with colours, what I'm worried about are the parser bugs producing invalid markup like I showed above.

Bonuspunkt · 2016-10-30T22:24:57Z

oh sorry missed that.
the position was not correctly updated, so #&x would find #&x and next round start at & and find &x
which resulted than in #&x&x

also fixed

xPaw · 2016-11-06T11:45:13Z

Mind looking at (http://example.com) getting ) in the link?

Perhaps it's also possible to optimize the generated code if there are same colours? E.g:
test \x0312#\x0312\x0312\"te\x0312st\x0312\x0312\x0312\x0312\x0312\x0312\x0312\x0312\x0312\x0312\x0312a" generates: test #"testa

If not possible or easy to optimize, it should not at least generate empty tags like 

xPaw · 2016-11-06T13:57:22Z

test/client/js/libs/handlebars/parse.js

@@ -74,6 +74,14 @@ describe("parse Handlebars helper", () => {
 					"https://theos.kyriasis.com/~kyrias/stats/archlinux.html" +
 				"</a>" +
 				"&gt;"
+		}, {
+			input: "(http://example.com)",


Then add a test for links that do have balanced () in them. Like http://example.com/Test_(Page).

xPaw · 2016-11-06T13:59:06Z

client/js/libs/handlebars/parse.js

@@ -102,6 +102,13 @@ function analyseText(text) {
 	// NOTE: channel prefixes should be RPL_ISUPPORT.CHANTYPES
 	var channelPrefixes = ["#", "&"];
 	var punctuations = ["'", "\"", ".", ",", "!", "?", "¿", "<", ">", "(", ")", "{", "}"];
+	var commonProtocols = [


Couple more: mailto, steam, mumble, ts3server, ssh

mailto won't match - url detection is based on ://
never have seen mumble, ts3server urls, are they using ://?

Bonuspunkt · 2016-11-06T18:21:24Z

i'm thinking about replacing the url detection with https://www.npmjs.com/package/autolinker

MaxLeiter · 2016-11-27T21:16:29Z

bump? would love to see this merged

astorije · 2016-12-05T05:36:22Z

client/js/libs/handlebars/parse.js

-);
-
-function uri(text) {
-	return window.URI.withinString(text, function(url) {


I see you are getting rid of URI.withinString which is, I think, our only use of URI.js (in our repo, official repo). Any specific reason for this or just killing a dependency?

This seems to be a very active project, and they've just released a version that fixes one of the bugs I noticed a long time ago.
The latest release packs quite a lot of changes since the version we use, see medialize/URI.js@v1.14.1...v1.18.4.

Do you want to try running your tests against our version of URI.js and the latest one released?

I know we like to kill dependencies when they are tiny, but I think at that point I'd rather not roll our own URL parser.

i need start & end position of the url to correctly merge it with the styling information.
i'll look at it.

Bonuspunkt · 2016-12-05T17:12:19Z

tests are against latest version of URI.js
tried to use the file, but crashed with could not resolve 'punycode'

xPaw · 2016-12-10T20:57:04Z

Really solid job so far! However I am getting an error at the current version when sending a link:

Uncaught TypeError: Cannot read property 'length' of undefined
    at Function.URI.withinString (uri.js:819)
    at findLinks (parse.js:112)
    at fullProcess (parse.js:228)
    at Object.<anonymous> (parse.js:282)
    at Object.main (lounge.templates.js:506)
    at main (handlebars.js:1092)
    at Object.ret [as actions/topic] (handlebars.js:1095)
    at render (lounge.js:56)
    at Object.<anonymous> (lounge.js:71)
    at Object.main (lounge.templates.js:170)

Bonuspunkt · 2016-12-10T23:18:27Z

would have preferred replacing client/js/libs/uri.js with a symlink to node_modules/urijs/src/URI.js but windows
"fix" till #640 lands

xPaw · 2016-12-16T10:23:41Z

I think we will focus on this PR for 2.3.0 after 2.2.0 is shipped. cc @astorije

xPaw · 2016-12-31T09:58:05Z

I'm not a particular fan of moving parsing stuff into a separate repository we have no access to.

Bonuspunkt · 2016-12-31T10:51:53Z

i have no problem with transfer of repo & npm
hit me on irc

xPaw · 2017-01-28T12:19:19Z

@astorije What do you think? Should the code kept in the repo, or have this as a separate project under thelounge?

MaxLeiter · 2017-01-28T19:04:35Z

I dont think it makes much sense to move it out of Lounge unless we remove Lounge specific code (like in the merge function)

kode54 · 2017-01-29T21:59:16Z

This parser mis-parses the following URL:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms644989(v=vs.85).aspx

It drops the period before the aspx from the actual link.
It returns the clickable text range starting from the first character in the line, and ending after the correct number of characters, or possibly correct excluding the above mentioned period.

Bonuspunkt · 2017-01-29T23:03:40Z

medialize/URI.js#325

astorije · 2017-02-12T21:26:45Z

@Bonuspunkt, urijs has been bumped in master. Mind rebasing this PR and see what happens? Do you think this PR is in a reviewable/mergeable state?

Bonuspunkt · 2017-02-12T22:29:48Z

rebased to master, it is reviewable.

astorije · 2017-02-12T22:56:57Z

@astorije What do you think? Should the code kept in the repo, or have this as a separate project under thelounge?

I see @Bonuspunkt did a good job with setting this up: nice README, semver-releases, good test coverage. For these reasons, I'm happy to keep it in a separate repo at least at the moment.
However, I do agree with @xPaw that we would prefer to keep it "in-house". @Bonuspunkt, would you be OK to transfer it under the organization and still be its maintainer? Just like @williamboman is maintaining docker-lounge, etc.
Also, do you mind using the same license than https://github.com/thelounge/lounge, i.e. MIT license?

astorije

I think it's already better than what we have on master (but still a few comments inline).
Also, 👏 👏 on the rigorous test suite!

Any idea how to fix CI?

astorije · 2017-02-12T23:04:05Z

client/js/libs/handlebars/parse.js

+	"irc", "ircs",
+	"svn", "git",
+	"steam", "mumble", "ts3server", "ssh",
+];


Wouldn't it quickly become an issue to whitelist schemes here? I feel like this is a well-known problem that should be handled in one of the libraries we use instead of in our code. Thoughts?

no, this is the improvement for #730 also list was defined by @xPaw

astorije · 2017-02-12T23:04:30Z

client/js/libs/handlebars/parse.js

+	"svn", "git",
+	"steam", "mumble", "ts3server", "ssh",
+];
+const incorrectDetections = ["www"];


So, we don't want to parse www.google.com as a URL? Why is that?

that's the fix for matching www. (also covered in the test line126+)

edit: medialize/URI.js#327

Bonuspunkt · 2017-02-13T16:31:47Z

would you be OK to transfer it under the organization and still be its maintainer?

yes

license

currently it's ISC, MIT seems pretty much the same, so i'm fine with it.

fix CI

node v4 has not enough ES6 support ( http://node.green/ )
could add "use strict" on top of parse.js but ircmessageparser has parameter defaults. that would need transpiling

astorije · 2017-02-14T05:02:54Z

yes
currently it's ISC, MIT seems pretty much the same, so i'm fine with it.

Cool!

could add "use strict" on top of parse.js but ircmessageparser has parameter defaults. that would need transpiling

Could you get rid of the default params instead? It's a small library and transpiling only for this is a bit of a shame IMO :)
I do think it's important we keep Node v4 support in The Lounge as it's sill a LTS for quite a while.

Bonuspunkt · 2017-02-14T20:09:31Z

wait, the code is run by node out of convenience. when the lounge is running, this code is only executed by the webclient.

also it's not worth removing syntactic sugar as i have to readd it.

xPaw · 2017-03-12T10:08:48Z

So yeah, I think keeping all the code in the client would be better for us. As for the polyfill, you've introduced a lot of unnecessary code that we will have to maintain.

Overall, the cleaner solution is just to keep the code here IMO. If you can't be bothered to revert to how it was, we could handle it ourselves (create a new branch in lounge), if you allow it.

I really do want to get this into 2.3 release.

Bonuspunkt · 2017-03-12T23:48:20Z

not sure where in time the "how it was" was, but sure, go ahead. if you need help ping me

edit: very old might be in this branch https://github.com/Bonuspunkt/lounge/tree/messageParserOld

xPaw · 2017-03-18T08:13:55Z

By "how it was" I mean when the parser was contained within the lounge code, and not a separate project.

Bonuspunkt · 2017-03-18T08:31:07Z

it's at https://github.com/Bonuspunkt/lounge/tree/messageParserOld

xPaw · 2017-03-18T09:24:13Z

Closing in favour of #972. I hope to get that PR merged ASAP. @Bonuspunkt, I've used your latest code in the separate package, not messageParserOld branch.

astorije mentioned this pull request Oct 19, 2016

Add a simple (first) test for localetime Handlebars helper #703

Merged

xPaw reviewed Oct 20, 2016

View reviewed changes

xPaw reviewed Oct 21, 2016

View reviewed changes

MaxLeiter mentioned this pull request Nov 2, 2016

URL's getting mapped to whole word segment. #730

Closed

xPaw reviewed Nov 6, 2016

View reviewed changes

astorije requested changes Dec 5, 2016

View reviewed changes

xPaw modified the milestones: 2.2.0, 2.3.0 Dec 16, 2016

xPaw removed this from the 2.2.0 milestone Dec 16, 2016

xPaw mentioned this pull request Dec 20, 2016

URL parser incorrectly escapes characters #199

Closed

improve client message parsing

72c0cd9

astorije requested changes Feb 12, 2017

View reviewed changes

apply fix medialize/URI.js#327

ef4f0f4

Bonuspunkt added 3 commits February 14, 2017 21:23

don't test on node < v6

4b1573c

load polyfill if required

23ab21b

ignore polyfill

fc0c8b0

xPaw mentioned this pull request Mar 18, 2017

New IRC message parser #972

Merged

xPaw closed this Mar 18, 2017

improve client message parsing #699

improve client message parsing #699

Conversation

Bonuspunkt commented Oct 17, 2016

xPaw commented Oct 17, 2016

astorije commented Oct 17, 2016

xPaw commented Oct 17, 2016

Bonuspunkt commented Oct 17, 2016

Bonuspunkt commented Oct 19, 2016

Choose a reason for hiding this comment

Bonuspunkt commented Oct 21, 2016

Choose a reason for hiding this comment

xPaw commented Oct 21, 2016 • edited

Bonuspunkt commented Oct 21, 2016

xPaw commented Oct 21, 2016

Bonuspunkt commented Oct 21, 2016

xPaw commented Oct 21, 2016

Bonuspunkt commented Oct 21, 2016 • edited

xPaw commented Oct 30, 2016

Bonuspunkt commented Oct 30, 2016

xPaw commented Nov 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bonuspunkt Nov 6, 2016 • edited

Choose a reason for hiding this comment

Bonuspunkt commented Nov 6, 2016

MaxLeiter commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bonuspunkt commented Dec 5, 2016

xPaw commented Dec 10, 2016

Bonuspunkt commented Dec 10, 2016

xPaw commented Dec 16, 2016

xPaw commented Dec 31, 2016

Bonuspunkt commented Dec 31, 2016

xPaw commented Jan 28, 2017

MaxLeiter commented Jan 28, 2017

kode54 commented Jan 29, 2017

Bonuspunkt commented Jan 29, 2017

astorije commented Feb 12, 2017

Bonuspunkt commented Feb 12, 2017

astorije commented Feb 12, 2017

astorije left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bonuspunkt Feb 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bonuspunkt Feb 13, 2017 • edited

Choose a reason for hiding this comment

Bonuspunkt commented Feb 13, 2017 • edited

astorije commented Feb 14, 2017

Bonuspunkt commented Feb 14, 2017

xPaw commented Mar 12, 2017 • edited

Bonuspunkt commented Mar 12, 2017 • edited

xPaw commented Mar 18, 2017

Bonuspunkt commented Mar 18, 2017

xPaw commented Mar 18, 2017

xPaw commented Oct 21, 2016 •

edited

Bonuspunkt commented Oct 21, 2016 •

edited

xPaw commented Nov 6, 2016 •

edited

Bonuspunkt Nov 6, 2016 •

edited

Bonuspunkt Feb 13, 2017 •

edited

Bonuspunkt Feb 13, 2017 •

edited

Bonuspunkt commented Feb 13, 2017 •

edited

xPaw commented Mar 12, 2017 •

edited

Bonuspunkt commented Mar 12, 2017 •

edited