Add `inputLength` to `seek` and `keyCount` to `keypress` #308

haberdashPI · 2023-07-27T02:33:58Z

This enables both seek and keypress ux more generally to accept more than one keypress.

The most immediate use for this is to implement the equivalent of vim-sneak. Sneak accepts two key presses, and like t, sneak jumps the cursor right before the next match of the input.

This also includes some shortcut commands for easily sneaking with the s key in normal mode using select.orSneak.

Would close #305

The new argument `inputLength` (which defaults to 1) determines how many characters the `seek` command accepts. This uses the newly implemented `keyCount` argument (which also defaults to 1) to accept multiples keypresses. The most immediate use for this is to implement the equivalent of vim-sneak. Sneak accepts two key presses, and like `t`, sneak jumps the cursor right before the next match of the input. This also includes some shortcut commands for easily sneaking with the `s` key in normal mode using select.orSneak.

71

Thanks! Sorry for taking that long to review, I've been really busy.

Also, please add a test (and preferably one for each selection mode)! A good location would be test/suite/commands/seek.md.

71 · 2023-08-01T11:57:28Z

src/api/prompt.ts

 */
-export async function keypress(context = Context.current): Promise<string> {
+export async function keypress(context = Context.current, keyCount: number = 1): Promise<string> {


Suggested change

export async function keypress(context = Context.current, keyCount: number = 1): Promise<string> {

export async function keypress({ keyCount = 1 }: Readonly<{ keyCount: number }> = {}, context = Context.current): Promise<string> {

By convention, context should be the last parameter. In case newer parameters are added later, I think we should put keyCount in an object as well.

71 · 2023-08-01T11:59:51Z

src/commands/selections.ts

@@ -351,6 +351,7 @@ export function filter(
 * | Title          | Identifier      | Keybinding            | Command                                                                                           |
 * | -------------- | --------------- | --------------------- | ------------------------------------------------------------------------------------------------- |
 * | Leap or select | `select.orLeap` | `s` (kakoune: normal) | `[".ifEmpty", { then: [[".seek.leap", { ... }]], otherwise: [[".selections.select", { ... }]] }]` |
+ * | Leap or select | `select.orSneak` | `s` (kakoune: normal) | `[".ifEmpty", { then: [[".seek", { inputLength: 2, ... }]], otherwise: [[".selections.select", { ... }]] }]` |


Suggested change

* | Leap or select | `select.orSneak` | `s` (kakoune: normal) | `[".ifEmpty", { then: [[".seek", { inputLength: 2, ... }]], otherwise: [[".selections.select", { ... }]] }]` |

* | Sneak or select | `select.orSneak` | `s` (kakoune: normal) | `[".ifEmpty", { then: [[".seek", { inputLength: 2, ... }]], otherwise: [[".selections.select", { ... }]] }]` |

Also, s conflicts with select.orLeap above. For now I think we should keep it this way (and add no keybinding for select.orSneak, then if users start to use .orSneak more we can change the keybinding (which should be fine, since both of these bindings are experimental.

Also also, please re-format the whole table to take the longer identifier / title into account.

71 · 2023-08-01T13:36:42Z

src/commands/selections.ts

+ * | Title                    | Identifier                    | Keybinding              | Command                                                                                                                         |
+ * | ------------------------ | ----------------------------- | ----------------------- | ------------------------------------------------------------------------------------------------------------------------------- |
+ * | Leap or select backward  | `splitLines.orLeap.backward`  | `a-s` (kakoune: normal) | `[".ifEmpty", { then: [[".seek.leap", { direction: -1, ... }]], otherwise: [[".selections.splitLines", { ... }]] }]`            |
+ * | Sneak or select backward | `splitLines.orSneak.backward` | `a-s` (kakoune: normal) | `[".ifEmpty", { then: [[".seek", { inputLength: 2, direction: -1, ... }]], otherwise: [[".selections.splitLines", { ... }]] }]` |


Suggested change

* | Sneak or select backward | `splitLines.orSneak.backward` | `a-s` (kakoune: normal) | `[".ifEmpty", { then: [[".seek", { inputLength: 2, direction: -1, ... }]], otherwise: [[".selections.splitLines", { ... }]] }]` |

* | Sneak or select backward | `splitLines.orSneak.backward` | | `[".ifEmpty", { then: [[".seek", { inputLength: 2, direction: -1, ... }]], otherwise: [[".selections.splitLines", { ... }]] }]` |

71 · 2023-08-01T13:44:18Z

src/commands/load-all.ts

+function getInputLength(argument: { inputLength?: number }) {
+  const len = argument.inputLength || 1;
+  if (len > 0 && Number.isInteger(len)) {
+    return len;
+  }
+  throw new ArgumentError('"inputLength" must be positive integer');
+}


This function is not used.

71 · 2023-08-01T13:45:18Z

src/commands/load-all.ts

+  if (len > 0 && Number.isInteger(len)) {
+    return len;
+  }
+  throw new ArgumentError('"inputLength" must be positive integer');


Suggested change

throw new ArgumentError('"inputLength" must be positive integer');

throw new ArgumentError('"inputLength" must be positive integer', "inputLength");

71 · 2023-08-01T13:48:21Z

src/commands/load-all.ts

@@ -30,6 +30,14 @@ function getRegister<F extends Register.Flags | Register.Flags[]>(
  return (argument.register = register as any);
 }

+function getInputLength(argument: { inputLength?: number }) {
+  const len = argument.inputLength || 1;


If argument.inputLength is 0, this will yield 1. I think using the same logic as getCount (using + to cast to a number and then checking that len > 0 && Number.isInteger(len) would be better (so if the input is a string for some reason, that still works).

src/api/prompt.ts

haberdashPI · 2023-08-07T02:33:46Z

Just wanted to leave an update here. I've responded to all of the minor comments at this point and am working through the tests. I love the test framework you're using by the way, super cool!

In setting up / thinking through the tests I noticed a bug where the value of repetitions is 1 inside seek regardless of what numeric argument is entered by the user, but only when inputLength == 2 (presumably it would also be true for > 2 but I haven't tested that). I haven't looked much at the machinery for how the count state is maintained, but I'm guessing there is some logic there for how/when the count is reset to 1 that assumes an invariant the extra keystroke violates. I can track down myself, but thought it was worth mentioning in case something obvious occurs to you.

haberdashPI · 2023-08-23T22:04:17Z

Brief update: I figured out the cause of my woes when dealing with counts for sneak commands (see #310).

Having identified that as a separate issue I'm now moving on to finalizing the tests for this.

haberdashPI · 2023-08-23T22:33:59Z

Alright, tests are passing locally. I think this is ready for review again.

haberdashPI · 2023-09-18T18:08:25Z

Hey, just checking in on this. Are there any issues I need to address before you can review?

71

Thanks for the changes! Sorry it took me so long to review this PR.

71 · 2023-09-27T14:17:25Z

src/api/prompt.ts

-export async function keypress(context = Context.current): Promise<string> {
+export async function keypress(params: Readonly<{ keyCount?: number }>,
+                               context = Context.current): Promise<string> {
+  const keyCount = params.keyCount || 1;


If params.keyCount is 0, this will evaluate to 1. I'm not sure how I feel about that. IMO for params.keyCount === 0 we could return immediately with an empty string.

71 · 2023-09-27T14:19:02Z

src/api/prompt.ts

    try {
+      let keys = "";


nit: I try to only put throwing stuff in try, and to keep everything else out of it.

Suggested change

try {

let keys = "";

let keys = "";

try {

71 · 2023-09-27T14:20:01Z

src/commands/load-all.ts

+function getInputLength(argument: { inputLength?: number }) {
+  const len = +(argument.inputLength as any);
+  if (len > 0 && Number.isInteger(len)) {
+    return len;
+  }
+  throw new ArgumentError('"inputLength" must be positive integer', "inputLength");
+}


I believe this function is still unused?

You probably wanted to automatically use this function for inputLength arguments, similarly to what's done with count and repetitions?

71 · 2023-09-27T14:23:20Z

src/api/data/commands.yaml

+
+  keys:
+    qwerty: ""


I'm guessing this is a result of having an initial command and then removing it (the script that generates this file tries to keep old values to avoid throwing away manual changes). This should be removed (and ditto a bit lower in the file).

71 · 2023-09-27T14:27:25Z

test/suite/commands/seek.md

@@ -250,3 +250,50 @@ ghi
 jkl
 mno
 ```
+
+# 4


Could you add a test when seeking backward? I don't know what the expected result is, but there might be some problems with the selection not extending exactly as expected.

haberdashPI · 2023-09-27T20:22:31Z

Thanks for the changes! Sorry it took me so long to review this PR.

No problem. Thanks for taking a look, I'll take a read through and update as needed.

haberdashPI force-pushed the dfl/seek-count branch from 6420daa to 05bfdd8 Compare August 1, 2023 02:33

71 requested changes Aug 1, 2023

View reviewed changes

cleanup based on review

23182a6

wip tests

4f8b985

haberdashPI mentioned this pull request Aug 23, 2023

ifEmpty does not pass count to it's subcommands. #310

Closed

haberdashPI added 2 commits August 23, 2023 18:11

working tests?

d2d5cee

tests for multiple character modes

6b07685

haberdashPI requested a review from 71 August 23, 2023 22:33

doc cleanup

3ef52e4

71 requested changes Sep 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `inputLength` to `seek` and `keyCount` to `keypress` #308

Add `inputLength` to `seek` and `keyCount` to `keypress` #308

haberdashPI commented Jul 27, 2023 •

edited

Loading

71 left a comment

71 Aug 1, 2023

71 Aug 1, 2023

71 Aug 1, 2023

71 Aug 1, 2023

71 Aug 1, 2023

71 Aug 1, 2023

haberdashPI commented Aug 7, 2023 •

edited

Loading

haberdashPI commented Aug 23, 2023 •

edited

Loading

haberdashPI commented Aug 23, 2023

haberdashPI commented Sep 18, 2023

71 left a comment

71 Sep 27, 2023

71 Sep 27, 2023

71 Sep 27, 2023

71 Sep 27, 2023

71 Sep 27, 2023

haberdashPI commented Sep 27, 2023

	export async function keypress(context = Context.current, keyCount: number = 1): Promise<string> {
	export async function keypress({ keyCount = 1 }: Readonly<{ keyCount: number }> = {}, context = Context.current): Promise<string> {

	* \| Leap or select \| `select.orSneak` \| `s` (kakoune: normal) \| `[".ifEmpty", { then: [[".seek", { inputLength: 2, ... }]], otherwise: [[".selections.select", { ... }]] }]` \|
	* \| Sneak or select \| `select.orSneak` \| `s` (kakoune: normal) \| `[".ifEmpty", { then: [[".seek", { inputLength: 2, ... }]], otherwise: [[".selections.select", { ... }]] }]` \|

	* \| Sneak or select backward \| `splitLines.orSneak.backward` \| `a-s` (kakoune: normal) \| `[".ifEmpty", { then: [[".seek", { inputLength: 2, direction: -1, ... }]], otherwise: [[".selections.splitLines", { ... }]] }]` \|
	* \| Sneak or select backward \| `splitLines.orSneak.backward` \| \| `[".ifEmpty", { then: [[".seek", { inputLength: 2, direction: -1, ... }]], otherwise: [[".selections.splitLines", { ... }]] }]` \|

	throw new ArgumentError('"inputLength" must be positive integer');
	throw new ArgumentError('"inputLength" must be positive integer', "inputLength");

@@ @@ -250,3 +250,50 @@ ghi @@
               jkl
               mno
               ```
+              # 4

Add inputLength to seek and keyCount to keypress #308

Are you sure you want to change the base?

Add inputLength to seek and keyCount to keypress #308

Conversation

haberdashPI commented Jul 27, 2023 • edited Loading

71 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haberdashPI commented Aug 7, 2023 • edited Loading

haberdashPI commented Aug 23, 2023 • edited Loading

haberdashPI commented Aug 23, 2023

haberdashPI commented Sep 18, 2023

71 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haberdashPI commented Sep 27, 2023

Add `inputLength` to `seek` and `keyCount` to `keypress` #308

Add `inputLength` to `seek` and `keyCount` to `keypress` #308

haberdashPI commented Jul 27, 2023 •

edited

Loading

haberdashPI commented Aug 7, 2023 •

edited

Loading

haberdashPI commented Aug 23, 2023 •

edited

Loading