Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sublime-like changes #109
base: master
Are you sure you want to change the base?
Sublime-like changes #109
Changes from 9 commits
d202ae3
83692fd
4f9d809
2d2763e
a4e9f93
386f22b
dfd0781
0699d1c
0bc9269
117299f
702b140
39995fd
1c11099
db5488b
1424a88
2b38760
decb28f
c276d9b
b6472f1
fbd71c2
574c9a9
15ce710
495083c
e31365d
62a601f
6c32c20
0e77c3d
ab35a26
f19eb17
8bfcfa1
694b3b1
88c8b35
3790457
b1f795c
e94e3e9
7e1aea1
75dff11
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
words yes, word_ends no. I have it in there so that serde can parse the given keymap without choking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theby
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 makeby
redundant). But I didn't have the time to hack that into the serde parsing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does
extend
do?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.