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.

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

Honor Regex match groups #40

Open
willdady opened this issue Dec 8, 2014 · 15 comments
Open

Honor Regex match groups #40

willdady opened this issue Dec 8, 2014 · 15 comments

Comments

@willdady
Copy link

willdady commented Dec 8, 2014

@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
Copy link
Owner

@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
Copy link
Author

willdady commented Dec 9, 2014

@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
Copy link
Owner

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
Copy link
Author

willdady commented Dec 9, 2014

@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
Copy link
Owner

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

@DavidLGoldberg
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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

@DavidLGoldberg
Copy link
Owner

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
Copy link
Author

@DavidLGoldberg PR is here - #41

@DavidLGoldberg
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

@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
Copy link
Owner

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants