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

Recursive stackop #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Recursive stackop #15

wants to merge 2 commits into from

Conversation

snoe
Copy link

@snoe snoe commented Apr 29, 2015

PR for other half of #10.

This change allows you to capture the next (or previous) element into the sibling form containing your cursor. Another way to say it is that capture will recurse up ancestor forms until it finds an element to capture and moves it into that ancestor.

See the issue for more details.

Cheers

Copy link
Collaborator

@bpstahlman bpstahlman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snoe I like the general approach, but I'm wondering whether there might also be a need for a second flavor of emit/capture: one that always keeps cursor with the current list, and treats adjacent elements selected by a count as a single unit. This capture variant would attempt to pull [count] elements into the current list, even when doing so causes one or more captured elements to cross multiple brackets.

Example: Applying capture with [count]=2
(((((a b |c))) d) e) => (((((a b |c d e)))))
Note how the d had to cross 3 close brackets, while the e crossed 4.

The other emit variant would attempt to eject [count] elements from the current list, regardless of where cursor position starts relative to the elements being ejected.

Example: Applying emit with [count]=3
(((a b c d |e f))) => (((a b |c) d e f))
Contrast: The emit command implemented on this PR performs the following transformation:
(((a b c d |e f))) => (((a b c d) |e) f)
But when the cursor starts on the d instead of the e, the result is identical to what would be produced by the original emit implementation, except that the cursor escapes the list in the PR implementation.
(((a b c |d e f))) => (((a b c) |d e f))
In other words... The behavior of emit in the PR implementation is highly dependent upon initial cursor position. This may sometimes be what you want, but there may also be situations in which you simply want to eject [count] elements from the current list, without first having to re-position the cursor on an element that's not about to be ejected.

Brainstorming... One way to support both use cases would be to provide 2 distinct sets of emit/capture commands: e.g., emit/capture vs barf/slurp or some such... Another possibility would be to consider whether cursor starts on a bracket or on an element within the list:

  • Cursor on open or close bracket: emit/capture pushes/pulls [count] elements out of/into current list without moving cursor off the bracket
  • Cursor on element within list: emit/capture works as implemented in this PR

There's actually a certain intuitive basis for considering initial cursor position in this manner. With the PR's emit implementation, the cursor stays with the element it started on, even when that element is ejected from the list. But when the cursor starts on a list bracket, there is no current element to stay with. Strictly speaking, the PR implementation attempts to keep the cursor not merely on a specific element, but on a specific character. If that character is ejected from the list, the cursor is as well. Since a list bracket cannot possibly be ejected from the list it delimits, it makes sense that positioning the cursor on a list bracket would protect it from being "ejected" from the list during the emit operation.

The drawback to this approach (versus one that provides distinct command variants) is that it requires the user to pre-position the cursor on a list bracket to get the alternate behavior. This requirement might seem onerous for a user who uses the alternate behavior often...

Thoughts?

endif
catch /sexp-noop-error/
if a:mode !=? 'v'
silent! undo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emit with a count doesn't work properly when a count is used to emit all elements from a list, and cursor starts on the head element.
Example: Starting with...
(((|foo bar baz)))
...emit 3 elements, 2 different ways:

  1. with a count of 3
    (((|foo bar) baz))
  2. with 3 consecutive applications of emit (no count)
    (((|foo) bar) baz)

Root Cause: The :undo invoked in the non-visual "sexp-noop-error" handler undoes all the changes made in the docount loop thus far. If cursor doesn't start on the first element, it will already have escaped the list before the head element is reached, thereby preventing the "sexp-noop-error" from being thrown.
Question: What exactly is being undone in the noop case? Has anything even been done?

silent! undo
endif

call sexp#move_to_nearest_bracket(a:mode, a:last)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stack overflow occurs when you attempt to emit the only element of a list and there are no elements in ancestor lists to emit.
Example: At top level...
(((|foo)))

E132: Function call depth is higher than 'maxfuncdepth'

Root Cause: Attempt to emit the only element of a list results in "sexp-noop-error", whose catch handler simply attempts to move to containing bracket and retry. But when there are no elements in any ancestor lists to emit, this unconditional retry results in an infinite loop, since attempting to find containing bracket when you're already on a toplevel bracket doesn't move the cursor, and the catch handler doesn't check for this...

else
let cursorcol -= 1
endif
let cursorcol += (a:last ? 1 : -1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor not adjusted correctly for bracket displacement in some cases.
Root Cause: The test for bracket displacement is too simplistic: e.g., it doesn't work when char adjacent to cursor position happens to be same as original cursor char.
Example: Note the difference in final cursor position for the following 2 emits, which differ only in the value being emitted.
(+ 100 (foo 4|2)) => (+ 100 (foo) 4|2)
(+ 100 (foo 4|4)) => (+ 100 (foo) |44)

else
let cursorcol -= 1
endif
let cursorcol += (a:last ? 1 : -1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cursor positioning logic discussed in the preceding comment also leads to unexpected results in captures such as this one:

 (let
   ((|a b)))

=>

 ((let
   (a| b)))

The problem is that the call to stackop that actually captures the "let" is a recursive call made from the "sexp-noop-error" catch handler. Although this recursive call does in fact adjust cursorcol by negative 1 as desired, the adjustment is thrown away, since it's the adjustment performed in the toplevel call to stackop that determines the final cursor position.

@bpstahlman
Copy link
Collaborator

To flesh out several distinct concepts of emit and capture, I've documented 3 candidate "emit" commands, and 2 candidate "capture" commands. Would appreciate feedback from @snoe and others on the usefulness of the proposed commands. If the consensus is that only one command in each set is particularly useful (or one is significantly more useful than the others), perhaps the best approach is to change sexp's current emit/capture to work accordingly. OTOH, if multiple variants are deemed useful, perhaps it makes sense to provide several distinct emit/capture variants; alternatively, a single emit/capture command could be provided, along with an option to configure its behavior.

Note: I'm not overly attached to the proposed names - just needed a way to distinguish between the various commands...
Note: In all of the examples below, | indicates cursor position.

Emit commands

Emit

sexp_emit_head_element
sexp_emit_tail_element

Description: Emit [count] elements from current list, preserving cursor position unless element under cursor is emitted from list, in which case, cursor will be moved to the list bracket.
Note: The only difference between this and the original sexp emit is that the original always positions the cursor on list bracket (even when element under cursor is not emitted).

Example:

Direction Count Before After
tail 3 (((foo (a |b c)) d e)) (((foo (|) a b c) e))
tail 2 (((foo (|a b c)) d e)) (((foo (|a) b c) d e))

Note: I can see an argument in favor of removing lists made empty by the emit: e.g., the empty list before a in the 1st example above.

Barf

sexp_barf_head_element
sexp_barf_tail_element

Description: Emit a single element from current list [count] times, preserving cursor position relative to the element beneath it, even when the element under cursor is emitted from the list.
Note: This is essentially the snoe PR. It is also closest to emacs paredit behavior, hence the name.

Example:

Direction Count Before After
tail 3 (((foo (a |b c)) d e)) (((foo (a) |b) c d e))

Eject

sexp_eject_head_element
sexp_eject_tail_element

Description: Push element under cursor out of [count] containing forms, pushing any elements in its path.
Note: AFAIK, there is no comparable emit command in either vim-sexp or emacs paredit.

Example:

Direction Count Before After
tail 3 (((foo (a |b c)) d e)) (((foo (a))) |b c d e)
head 2 (((foo (a |b c)) d e)) ((foo a |b ((c)) d e))

Capture command

Capture

sexp_capture_head_element
sexp_capture_tail_element

Description: Pull [count] elements into the current list from ancestor forms, preserving original cursor position.
Note: This command is similar to the original sexp capture, but allows elements to be pulled from lists other than the immediate parent.

Example:

Direction Count Before After
tail 3 (foo ((a |b) (c d)) e f) (foo ((a |b (c d) e f)))
head 2 (foo ((a b) (c |d)) e f) (((foo (a b) c |d)) e f)

Slurp

sexp_slurp_head_element
sexp_slurp_tail_element

Description: Pull a single element into an ancestor list [count] times, preserving original cursor position.
Note: This is essentially the snoe PR. It is also closest to emacs paredit behavior, hence the name.

Example:

Direction Count Before After
tail 3 (foo ((a |b) (c d)) e f) (foo ((a |b (c d) e)) f)

@snoe
Copy link
Author

snoe commented Sep 30, 2018

@bpstahlman great write up. I think your capture and emit commands could work but would take some getting used to. There might be some small benefit to matching paredit because the commands are so ingrained across the ecosystem. On the other hand, if capture/emit feel more vim-like then I think that would fall inline best with this plugin.

Honestly I would be happy with any change that keeps cursor position getting to master; I see colleagues fumble around this issue all the time.

@bpstahlman
Copy link
Collaborator

@snoe Thanks. So maybe the best path forward for now is your recursive_stackop branch. I like your basic approach, and the bugfixes seem manageable. Additional flavors of emit/capture could always be added independently at a later date if enough users deem them useful...

I strongly concur on the importance of preserving cursor position. While adding a few features on my own branch, (enhanced-sexp-objects), I added a non-trivial amount of logic to ensure cursor position is preserved across command execution. Incidentally, if you or your colleagues ever want to take a look at any of the features on that branch, I'd welcome any feedback. I've got a significant amount of code cleanup to do yet, and nothing's fixed in stone, but the features are all documented in their current form, and should be basically working. It started as a branch to enhance sexp objects to allow the ie/ae objects to expand with multiple applications (just like Vim's iw, aw, etc.), but I ended up adding several other features I find useful. Also, if you have an emacs background and haven't updated recently, you might be interested to know that convolute and several new motion commands were merged to master over a year ago.

@bpstahlman
Copy link
Collaborator

Incidentally, in case anyone happened to try the (enhanced-sexp-objects) branch I mentioned in the previous post, I just pushed a fix involving the indent-triggered whitespace cleanup.

@bpstahlman bpstahlman mentioned this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants