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

Add Sentence motion (, ) #441

Closed
t9md opened this issue Oct 20, 2016 · 24 comments
Closed

Add Sentence motion (, ) #441

t9md opened this issue Oct 20, 2016 · 24 comments

Comments

@t9md
Copy link
Owner

t9md commented Oct 20, 2016

Continuation of #439

Originally reported on #67. #67 is for both sentence motion and sentence text-object.

@t9md
Copy link
Owner Author

t9md commented Oct 20, 2016

@bronson
Although I'm not sure I understand original intention of moveToEndOfSentence, I could remove this function and pass spec.
Am I missing something?

t9md added a commit that referenced this issue Oct 20, 2016
@bronson
Copy link
Contributor

bronson commented Oct 20, 2016

You mean moveToEndOfString? Oops, right. That was from before the regex was smart. Glad that hack is not needed anymore, good fixups!

Now that we have sentence motion, how hard is that to turn into a text object?

@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

  • Official reference

http://vimdoc.sourceforge.net/htmldoc/motion.html#sentence

  """
  A sentence is defined as ending at a '.', '!' or '?' followed by either the
  end of a line, or by a space or tab.  Any number of closing ')', ']', '"'
  and ''' characters may appear after the '.', '!' or '?' before the spaces,
  tabs or end of line.  A paragraph and section boundary is also a sentence
  boundary.
  If the 'J' flag is present in 'cpoptions', at least two spaces have to
  follow the punctuation mark; <Tab>s are not recognized as white space.
  The definition of a sentence cannot be changed.

  =>
    - end with ['.', '!', '?']
    - optionally followed by [')', ']', '"', "'"]
    - followed by ['$', ' ', '\t']
    - paragraph boundary is also sentence boundary
    - section boundary is also sentence boundary(???)
  """

@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

#442

t9md added a commit that referenced this issue Oct 21, 2016
@bronson
Copy link
Contributor

bronson commented Oct 21, 2016

Yep, I put that info in a comment in #439. :)

My question is: in vim-mode-plus, how similar are motions and text objects? Once you have a motion, does the text object fall out easily? Or are they fairly separate?

@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

Separated, conceptually its' similar and code is reusable, but at least now I'm basically avoid reusing text-object in motion code or vise versa.

development of vim-mode-plus is open-end process from "adding feature also add mess" then "reducing that mess by overhauling existing code".

If I go too much abstraction or sophistication, overhauling, refactoring would become very difficult.

So treat motion, textobject, operator independently is my strategy at now.

@bronson
Copy link
Contributor

bronson commented Oct 21, 2016

That makes total sense. A sure way to make a project late is to perform a lot of abstraction up front.

@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

Yep, I tried several time in my local experiment, but I learned too much abstraction also restrict me.
Early abstraction make easy-task to very difficult.

So explicitness, separateness, not depending to other part leads less brain confusion.
Less confusion from maintainer perspective is always 1st priority.

t9md added a commit that referenced this issue Oct 21, 2016
@t9md t9md closed this as completed Oct 21, 2016
@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

As a result of try&error I removed original code.
But it's still worth tying to limit scanRange till paragraphBoundary(performance effective? and might be able to reduce code further).
But for now. I'm very tired, was hard to be compatible to pure-Vim than I thought initially.

@bronson
Copy link
Contributor

bronson commented Oct 21, 2016

Oh no, I'm sorry to see these changes. I understand you're trying for 100% vim compatibility, but I think Vim's behavior here is buggy. (and that's probably why it's so hard to match).

Why would the first blank line following a sentence be considered a sentence? Vim's docs don't say anything about this, and it doesn't make intuitive sense to me.

Personally, I liked it better before, where it just matched sentences. I understand why bug-for-bug compatibility would be desirable, though, so I can live with this too.

@bronson
Copy link
Contributor

bronson commented Oct 21, 2016

Actually, would this be a good opportunity for a vim-mode-plus plugin? vim-mode-plus-better-than-vim-sentence-motion?

@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

So you like previous behavior right?

@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

Umm, I understand your point. Yes, pure-vim's behavior is inconsistent between next and previous movement.
It's not only for sentence motion, move-to-word and move-to-previous-word is also inconsistent. That's why I use move-to-smart-word by mapping it to w.

I'll think about that after I finished cleanup and might give some option or another motion to skip blank row after sentences.

@bronson
Copy link
Contributor

bronson commented Oct 21, 2016

Naw, v-m-p already has too many options. :)

Yes, I liked it better before. I don't understand why sentence motion would ever stop on a blank line. This is one place where I think it makes sense to ALMOST do what Vim does, especially if it makes the code simpler.

But I'm happy to go with whatever you choose.

@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

I understand your point and agree with you.
This is important topic for me, but I want to think carefully to breaking compatibility.
I can not treat this sentence motion exceptionally.
I have many other same kind of minor frustration for pure-Vim.
One big one is pure-Vim move cursor after operation like y i p.
Thats why I implement stayOnYank or other stayOn family option.
Personally I want make this option true by default.
I want make incremental search true as default.

But what motivate me to develop vim-mode-plus is many user from pure-Vim to Atom can easily migrate with small frustration.

So I cannot decide easily. But glad to hear that I could know that I'm not only one who think original sentence behavior is not comfortable.

@bronson
Copy link
Contributor

bronson commented Oct 21, 2016

I remember vim-mode said they wanted to be compatible with Vim, but not bug-for-bug compatible. If it simplified the code or fixed an apparent bug, they'd prefer to break with Vim. Otherwise, little inconsistencies would be stuck in our lives forever.

(if I remember the argument correctly)

It sounds like vim-mode-plus wants to adhere much closer to actual Vim behavior? That makes sense too. I agree -- if v-m-p frustrates Vim users, then it's not doing its job.

Judgement calls aren't easy!

@bronson
Copy link
Contributor

bronson commented Oct 21, 2016

Before, you were talking about the SmartWord text object? It seems like that uses /[\w-]+/ instead of Atom's wordRegex? Or is there more to it than that?

@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

I no longer user Atom's native word selecting function.
In inset-mode-editor cursor is between two character. But vim's normal-mode, cursor is on-single-character.
Because of that diff, directly use Atom's native feature sometimes very not intuitive in vim-world.

t9md added a commit that referenced this issue Oct 21, 2016
@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

@bronson
Can you check behavior with following keymap with latest-master?

'atom-text-editor.vim-mode-plus:not(.insert-mode)':
  ')': 'vim-mode-plus:move-to-next-sentence-skip-blank-row'
  '(': 'vim-mode-plus:move-to-previous-sentence-skip-blank-row'

I think this is what you want behavior but want to confirm.

@t9md
Copy link
Owner Author

t9md commented Oct 21, 2016

It sounds like vim-mode-plus wants to adhere much closer to actual Vim behavior? That makes sense too. I agree -- if v-m-p frustrates Vim users, then it's not doing its job.

Not always, it's always depends on situation.
Trying to mimic original pure-Vim behavior sometimes teach me a lot.
It's fun exercise of programming and also it's enforce me to refactor internal architecture to accommodate difficult requirement to support original-pure-vim-behavior.

But what I want is stress-free-editor-for-my-personal-use. in this perspective, compatibility is NOT important.

Postponing BIG decision, until I have enough background, motivation, convince to drop compatibility.

t9md added a commit that referenced this issue Oct 21, 2016
@bronson
Copy link
Contributor

bronson commented Oct 22, 2016

haha, yeah, you probably hit on the best solution. Glad to not have more options.

I'll play with both bindings... And I might submit a delete-bronson's-preferred-sentence-motion PR in the future. Less code always wins, even if it's just slightly less. Agreed, we'll see. Thanks for all the considerations!

@t9md
Copy link
Owner Author

t9md commented Oct 22, 2016

Thanks for checking the behavior, I'll release in a few days.

@bronson
Copy link
Contributor

bronson commented Oct 24, 2016

Gotta say, I really like the skip-blank-row behavior. And your implementation is simple, easy to use, and doesn't bloat the settings dialog. ❤️ 💚 ❤️

@t9md
Copy link
Owner Author

t9md commented Oct 24, 2016

Not sure, I still like your idea to limit-scan-range-to-paragraph-boundary-first.

But original PR have to fixed for case move-to-previous-sentence stuck when there is line-end-with-period(.).

So will re-visit I have time.
Thanks for your feedback.

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