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

ctrl-j and enter were indistinguishable #149

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

kuro337
Copy link
Contributor

@kuro337 kuro337 commented Jan 13, 2025

Hey!

I realized that the library logic currently (from my understanding) was making it indistinguishable to detect if the keypress is Ctrl-j or Enter.

This was for example in terminal applications treating it as Enter. Vim terminal users pretty frequently use Control-j as a separate keymap so I made this PR - let me know what you folks think, I just added both checks everywhere there was the existing Enter checks such as in the examples.

https://github.com/natecraddock/zf/blob/f8ba4a46b5ee6261c2c3d92fc0bc7239806d5406/src/tui/ui.zig#L268

    fn handleInput(state: *State, loop: *vaxis.Loop(Event), num_filtered: usize) !?Result {
        const old = state.*;

        const event = loop.nextEvent();
        switch (event) {
            .key_press => |key| {
                const visible_rows = @min(@min(state.config.height, state.vx.screen.height) - 1, num_filtered);

                if (key.matches('c', .{ .ctrl = true })) {
                    return .cancel;
                } else if (key.matches('w', .{ .ctrl = true })) {
                    if (state.query.len() > 0) deleteWord(&state.query);
                } else if (key.matches('u', .{ .ctrl = true })) {
                    state.query.deleteTo(0);
                } else if (key.matches(Key.backspace, .{})) {
                    state.query.delete(1, .left);
                } else if (key.matches('a', .{ .ctrl = true })) {
                    state.query.setCursor(0);
                } else if (key.matches('e', .{ .ctrl = true })) {
                    state.query.setCursor(state.query.len());
                } else if (key.matches('d', .{ .ctrl = true })) {
                    state.query.delete(1, .right);
                } else if (key.matches('f', .{ .ctrl = true }) or key.matches(Key.right, .{})) {
                    state.query.moveCursor(1, .right);
                } else if (key.matches('b', .{ .ctrl = true }) or key.matches(Key.left, .{})) {
                    state.query.moveCursor(1, .left);
                } else if (key.matches(Key.down, .{}) or key.matches('n', .{ .ctrl = true })) {
                    lineDown(state, visible_rows, num_filtered - visible_rows);
                } else if (key.matches(Key.up, .{}) or key.matches('p', .{ .ctrl = true })) {
                    lineUp(state);
                } else if (key.matches('k', .{ .ctrl = true })) {
                    if (state.config.vi_mode) lineUp(state) else state.query.deleteTo(state.query.len());
                } else if (key.matches(Key.tab, .{ .shift = true })) {
                    try state.selected_rows.toggle(state.selected + state.offset);
                    lineUp(state);
                } else if (key.matches(Key.tab, .{})) {
                    try state.selected_rows.toggle(state.selected + state.offset);
                    lineDown(state, visible_rows, num_filtered - visible_rows);
                } else if (key.matches(Key.enter, .{})) {
                    if (num_filtered == 0) return .none;
                    if (state.selected_rows.slice().len > 0) {
                        return .many;
                    }

The tests were failing for me on Aarch64 MacOS so due to an error for a missing method on the tty in src/tty.zig - not sure how relevant it is. But the tests and build passed after I just added it to the test struct.

Great library!

@rockorager
Copy link
Owner

You are correct that we don't distinguish between these two cases. I looked at a couple of other TUI libraries (tcell, notcurses, crossterm) and they handle things differently:

Tcell => Ctrl+J is a distinct event
Notcurses => Ctrl+J is the same as Enter
Crossterm => Ctrl+J is a distinct event

I normally would say that @dankamongmen (notcurses) is the end-all, be-all truth. However in this case I do think it makes sense to distinguish.

The method you've gone is not the right way though. We don't have any keys that are defined as a ctrl_<ascii> key. We always would do a 'j' + .{.ctrl = true} for this type of event. Do you think you could update that?

src/Parser.zig Outdated Show resolved Hide resolved
src/Key.zig Outdated Show resolved Hide resolved
examples/text_input.zig Outdated Show resolved Hide resolved
src/tty.zig Outdated Show resolved Hide resolved
src/tty.zig Outdated Show resolved Hide resolved
src/vxfw/Button.zig Outdated Show resolved Hide resolved
src/widgets/terminal/key.zig Outdated Show resolved Hide resolved
@dankamongmen
Copy link

I normally would say that @dankamongmen (notcurses) is the end-all, be-all truth. However in this case I do think it makes sense to distinguish.

i agree. i feel this is a bug in notcurses. thanks for bringing it to my attention!

@kuro337
Copy link
Contributor Author

kuro337 commented Jan 17, 2025

@rockorager

Thank you for the comments and feedback, appreciate it!

I made the changes, and tested ctrl-j is being successfully detected.
By the way how do you handle tests for this? I tested it by rebuilding it and using it with the commit here, and it properly detects it; not sure what the approach for writing tests is here.

@rockorager
Copy link
Owner

rockorager commented Jan 17, 2025 via email

@rockorager rockorager merged commit 91a310c into rockorager:main Jan 17, 2025
4 checks passed
@rockorager
Copy link
Owner

Also FYI squashed this one down to a single commit and reworded the commit title and message.

Thanks again!

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.

3 participants