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

[WIP] Multiple buffers #40

Merged
merged 11 commits into from Apr 26, 2016
Merged

[WIP] Multiple buffers #40

merged 11 commits into from Apr 26, 2016

Conversation

wesleywiser
Copy link
Contributor

@wesleywiser wesleywiser commented Apr 25, 2016

Related to #24

Here's what's currently implemented:

  • Support for multiple buffers
  • o file opens the file in a new buffer
  • ls lists the available buffers in a new buffer
  • b{n} switches to the specified buffer

I still need to:

  • Update help.txt
  • Add bd to remove the current buffer

I'd appreciate feedback on the code and\or the features.

Thanks!

SwitchToBuffer(usize),
}

fn try_get_buffer_command(c: &str) -> Option<BufferCommand> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is a little odd IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wrote it expecting that I would need special logic to handle the buffer commands that take a buffer id with them (like b{n} does). That's the only one that needs it now. Do you think I should just move this logic down into the call site?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, yeah, perhaps.

@ticki
Copy link
Contributor

ticki commented Apr 25, 2016

What a suprise to wake up to. Beautiful work! I left some nitpicks.

let cursor = self.cursor().clone();
self.cursors.insert(self.current_cursor as usize, cursor);
let current_cursor_index = self.buffers.current_buffer_info().current_cursor as usize;
self.buffers.current_buffer_info_mut().cursors.insert(current_cursor_index, cursor);
self.next_cursor();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think I should move cursor(), next_cursor() and prev_cursor() into BufferInfo? If I did that, then I could pull self.buffers.current_buffer_info_mut() into a local. It doesn't work now because of the borrow checker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think BufferInfo should be renamed to Buffer and then place this kind of stuff in it, since that would be more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good idea. Right now there is a Buffer trait that the SplitBuffer implements. Should I rename it or leave it?

@wesleywiser
Copy link
Contributor Author

Thank you!

Are there any other commands I should implement? :e {file}, :ls, b{n}, and bd are basically the only buffer commands I use frequently in Vim. I have some ideas regarding the output of ls but it's probably best to get the basics implemented first.

@ticki
Copy link
Contributor

ticki commented Apr 25, 2016

I think we're good for now. Very awesome work!

@ticki ticki added the feature label Apr 25, 2016
@wesleywiser
Copy link
Contributor Author

Thanks!

@ticki
Copy link
Contributor

ticki commented Apr 26, 2016

Awesome! Will merge.

@ticki ticki merged commit c7a37e8 into redox-os:master Apr 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants