Update refactored split functions: allow delete

[mneilsen]

This change should fix M-delete and M-d's behavior by requiring callers to specify whether or not "processed" characters should be omitted from the split routines' results.

Tested in iTerm2 on MacOS.

[hannesm]

could you please describe the problem with the current behaviour, and how this PR fixes it? -- maybe providing an example -- I suspect it's all about the whitespace character!?

[mneilsen]

Certainly!

Here's the current behavior, after common code was refactored into split_backward and split_forward in cli/cli_support.ml (941a4a). The "|" indicates where the cursor is on the input line, and the key pressed follows to the right ("[M-backspace]" for example). The next line shows the updated input line, and so on.

12 345|    [M-backspace]
12 |34     [M-backspace]
|1234

|12 345    [M-d]
2| 345     [M-d]
2345|

Here's what we want to see, and the behavior this changeset restores from my previous pull request (68545f).

12 345|    [M-backspace]
12 |       [M-backspace]
|

|12 345    [M-d]
| 345      [M-d]
|

During 941a4a's refactoring, we lost a really subtle difference in how List.fold_left's accumulator handles the case where we want to delete the characters "processed" by the fold in split_backward and split_forward. When deleting, each non-whitespace character we come across should not be included in the accumulator. When we do include these characters, which is the current implementation, we have the first example above for M-backspace and M-d. Perhaps this is the behavior you observed when commenting on my first pull request?

This changeset introduces a flag to the refactored code which allows us to specify whether or not we want to "delete" characters during processing (more specifically, whether or not we will propagate eligible characters through the List.fold_left's accumulator or not).

[hannesm]

thank you! this is helpful -- I'm also thinking of factoring out these parts into a library, and have some unit tests in there (testing entire jackline is a bit off-scope)...

[hannesm]

I wonder if there is a less complex solution -- could you give the tst branch of this repo a try?

[mneilsen]

I had to play with the tst branch's cli/cli_support.ml a bit before I stumbled on the right behavior (though another set of eyes to confirm would be appreciated). Here's a tiny patch with the changes I made:

diff --git a/cli/cli_support.ml b/cli/cli_support.ml
index 4dc23b2..74f7407 100644
--- a/cli/cli_support.ml
+++ b/cli/cli_support.ml
@@ -280,12 +280,12 @@ let split_backward pre =
   (pre, prep, middle)

 let backward_word pre post =
-  let pre, _, middle = split_backward pre in
-  (pre, middle @ post)
+  let pre, prepost, middle = split_backward pre in
+  (pre, prepost @ middle @ post)

 let backward_kill_word pre post =
-  let pre, prepost, _ = split_backward pre in
-  (pre, prepost @ post)
+  let pre, _, _ = split_backward pre in
+  (pre, post)

 let emacs_bindings = function
   | `Key (`Uchar x, [`Ctrl]) when Uchar.to_int x = 0x41 (* C-a *) -> `Ok (fun (pre, post) -> ([], pre @ post))

I think there's still a little more that could be done in terms of refactoring to make these operations' implementations a little more apparent/obvious, but (assuming I maintained the expected behavior) I also think this is a step in the right direction.

[hannesm]

@mneilsen thank you for testing, and for discovering this issue (I changed the wrong function, and ended up in a mess). I amended the fixing commit, and added https://github.com/hannesm/jackline/commit/43ac78713e9986fa7dafbc48e1c2aa2bd9a71b14 which simplifies the implementation by returning only two values instead of one. If you can test the tst branch again, and let me know if I introduced again any regressions, that'd be great. If there are no regressions, I'll merge that into master.