Sublime-like changes #109
Sublime-like changes #109
Conversation
…icer way right now.
…d of 'move left', 'move right'
… keys are now mapped.
…t just handles the command supposed for itself and hands the rest down
… do ctrl+shift+char
…ach occurence, as well as line-wise.
…(with the exception of prompt commands with arguments).
…needs to be redesigned from scratch.
…you couldn't copy from one view to another.
…nds not yet working (needs a map of its own).
There was a problem hiding this comment.
Thanks a lot for all this work @msirringhaus. I only looked at the first 3 9 commits (up to 0bc9269) for the moment but there may be enough so spark some discussions already. Since I'm reviewing commit by commit, there are probably things that are already addressed, so feel free to mark those comments as resolved with a link to the relevant commit if that's the case.
src/core/cmd.rs
Outdated
| "d" | "delete" => Ok(Command::Delete), | ||
| "bn" | "next-buffer" => Ok(Command::NextBuffer), | ||
| "bp" | "prev-buffer" => Ok(Command::PrevBuffer), | ||
| "q" | "quit" | "exit" => Ok(Command::Quit), |
There was a problem hiding this comment.
I think we should refrain from adding more command aliases. We could discuss modifying the existing ones, but I think it will confusing to have too many commands that do the same thing.
There was a problem hiding this comment.
Well, I never planned to do a full PR.
I switched to command names used by sublime texts keymap-file, but didn't want to remove the old names just yet.
| @@ -0,0 +1,742 @@ | |||
| [ | |||
There was a problem hiding this comment.
This is kind of bike-sheddy but I'm not a fan of JSON as a configuration format, even though in this case JSON5 is a bit easier for humans to read. For something as simple as this configuration, something like toml may be a bit more concise and readable.
On another topic, this file contains lots of things that don't apply to xi. It would be nice to reduce it to what is currently supported.
There was a problem hiding this comment.
Again, my goal, which I know is not compatible with xi-terms goal is to have compatibility with sublime. Thus, I used their keymap-files, so existing user-configs could just be copied in.
There was a problem hiding this comment.
which I know is not compatible with xi-terms goal is to have compatibility with sublime
Oh I had misunderstood that part. I thought you wanted to make something somewhat similar to Sublime, but not 100% compatible. In that case, would you mind if I took whatever I'm interested in from your changes, and adapt it to what I envision for xi-term?
There was a problem hiding this comment.
Yes, absolutely. That was my suggestion from the beginning :-)
To be clear: I don't have 100% compatibility as a hard requirement, although it would be nice and handy.
But since you are, as far as I understand, going for vim-like modular editing my editor (be it 100% sublime compatible or not) will definitely not be compatible with xi-term.
So i figured a hard fork is my only path.
It certainly would be nice join forces, though. But currently I don't really see how.
There was a problem hiding this comment.
But since you are, as far as I understand, going for vim-like modular editing my editor
Oh I don't have a clear path actually. I don't really want to make a vim clone for multiple reasons (I don't like VimScript in the first place). It's true that I'd like to provide a way to do vim-like editing because that's what I'm used to, but I'd like to make that optional, and allow users to use their own keybindings. Emacs is doing that with evil mode for instance (which is what I use), and I really like that approach.
It certainly would be nice join forces, though. But currently I don't really see how.
As far as I can tell the only thing that is really specific to Sublime in this PR is the configuration format and the default bindings. To me, the Sublime bindings are fine, and it's easy to use different ones anyway. The only thing I'm not fond is using json for the config file but I think I can live with that for now and switch to something else in the future (there are solutions if we want to keep supporting json: having multiple parsers, or providing a tool to convert config files, etc.).
I've been using vim for a long time, but I'm not opposed to ideas from other editors because I'm not convinced that making re-implementing an existing editor is likely to succeed. First, it's hard to replicate the exact behavior of another editor. Second, there are behaviors don't want to keep (I don't like vim's registers I prefer emacs kill ring, and I don't like emacs undo system, I prefer vim's for instance). Finally, users won't switch to something new if it's exactly the same thing.
So yeah, I'm open to non-vim-like behavior :)
There was a problem hiding this comment.
Finally, users won't switch to something new if it's exactly the same thing.
Absolutely. The only reason I started this was because there simply is no terminal editor like sublime at all. There is one without multiple cursors and such, but that is abandoned (and written in JS. Of course...).
So yeah, I'm open to non-vim-like behavior :)
Ok, cool! So maybe we could join forces after all. That would be nice.
Yes the only really sublime-specific stuff is the json-format (including structs for parsing it).
Once we have the translated map of Temion-Keyevents and Command, it doesn't matter anymore.
And since the Command-structs for parsing the json could get different names with the serde-macros, we can hide sublime heritage there as well.
Only, in the prompt, I currently expose sublimes names to the user as well. There could of course be another layer in there as well. Tried to keep it simple for now.
The only blocker I see would be the modal editing (insert mode, vs. command mode), if you envision that for your editor.
There was a problem hiding this comment.
So maybe we could join forces after all.
I hope so!
Once we have the translated map of Temion-Keyevents and Command, it doesn't matter anymore.
Yeah, that's why I'm not to worried about using JSON for now. It should be easy to change in the future.
The only blocker I see would be the modal editing (insert mode, vs. command mode), if you envision that for your editor.
Well for now no frontend has it, and it's unclear whether that requires support in xi-core or not. Plus there are tons of other features to add first, so it's not a priority for me right now.
There was a problem hiding this comment.
Well for now no frontend has it, and it's unclear whether that requires support in xi-core or not. Plus there are tons of other features to add first, so it's not a priority for me right now.
Alright. Although I see (for the base functionality) no reason to involve xi-core. If editor.rs has a mode it can interpret a keypress-event like "w" as either Insert(w) or RelativeMove{by: word} and act accordingly.
src/core/config.rs
Outdated
| } | ||
|
|
||
| impl KeybindingConfig { | ||
| pub fn parse(config_path: &Path) -> Result<KeybindingConfig, Box<std::error::Error + Sync + Send + 'static>> { |
There was a problem hiding this comment.
We should probably have an ConfigError error instead of using a trait object. Something like:
enum ConfigError {
Parsing(...), // contains the inner error
Invalid(....),
.... // potentially a bunch of other variants.
}There was a problem hiding this comment.
Yeah, error handling was pretty low on my list, to be honest. I basically just reused existing error types. This definetly needs a major cleanup (I actually think of removing "failure"-crate in the process, as its not really maintained anymore AFAIK)
| pub by: RelativeMoveDistance, | ||
| pub forward: bool, | ||
| #[serde(default)] | ||
| pub extend: bool |
There was a problem hiding this comment.
Yes, not a perfect name (taken from sublime). If true, it states that this movement should extend the current selection (e.g. move one word right with extend selects that word).
Works for all kinds of movements (page up down, go to beginning / end of file / line, and so on).
src/core/cmd.rs
Outdated
| /// Move to new word | ||
| words, | ||
| /// Move to end of word | ||
| word_ends, |
There was a problem hiding this comment.
Are these all actually implemented in xi? I'd rather have only what's currently implemented, with maybe a comment explaining how this may be extended in the future.
There was a problem hiding this comment.
words yes, word_ends no. I have it in there so that serde can parse the given keymap without choking
| #[derive(Debug, Deserialize, Serialize, PartialEq, Clone)] | ||
| pub struct RelativeMove { | ||
| pub by: RelativeMoveDistance, | ||
| pub forward: bool, |
There was a problem hiding this comment.
I'm not sure how I feel about forward. On one hand, I think enums are clearer: Direction::{Left,Up,Right,Down} is clearer than have to think: "horizontal move + not forward => left", "vertical move + forward => down", etc. On the other, having an enum with all 4 directions here would be partially redundant with the by attribute, because when moving by lines, we already know the direction is either up or down for instance.
There was a problem hiding this comment.
This again comes from sublimes keymap. Thats how they specify it and I just went along. I agree that an enum would be nicer here (but I would actually just use Direction::{Forward, Backward}, so not to make by redundant). But I didn't have the time to hack that into the serde parsing.
|
@msirringhaus do you mind explaining a bit what you added in db5488b ? Where are the cursors rendered here? It seems like what this adds is the ability to select multiple regions corresponding to a search result, is that right? |
That is correct. Its the Once you have something selected, repeated
Indeed, they are currently not. Edit: To clarify: It works, but you can only see the main cursor at the moment |
|
Selecting lines suffers the same issue than #99 (comment) Edit: Oh, I see you already commented on that issue here, sorry. |
| @@ -91,12 +96,22 @@ impl KeybindingConfig { | |||
| "delete" => Some(Event::Key(Key::Delete)), | |||
| "insert" => Some(Event::Key(Key::Insert)), | |||
| "escape" => Some(Event::Key(Key::Esc)), | |||
| "ctrl+right" => Some(Event::Unsupported(vec![27, 91, 49, 59, 53, 67])), | |||
There was a problem hiding this comment.
I'm a bit confused: why are these named "ctrl+something" instead of an actual command name like "word-left" or "word-right"? Other question: what are ctrl+shift+up and ctrl+shift+down supposed to do? It seems that it's not doing anything visible at the moment.
There was a problem hiding this comment.
I'm a bit confused: why are these named "ctrl+something" instead of an actual command name like "word-left" or "word-right"?
It looks different on HEAD. There its separated. a bit clearer.
But in principle there are two parsers: One is to parse a command name to Command-struct (e.g. select_all).
The other is to parse key-descriptions to Termion::Event, e.g. escape to Key::Esc.
Now a user can edit their JSON file to let pressing escape trigger select_all for example.
ctrl+right and others are not supported by Termion right now, as they differ from Terminal to Terminal, which...sucks. But to have the functionality, I basically used the Unsupported event to make it work with my terminal.
So this is already a "known bug" of this implementation. Another is, that Termion doesn't differentiate between ctrl+a and ctrl+shift+a. I have opened a bugreport there, but I fear its not really actively maintained anymore.
Other question: what are ctrl+shift+up and ctrl+shift+down supposed to do? It seems that it's not doing anything visible at the moment.
Try typing after doing that :-) It adds a cursor below or above the current cursor, which are unfortunately invisible at the moment. I have some screencasts showing of the features, just have to find a place to upload to
There was a problem hiding this comment.
Ah, sorry. I confused ctrl+shift+up/down with alt+shift+up/down.
ctrl+shift+up/down is supposed to move the current line one line up or down (swap lines). But this is currently not supported by xi-core ( #793 ).
Either way, this part is not there to define functionality! Its there so that users can use ctrl+shift+up/down for whatever they want (They could map the function of alt+shift+up/down to ctrl+shift+up/down. Or they could for whatever reason map ctrl+shift+up to select_all and ctrl+shift+down to next buffer.).
This part of the code is only there to "know a key-combination exists". Not connect it to any functionality.
But for completeness, here are the screencasts I talked about (just "view raw" for each file):
https://gist.github.com/msirringhaus/83fbc1ef10ae93e56fe8d68dea1dd3b3
"switchcase" is what alt+shift+up/down is doing at the moment.
ctrlD obviously is what ctrl+d does.
And Palette is the CommandPrompt with suggestions. There is a crate to have sublime-like fuzzy-search, which I want to implement next. Then finally, the Prompt should handle up/down movements to select suggestions (at the moment it only shows them)
|
@msirringhaus |
|
@msirringhaus I've looked at every commit at this point although I just glanced at some, and I played a bit with the editor. This is pretty awesome work, and I'd love to integrate it into xi-term. But as it stands, this PR a bit too big to review properly, and I won't be able to do a good job at merging it. Do you think you could try to split it into smaller PRs and rework the git history so that commits don't have some unrelated changes? (No pressure, I'm not in a hurry). |
|
In principle yes. First off, it might be way less work to just review HEAD as it is. Most of the commits are me reworking my own features a bazillion times. Thus adding a lot of noise which is not visible on HEAD anymore. But if that is still too big, I could in theory split it up. But do you require for those intermediate commits to actually work and compile? That would be a lot of work, and unfortunately my "work on whatever project you want"-week is now over :-( So time is again limited. |
No description provided.