Discussing PR for performance improvement, better support for many labels, vim-mode-plus integration and others

[rixo]

Hi,

I've been hacking around your wonderful package recently, and I've got some pretty interesting results I'd like to contribute back, if possible.

I started by customizing the regex because I wanted to match start of word, end of word, camel bumps, start of line, end of line, symbols, empty lines... Basically everywhere. I ended up with the following monster:

matchPattern: "[\\w]\\b\\s*|\\S\\s+|\\b[\\S]|.$|&+|[=!]?={1,2}|[!|?_.@$#*\\^\"'`{}<>()\\[\\]/]|[A-Z](?![A-Z])|^s*$"

Don't try it in the current version, it will loop infinitely if ^s*$ matches an empty line. I had to hack something in here.

Anyway, the regex gives me satisfaction, but there are overlapping labels. So, I added some CSS classes to be able to hide the first character that was matched, in order to see the 2nd char for the overlapped labels.

Also, I prefer to always have labels in uppercase for better visibility. So I added another class to be able to style uppercase chars so I can differentiate them (I made them red).

Unfortunately, so many labels began to noticeably drag performance #110 ... I identified the bottleneck to be Atom's markers rendering. So I changed strategy to direct DOM manipulation, for a 3-4x gain from finger to eye :)

The reason I wanted such a fine control over the landing point in the first place was for smooth integration with vim-mode-plus. So I've added some interop by turning jumps into a vim-mode-plus motion, so I can yank to jump, or d to jump directly.

Having familiarized myself with the code base, I took the opportunity to enable Jumpy for the settings view because I was quite frustrated with keyboard navigation in settings since I switched to Atom. And I also enabled Jumpy for the tree view, because it was easy at this point.

Finally, I've added an alternating left/right hand label generation strategy (on a best effort basis, considering I usually have too much labels to spare using all possible combos), and smart case match -- case insensitive matching if there's no ambiguity, for when I forget caps lock.

The problem is all this required rewrite of significant portions of code, and all parts have became quite interdependent, so I won't be able to PR features individually. Also, I've not always been ultra respectful of the existing coding style or the TS semantics... And I broke your tests too, for now. Maybe my changes have a chance of being breaking to some user's customisations too (dunno about that).

That's why I'd like to discuss how best to proceed with you. So I'd be glad if you could have a look at my fork (branch rixo) and tell me if you'd be interested in collaborating in order to integrate my changes back into Jumpy.

Cheers.

[DavidLGoldberg]

This is all amazing and a lot of stuff I had on my lists :)

I just made a commit to Travis file last night but somehow missed this last night... I'll respond again tonight in more detail.

I knew the marker stuff was a little bit of a bottleneck... But not using that runs some risk of being brittle with atom upgrades... Jumpy started out of course without them :p

Oddly I had accidentally been using an older version of atom for a little bit and when I upgraded.. Everything was faster including jumpy... Have to still look @ what commit did that and how... Make sure you upgrade heh...

I was just thinking it'd be cool if there was someone out there who I could give commit license to to help me maintain it
.. Maybe you? Heh

I was wondering if we can get on a Google hangout sometime or something? Lots to discuss maybe the gitter? Does that still work haven't tried in a while... Could make a slack too... Anyway I have a whole branch that involves elm and has a much much better architecture.... But I need to fix the tests...unfortunately I find the tests very helpful to sniff out breaks from the beta build... So we need those... I would love to incorporate your changes on top of my new release coming soon... I can do some of the leg work merging too... Maybe we will take out the decorations we'll see... One of the reasons for the new branch is to have a shared architecture with... Editor du jour... So like... Vs code... (I want to make a version for that... The jumpy clone there doesn't seem so great... But people love it heh )

[DavidLGoldberg]

Also, I just noticed, you didn't make a PR right? Feel free!

[DavidLGoldberg]

What are your thoughts on:

#115

I finally pushed up the commit. It uses a state machine file that's written in Elm, but it compiles to a JS, that could be used either in Atom, or in theory in VS Code.

The surrounding code around the state machine is I think a lot easier to understand, so basically the editor specific stuff, should live ideally in the labelers or .....at least in the *.ts files.

The only thing missing in this branch, and why it didn't go months ago, was that the spec tests are broken. Last I checked I had thought it was all because of async nature. I have to verify that some more. I haven't looked at it in a few months :(

I wish I had finished it so your work could all be on top (and maybe less work, as the code is a bit better clearer IMO).

I took a first pass at your fork. Looks like some amazing stuff.

Of course, I'd like to take it piece by piece if possible, and tests are a must.

I'm not a big fan of not just using DOM directly. I used to....and it was brittle with Atom changes. Interestingly things are really picking up on the speed front with the new release. Can you try #master (or this new branch) and tell me how the speed compares to your stuff.

BTW, I have a few threads somewhere where a few contribs and I decided that a lot of the patterns for beginning and end of line would just best be handled outside regex. Did you do a mixed approach? I didn't get to dig through that.

BTW, why Elm? I prefer it to JavaScript/Coffeescript or even Typescript. VS Code seemed to be mostly all TypeScript so I went with that since I prefer it to JS anyway. The JS interop stuff with Elm, actually seemed to lend itself really nicely to Jumpy IMO (clarity of code).

So before I can launch this I have to get the tests working! Then I was planning on building the Treeview... now at the very least I can use yours and throw you on as a "co-authored-by:" in the commit..... but anyway let's cross that bridge when we get there.

[DavidLGoldberg]

Also, to be clear (as you may have figured from the readme) I've always used vim-mode ...and now vim-mode-plus. I didn't dive too far into how your new integration works (a screen share / gif) would be cool!

[DavidLGoldberg]

Oh, and I invited you to the gitter chat group (linked from readme also)

[rixo]

Hey, nice to see you're excited about this!

Sorry for the long delay to give an answer, I've had to process all that...

I've joined the gitter. Hope to meet you there one day. Meanwhile, async communication is cool too.

Now, demo time! So you'll better see what I mean.

vim-mode-plus integration, first visual mode:

And support for jumpy as a vim motion:

And here's a more general view, with labels-a-plenty, tree & settings views:

What are your thoughts on: #115

So, you're telling me you intend on replacing the state machine part with Elm? Hmf. I'm sorry to say I'm not too sure. I'm afraid that will complicate contributing, and restrain users' ability to customize the package for themselves, as I have been able to do myself. Even TS set me a little back, initially. Elm would have stopped me right in my tracks. Don't you think it would be possible to achieve the same benefits without the burden of adding a whole new language to the mix?

Maybe rewriting the state machine to use the last version of the lib, and cleanly abstract the state/workflow logic from editor specific stuffs? Maybe I can give this a try, to see if I can bring it somewhere interesting.

I'm not a big fan of not just using DOM directly.

Eh eh, I've never had my code broken by Atom's changes yet. I'm probably too new to this game.

Anyway, for now at least, I'm willing to pay the price to maintain compatibility in order to support optimal performance in daily usage.

I've done a bit of benchmarking by placing a timer around the activate transition. After warm up (v8 optimizer I guess), in the long_text fixture file (that is, maximum possible labels), I get ~60ms with markers and ~30ms with DOM manipulations. And at this point, DOM labels are visible on the screen whereas Atom's markers are yet to be displayed, and they only appear after a very noticeable delay (I didn't find a easy way to objectively measure that though).

These durations might seem so small as to be irrelevant, but I know I first felt the pain while doing some real work on some real files. That's why I felt compelled to look into it in the first place. I'm using Jumpy all the time, so reaction time is crucial to me.

Also, I don't think Atom's markers really cater to our use case. They're intended to facilitate adding decorations that follow changes and moves of the buffer. But in our case, once Jumpy is activated, the user absolutely can't access the rest of the UI, so they can't change or move the buffer -- well, to be rigorously precise, maybe some automatic code generation, or some external changes, could change the buffer on its own; but I'm calling that a very edgy case.

Moreover, Atom's markers are only good for editor labels. And only for Atom. For other editors, or for other types of view, we'll have to rely on another solution. Absolute positioning is simple, works for everything, and we won't get any faster than that. So it gets my vote.

That being said, the label rendering strategy can very well be made pluggable, which would also make sense if you want to support multiple editors with the same code base. We could have an option to chose between DOM labels to favor performance, and Atom's markers to favor stability. I have already extracted most of the interface, I think (I needed that for DRY, and caching).

I decided that a lot of the patterns for beginning and end of line would just best be handled outside regex. Did you do a mixed approach?

I tried a mixed approach to circumvent the lack of support for regex lookbehind in Electron (yet). But I eventually managed to get a pure regex solution working. I just had to add some guards to prevent the regex from looping infinitely on empty matches.

The pure regex approach is good because it is blazing fast, and it offer easy support for advanced user customization.

I broke my regex down and assemble it with code in order to keep it somewhat maintainable. I was thinking of offering some hard coded pre-configured regexes like this one as an option to the user, to cater to frequent use cases.

Also, I just noticed, you didn't make a PR right? Feel free!

I'm not too sure how best to proceed yet. Let's discuss what we want to do and where we want to go first. Meanwhile my code / changes are visible in my branch.

[DavidLGoldberg]

Hi, just letting you know...totally going to reply to this... Need to write it all up on Tuesday night (6/26).

One thing that was interesting is....the line mode visual used to work...I just checked..it still kind of works (kind of... you have to move one more arrow after jumping left or right..that was some weird change at some point I believe)...but yeah better to have it work grabbing the full line for sure! Anyway, I'll respond more fully tomorrow night!

[DavidLGoldberg]

So sorry for the late reply, haven't had any time afterwork. (I don't work on this during the work day of course)

TBH I am pretty into the Elm, yeah kind of want that release to be the next thing :)

If I had it my way, it'd all be Elm, in fact...one of the reasons I didn't do MORE Elm for this was to keep with decorations as opposed to custom DOM...

What I like most about how I did the elm was the separation of concerns... I like how using "javascript as a service" via "ports" turned out... a little scaffold for clarity maintenance. I don't think most changes will be in the elm... it's kind of a locked state machine that any of the editors should be able to use in theory. Yes i'm sure there is a way to package up a module using JSM (old or new version) ...but the port stuff etc would have to be wired up...and even then I don't think it would compare to the clarity / robustness of the elm.

I think the majority of your work had nothing to do with the state machine stuff right? A lot is in the instances of the labels ...what did I call them labelers or something? Those files will probably have to be editor specific. Although I do see your point ...where if we use DOM..we could even share more of that across the editors... Like I said, I actually started with dom... I even had the equivalence of a "layer" (container) at one point for them all...The atom devs (github) that I've discussed Jumpy with (during certain releases perf issues) thought I should be able to keep it as decorations, that they were in theory supposed to handle a bunch.

I also had the same issue of testing the exact timing of the rendering as you did as you mentioned in one of your comments somewhere. That's frustrating for sure, I can't remember if I ever came up with a REALLY good way to do that...

I would be more willing to rethink the DOM stuff if we added some new tests for that and or I had you as another emergency contributor to Jumpy.

I do think there are a few more low hanging fruit we can do to bring some of your stuff into smaller PR's first right?

We should maybe start a series of issues / pr's milestones for how to end up incorporating most of yours but a bit slower than just commit your whole fork as is :)

Ultimately (at least for the moment) I have to maintain it so obviously I have my opinions / history with this thing (4 years I think).

I think the elm shouldn't pose issues... did you look at the new code...that is the new TS..I don't think you have to even really read the elm..it's ports do what you would expect them to do mostly...if not I should tweak it maybe..

It sure would be great to get any help on some of my next steps:

• What the heck is going on with travis CI failing Jumpy, when tests work locally?

  • Sometimes I get some weird encoding / diacritical issue on the atom beta build (you use that btw too right?) ...but that doesn't explain why I've been getting failures on the stable branch in Travis as well.. I'm using the suggested travis build.. what in my tests are going wrong..need to investigate

• New elm branch / pr ...those tests just don't work at all
  -- It's timing issues... I tried doing promises for commands (new feature to Atom actually, that wasn't around the first time I attacked it!) but didn't have any success yet.

The sooner I get done with that last one ^ ...I could push my branch...and begin to start working on your stuff...also, it might fix the first travis issue from above.

BTW I would really love to get jumpy to work on search results....

I'm not a fan of vim motion style stages.... I have to take a look at that.....but the whole point of jumpy was to be easier cognitively ....like easier than "easy motion" etc... just label and jump...type 2 big deal.. i hide the unavailable after first ones... the graying out of first character is ok i know vimium does it.... not sure how important it is but heck if ya did it.....sweet! I didn't give that effort previously for what I thought was added complexity for little reward....and potentially slower rendering / response times.. But yeah I'd be cool incorporating that in. There's the whole "least surprise" thing..so I think graying is better than removing the letter.

[GoldbergGLG]

FYI. I just checked and my travis is passing now after a restart ..it has all 4 (stable / beta X linux / mac) for the current build. So it's painfully non deterministic all of a sudden.... Again locally I can't get them to fail.

[GoldbergGLG]

I'll have a bunch of time after work this coming week!

[rixo]

Hi! I'm sorry, but I was really not psyched by the idea of messing with Elm, and on the other hand I was quite excited to play with state machines and all... I got really inspired by your code! So I've pushed with my fork to support the use cases and performance I wanted, and I will be using that for the time being.

I completely buy you on jumpy's lower cognitive charge. That's precisely why I want it to be able to go anywhere. And also why I'd like it to act as a motion that can be combined with other vim commands.

It's not the first character that I'm graying out, but the second one, or rather the one that's not currently "sensible". This is to address overlapping labels. I've got a lot of them with my regex... The fading allows to better see if 2 consecutive chars are part of the same label, or the first char of 2 distinct overlapping labels. The first char (i.e. already matched), I completely hide it, otherwise I wouldn't be able to see the 2nd char for labels that are overlapped). In fact, that's the very first thing that made me look into jumpy's code. (Second came performance.)

BTW have you thought of a solution for supporting non-editor views (e.g. search results), if not with a custom DOM layer?