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

Goto buffer #4756

Closed
wants to merge 10 commits into from
Closed

Goto buffer #4756

wants to merge 10 commits into from

Conversation

Elias-Graf
Copy link

Implemented a command to go to a specific buffer by index.

Command: buffer-goto <i>
Alias: b <i>

Inlined some functions, that had their origin in old implementation
detail, and are now unnecessary.
@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 15, 2022
= added 2 commits November 15, 2022 22:31
Now able to go to specific buffers by pressing CTRL+<[1-9]>.
helix-term/src/commands.rs Outdated Show resolved Hide resolved
Comment on lines 671 to 705
fn goto_first_buffer(cx: &mut Context) {
goto_buffer_by_idx_impl(cx.editor, 0);
}

fn goto_second_buffer(cx: &mut Context) {
goto_buffer_by_idx_impl(cx.editor, 1);
}

fn goto_third_buffer(cx: &mut Context) {
goto_buffer_by_idx_impl(cx.editor, 2);
}

fn goto_fourth_buffer(cx: &mut Context) {
goto_buffer_by_idx_impl(cx.editor, 3);
}

fn goto_fifth_buffer(cx: &mut Context) {
goto_buffer_by_idx_impl(cx.editor, 4);
}

fn goto_sixth_buffer(cx: &mut Context) {
goto_buffer_by_idx_impl(cx.editor, 5);
}

fn goto_seventh_buffer(cx: &mut Context) {
goto_buffer_by_idx_impl(cx.editor, 6);
}

fn goto_eight_buffer(cx: &mut Context) {
goto_buffer_by_idx_impl(cx.editor, 7);
}

fn goto_ninth_buffer(cx: &mut Context) {
goto_buffer_by_idx_impl(cx.editor, 8);
}
Copy link
Member

Choose a reason for hiding this comment

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

If this were a command, it should be implemented as a single command that uses cx.count() for the buffer number. I don't think that adding another command is necessary though: you can add the bindings you're after with just the typable command like so:

[keys.normal]
"C-1" = ":b 1"
"C-2" = ":b 2"
# ...

Copy link
Author

Choose a reason for hiding this comment

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

This is much cleaner. But the only location I could (relevantly) find "keys.normal" was in book/src/remapping.md. Does that mean there shouldn't keybindings by default? If yes, I have to disagree, as this is such an essential feature. Maybe I am using something incorrectly?

How would I use cx.count(), or better, how is count "provided"?.

Returns 1 if no explicit count was provided

I seem to always get 1, probably I'm not using it correctly.

Copy link
Member

@kirawi kirawi Nov 16, 2022

Choose a reason for hiding this comment

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

We have typable variants of the pipe commands, so there's precedence. You can have a typable version, and another version that utilizes cx.count(). As keybindings can be customizable, it's a bit ugly to work around that to support it in the default config.

helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
Comment on lines -670 to -671
fn goto_buffer(editor: &mut Editor, direction: Direction) {
let current = view!(editor).doc;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary to change the body of the old goto_buffer function - it has the same behavior before and after except that going to the previous buffer skips from the opposite direction

Copy link
Author

Choose a reason for hiding this comment

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

I mainly replaced the body of the function because I saw nearly identical code in both branches of the match statement. That just triggered my refactoring sense, lol.
After looking into it, using an iterator the way it is used here may be hard to understand / overkill for such a simple task.

except that going to the previous buffer skips from the opposite direction

I'm honestly not certain what you mean by this. buffer-next goes "to the right", and buffer-previous goes "to the left". Have I missed something that the original implementation does?

helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
= added 2 commits November 16, 2022 07:29
Previously collected into a vector, and then indexed that.
@QiBaobin
Copy link
Contributor

QiBaobin commented Nov 16, 2022

can we add this only to command so that we can take advantage of cx.count()? Regarding typable commands, I think it would be better to support name and path too . #4771

@Elias-Graf
Copy link
Author

Elias-Graf commented Nov 16, 2022

@QiBaobin I haven't yet figured out how cx.count() works. This has already been suggested in the reviews above, and I'm working on it. I don't have too much time on my hands during the day, so I will most likely get to it in the evening.

Regarding using the buffer name instead of the buffer ID (I assume you mean index, because the ID is different AFAIK). I think that doesn't really work, since two buffers can be named the same thing. For example, I was testing with multiple [scratch] buffers. In addition, you can already quickly switch between them by typing space+ b and then the name of your buffer (path also already works). I'm specifically implementing this, so I can use a shortcut (I'm hoping for ctrl + [1-9]) to switch between tabs (buffers). I heavily utilize this in my workflow, where for example components have multiple relevant files (e.g. HTML, CSS, TS), or I'm switching back and forth between implementation and spec / unit test.

I appreciate any suggestions and ideas relevant to this pull request :)

@QiBaobin
Copy link
Contributor

QiBaobin commented Nov 16, 2022

@QiBaobin I haven't yet figured out how cx.count() works. This has already been suggested in the reviews above, and I'm working on it. I don't have too much time on my hands during the day, so I will most likely get to it in the evening.

Regarding using the buffer name instead of the buffer ID (I assume you mean index, because the ID is different AFAIK). I think that doesn't really work, since two buffers can be named the same thing. For example, I was testing with multiple [scratch] buffers. In addition, you can already quickly switch between them by typing space+ b and then the name of your buffer (path also already works). I'm specifically implementing this, so I can use a shortcut (I'm hoping for ctrl + [1-9]) to switch between tabs (buffers). I heavily utilize this in my workflow, where for example components have multiple relevant files (e.g. HTML, CSS, TS), or I'm switching back and forth between implementation and spec / unit test.

I appreciate any suggestions and ideas relevant to this pull request :)

@Elias-Graf Good point. I recommend use name or path as I want to bind a key to do something like scatchpad, will would launch one specific buffer like scratch or terminal very quick.

Regarding cx.count(), you can get it in a function defined in src/commands.rs. It would work like that: you type number first then the key you bind to that function for example C-b. So that 1 C-b would take you to the first buffer, 2 C-b would be the second etc.

@Elias-Graf
Copy link
Author

@QiBaobin ohhh, thanks for telling me that. I bet it would have been ages until I figured that out. Sadly, this isn't really in-line with browsers / file explorers / terminals / other editors (maybe also see: #475 (comment)), which can be frustrating to use because the functionality is essentially the same :/

@QiBaobin
Copy link
Contributor

QiBaobin commented Nov 16, 2022

@QiBaobin ohhh, thanks for telling me that. I bet it would have been ages until I figured that out. Sadly, this isn't really in-line with browsers / file explorers / terminals / other editors (maybe also see: #475 (comment)), which can be frustrating to use because the functionality is essentially the same :/

@Elias-Graf Then we only need add a TypableCommand to accept the argument, we don't need touch commands.rs, just bind keys in the default config or our own config file to use them like C-1 = "buffer 1" etc.

However, I still think it's better to support name and path too, can you add them so that I can close my PR? If you don't have time, I can copy your logic to my PR.

@Elias-Graf
Copy link
Author

Elias-Graf commented Nov 16, 2022

@QiBaobin I will place them into the default config once I find it :P.

I'll see about the buffer name / path once I'm done with the things mentioned in the review. I would also appreciate the feedback of members / maintainers on this specifically (maybe @kirawi or @the-mikedavis). I personally don't see the use case (yet? :D), but I'm happy to add it if it's generally desired.

Some questions that come into mind:

  • What order would the evaluation be in?
  • What order would the completions be in?
  • What happens if a buffer is named 1?

#4393 might be relevant depending on answers to those questions (and how the renaming is implemented).

@Elias-Graf
Copy link
Author

Elias-Graf commented Nov 16, 2022

@QiBaobin Would be so kind and tell me where to do this:

just bind keys in the default config or our own config file to use them like C-1 = "buffer 1" etc.

I'm only able to find helix-term/src/keymap/default.rs, which, as far as my testing goes, does not support adding things like "C-1" => ":b1". I also found relevant text in the book book/src/remapping.md... which only talks about adjusting the local configuration.

@QiBaobin
Copy link
Contributor

@QiBaobin Would be so kind and tell me where to do this:

just bind keys in the default config or our own config file to use them like C-1 = "buffer 1" etc.

I'm only able to find helix-term/src/keymap/default.rs, which, as far as my testing goes, does not support adding things like "C-1" => ":b1". I also found relevant text in the book book/src/remapping.md... which only talks about adjusting the local configuration.

After looking at the code, it turned out we don't have any global config file like languages.toml yet, maybe we can create one so that we don't need write functions for every key.

@icefoxen
Copy link

icefoxen commented Feb 20, 2023

I think that doesn't really work, since two buffers can be named the same thing.

Poking my head in 'cause I was about to try to implement this anyway... iirc other editors suffix duplicate buffer names with a number when this happens, so if you have a buffer foo.txt then it's just called foo.txt, but if you open another buffer foo.txt then it is named foo.txt <2> or such. To me this seems nice because it lets you just use names in the common case of no duplicate names.

@gabydd gabydd mentioned this pull request Sep 7, 2023
@pascalkuthe
Copy link
Member

Closing this as stale. The vuffer picker and "gui like" order tabs workflow is not really a priority for us so I don't see us going forward with this.

Thank you for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants