DavidLGoldberg / jumpy

The fastest way to jump around files and across visible panes in Atom
https://atom.io/packages/jumpy
MIT License
123 stars 16 forks source link

Honor Regex match groups #40

Open willdady opened 9 years ago

willdady commented 9 years ago

@DavidLGoldberg Is it possible to add the ability to honor match groups within the Regex? I really want to there to be a jump point at the beginning and end of each line, ignoring indentation.

To match against just the first character on each line (ignoring preceding white-space) the regex would be as follows:

^\s*(\S)

The jump point should be added at the position within brackets.

DavidLGoldberg commented 9 years ago

@willdady , good idea. I'm going to be looking at a bunch of this stuff shortly. I want to try and handle the following:

empty newlines beginning end symbols (of length 2) symbols (of length 1 where on their own line)

I'll make an attempt at solving for all of these. I should get some time after work this week to work on Jumpy. This is definitely on my list.

willdady commented 9 years ago

@DavidLGoldberg

I had a crack at it yesterday. Here's the change I made, this starts around line 156 of jumpy-view.coffee

            for lineNumber in [firstVisibleRow...lastVisibleRow]
                lineContents = editor.lineTextForScreenRow(lineNumber)
                if editor.isFoldedAtScreenRow(lineNumber)
                    drawLabels 0
                else
                    while ((word = wordsPattern.exec(lineContents)) != null)
                        if word.length == 1
                            drawLabels word.index
                        else
                            matchStr = word[0]
                            for i in [1...word.length]
                                offset = matchStr.indexOf(word[i])
                                if offset != -1
                                    drawLabels(word.index + offset)

Everything you mentioned is what I'm looking for. I tried tackling the issue of matching against the beginning of each line (ignoring whitespace) and the end of each line.

^\s*(\S)|(\S)$

The problem I'm finding is I haven't figured out how to match against the end of a line. The best I've done is getting the cursor before the last character on the line (where It should be after). I believe the issue is partly to do with the fact that editor.lineTextForScreenRow doesn't return newline characters so we can't just match against \n.

I was thinking one way to solve this was to simply provide checkbox options on the settings screen, 'match line beginning' and 'match line ending'. What do you think?

DavidLGoldberg commented 9 years ago

Didn't go through your code carefully yet, but do you want to submit a pull request? What I can do is merge it in so you get your contrib and then like patch over it of course.. Also, I'll try to make the tests work for it if you're not familiar with those. Anyway, no pressure, just a thought!

BTW, the few times I've made a tweak to the default regex, no one's complained. I thought of extra options for some of this stuff too. So as far as the option you are proposing it should probably be checked by default or even baked into the default regex depending on how we do it, because, for a while, I've been leaning towards the more labels the merrier. I call it higher "accuracy" heh. Less moves to do after you get there (especially for non vim-mode users)

The real trick is they can't collide and overlap of course.

Also, eventually I want to investigate / convert to the new decoration / label stuff they've built.

Also, I hear you on the new lines problem. I'll take a look too (soon)

willdady commented 9 years ago

@DavidLGoldberg The only reason I think explicit options are needed is because I think it's impossible to put the cursor at the end of a line using a regex because of the truncated \n.

I think by default, it should automatically match against the beginning and end of each line as well as blank lines. These could be configurable in the settings but I think they should be on by default. Your drawLabels should de-dupe any existing matches (if it doesn't already).

DavidLGoldberg commented 9 years ago

Hey, so lots of work to be done, with deprecations and cleaning up these tests! I'll try to have this in ASAP!

DavidLGoldberg commented 9 years ago

Update: So, just to be clear, I'm working on fixing this stuff first:

http://blog.atom.io/2014/11/18/avoiding-style-pollution-with-the-shadow-dom.html

They're going to switch to the shadow dom by the default, and jumpy is going to break fast :)

Been working on that. I think I'm going to call the fix + hopefully integration of your stuff 2.0, as it'll behave differently, old styles for it will have "broke" and there will be huge refactoring, I think it warrants 2.0.

DavidLGoldberg commented 9 years ago

FYI. Was prioritizing to release 2.0. Which is like a complete rewrite for new Atom API (path to Atom 1.0) shadow dom etc.. I wanted to include your pull request with this but I'll be playing with this shortly after.

DavidLGoldberg commented 9 years ago

Especially, because they just went live with shadow dom as the default! 2.0 coming soon.

DavidLGoldberg commented 9 years ago

Hi @willdady , things cooled down with the deprecated functions etc, from what I can tell. Can you do me a favor and re submit your pull request (so you get credit / listed as a contributor) by pulling latest and reapplying your changes. Auto merge won't work anymore. Code changed a lot since you did that, but I think your strategy should still work. Still never got around to testing it, but I'd like to ASAP. Definitely the next on the list for jumpy.

willdady commented 9 years ago

@DavidLGoldberg PR is here - https://github.com/DavidLGoldberg/jumpy/pull/41

DavidLGoldberg commented 9 years ago

Hi, Thanks for updating. Just for me to be clear. What's your exact new regex that you're using that I should be testing with?

DavidLGoldberg commented 9 years ago

Oh, and also, when I pull your changes and run the test sweet (control+alt/option+cmd+p) in mac I get 10 errors. They may not be real breaks, so I'll have to investigate. I'll take a look at this tonight. If you get a chance take a look, no pressure :)

DavidLGoldberg commented 9 years ago

So, I checked it out real quick, and without a new regex.. the base functionality is broken (beginning of words and well entire words missing). I'd like to see this as additive if possible.. of course not sure we can do that so easily..

willdady commented 9 years ago

@DavidLGoldberg I just pushed an update. If you open up text_text.md manually and toggle jumpy it looks to be working as expected. I think the expected counts are now just off which is why the tests fail... Also you might need to change the default regex (or tests) because it contains match groups which might also be a problem.

Sorry I'm probably not going to be able to contribute much in the immediate future.

DavidLGoldberg commented 9 years ago

Ok, yeah no problem. I posted on the last commit. I was viewing it in real code, and saw some discrepencies with overlap.. I'll compare it to the test file too. I am toying with the idea of a "one off" (or 2 or 3) code tweaks for this functionality, but I may end up doing it your grouping way (original way I envisioned it too!)

Thanks so much for the great work!