noctuid / lispyville

lispy + evil = lispyville
GNU General Public License v3.0
315 stars 23 forks source link

"atom-end" motions are offset to the right #48

Open yuhan0 opened 6 years ago

yuhan0 commented 6 years ago
;;    Aa  B  Cc
(peanut butter sandwich)

With the cursor on A and the atom-movement theme enabled, pressing "E" and "gE" should bring the cursor to C and A respectively, but it ends up being offset one character to the right (c and a). Also the backward-atom-end function is missing a let-block for orig-pos variable.

It might not be the most elegant solution, but I fixed the behaviour by simply adding (forward-char -1) at the end of the functions and :type inclusive to keep dE, cE working as expected.

 (evil-define-motion lispyville-forward-atom-end (count)
   "Go to the next atom or comment end COUNT times."
+  :type inclusive
   (or count (setq count 1))
+  (forward-char 1)
   (if (< count 0))
...
-          (cl-return))))))
+          (cl-return))))
+    (forward-char -1)))

If this is alright (still haven't figured out how to run the tests properly) I could submit a simple PR :) Thanks!

noctuid commented 6 years ago

Also the backward-atom-end function is missing a let-block for orig-pos variable.

Thanks, I've fixed this.

The text objects and relevant motions are still considered "experimental." Even though this behavior is wrong from an evil standpoint, I've written the atom motions to conform to existing Emacs motions (e.g. forward-word) which makes them more easily usable for text objects. This is the expected behavior for now.

I am working on a library that will greatly improve/aid in the process of writing motions/text objects. Writing the lispyville motions/text objects was partly an exercise in what would be necessary of this library. Unfortunately, there are not yet tests for lispyville's atom motions, and I haven't worked on them more because the library I'm writing will drastically alter the existing code for text objects (reducing the needed code amount by at least half) and automatically make the changes neccessary for evil motions to work as expected (e.g. go to the end of the atom instead of the space after the atom). I'm fine with a temporary fix in the meantime if you want to make a PR, but my guess is that your solution may break the atom text objects. If that's the case, I'd recommend just using your fix locally until I have the time to properly handle this issue.

yuhan0 commented 6 years ago

Ok, I've actually been using this fix for the last couple of days and haven't noticed a problem with atom text objects - was there any particular operation you were expecting to fail? The only thing I had issues with were the numeric repeats ("3daa" / "d3aa") but they weren't working before the edit either.

targets.el looks interesting, but I'll probably hold off until it's available on MELPA :)

In any case I submitted a PR - also addresses a small edge case for the a-function text object at the end of a buffer.

yuhan0 commented 6 years ago

Another related set of behaviours - lispyville-forward/backward-function-end places the cursor to the point "after" the end of the function, emacs-style, whereas lispyville-end-of-defun places it one character before the end (vim-style). Was this also intended behaviour?

noctuid commented 6 years ago

I'll have to double check, but your pr is probably ok. Counts aren't expected to work without targets.el.

The behavior for the function motions is intended for now, or at least I am aware of it. The function motions/text objects are going to be moved entirely out of lispyville at some point.

Given how little free time I have, I expect things.el and targets.el won't be on Melpa for a while. If you are interested in progress or want to help try them out when they are more stable and I'm about to put them on Melpa, you can watch both repositories (or just things.el if you don't really want any other notifications).