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

revert overdeleted c-a binding from #715 #734

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Conversation

crides
Copy link
Contributor

@crides crides commented Jan 30, 2024

No description provided.

@fdncred
Copy link
Collaborator

fdncred commented Jan 30, 2024

@crides any reason?

@Tastaturtaste was there a reason that ctrl-a was removed?

@Tastaturtaste
Copy link
Contributor

Yes, the reason was that I wanted to avoid specifying default behaviour for nushell in multiple places. Since the nushell defaults are already defined in config.nu I removed the default from reedline while I was already at it removing conflicting defaults. I figured that since the feature was still pretty fresh and not yet released in nushell there shouldn't be any breakage.

@fdncred
Copy link
Collaborator

fdncred commented Jan 30, 2024

In nushell we prefer the reverse. If there is a default, it should be in rust code so that if you run nushell without a config file, you still get default keybindings as listed in default_config.nu. I don't see a conflicting ctrl+a. Is there one?

Also, not all nushell defaults are defined in the config.nu. Some of the VI keybindings aren't easily exposed and therefore are not available.

@Tastaturtaste
Copy link
Contributor

ctrl + char_a is already taken in default_config for move_to_line_start. Instead I added a binding for ctrl + shift + a for select_all.

And this confusion is the reason I would prefer to have all defaults in one place. But it's your call, of course.

@fdncred
Copy link
Collaborator

fdncred commented Jan 30, 2024

ctrl+a in default_config.nu is not conflicting with this PR because they're both move_to_line_start. what am I not understanding here?

    kb.add_binding(
        KM::CONTROL,
        KC::Char('a'),
        edit_bind(EC::MoveToLineStart { select: false }),
    );

If we were going to put select all on ctrl+a, then it would conflict but that's why I said ctrl+shift+a.

We have to have keybindings in rust code because if you run nushell without a config file, no keybindings would work at all.

@Tastaturtaste
Copy link
Contributor

There is no conflict, but there is only human vigilance to keep those places in sync. I realize now this is known and very much intended, even though I don't agree.

When I deleted the ctrl + a binding I didn't realize it was already there before my PR was merged and I didn't want to introduce further duplication. The removal of existing duplication was not my intention.

For all I know this PR should be fine to merge to get the previous default even without a default_config back.

@crides
Copy link
Contributor Author

crides commented Jan 30, 2024

@crides any reason?

Restoring previous well-known defaults

@fdncred
Copy link
Collaborator

fdncred commented Jan 30, 2024

ok, no worries at all @Tastaturtaste. I was just trying to understand your position. Thanks @crides.

@fdncred fdncred merged commit 9f0095f into nushell:main Jan 30, 2024
6 checks passed
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