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

edit/edcore: Implement functions for word-based movement. #721

Merged
merged 7 commits into from Aug 7, 2018

Conversation

@kwshi
Copy link
Contributor

kwshi commented Jul 16, 2018

Implemented kill-word-right, kill-small-word-right, move-dot-left-small-word, and move-dot-right-small-word.

Oops: I had overlooked the fact that @muesli had already implemented the last two functions (move-dot-left-small-word and move-dot-right-small-word); I originally set out to implement the kill- functions, saw that those two happened to be missing from the source, and accidentally did those too.

Note, however, that these two implementations behave differently. @muesli's implementation seems to always move to the next non-alphanumeric character, as though it only considers alphanumeric runs as valid small-words. This doesn't exactly match the current definition (as described by @xiaq in the comment before kill-small-word-left), which also considers non-alphanumeric runs as valid (but separate) small-words. This implementation tries to follow the current definition exactly.

@kwshi

This comment has been minimized.

Copy link
Contributor Author

kwshi commented Jul 16, 2018

By the way, this addresses #702.

@xiaq

This comment has been minimized.

Copy link
Member

xiaq commented Jul 22, 2018

Maybe rebase the work when #696 gets merged and apply the idea you proposed in #722?

@kwshi

This comment has been minimized.

Copy link
Contributor Author

kwshi commented Jul 23, 2018

Resolves #722.

@kwshi

This comment has been minimized.

Copy link
Contributor Author

kwshi commented Jul 23, 2018

Resolves #702.

return func() {
index := move(ed.buffer, ed.dot)

if index < ed.dot { // delete left

This comment has been minimized.

Copy link
@xiaq

xiaq Jul 29, 2018

Member

Use tabs for indentation. BTW, it seems that you are using a tab width of 2 and this looks totally wrong with GitHub's value of 4 :)

This comment has been minimized.

Copy link
@kwshi

kwshi Jul 31, 2018

Author Contributor

Strange. I'm using the automatic indentation with emacs's go-mode. I'll be sure to run gofmt next time...

return space
}

// NOTE(kwshi): The definition of word is the same as above. When killing a

This comment has been minimized.

Copy link
@xiaq

xiaq Jul 29, 2018

Member

As we discussed in #696, all of moveDot{Left,Right}{Word,SmallWord} can be derived from a "categorizer" function

@kwshi kwshi force-pushed the kwshi:master branch from fc0c0df to 341e218 Jul 31, 2018
@xiaq

This comment has been minimized.

Copy link
Member

xiaq commented Aug 2, 2018

Ah, I just saw that you have pushed more changes. I will review later today.

GitHub doesn't send out emails on pushes to PR, you would need to remind me to do another review (or if you don't mind, you can also rely on my occasional browsing of PRs ;-) )

// `categorizeAlphanumeric` for examples.
func moveDotLeftCategoryFunc(categorize func(rune) int) func(string, int) int {
return func(buffer string, dot int) int {
// move to last word

This comment has been minimized.

Copy link
@xiaq

xiaq Aug 6, 2018

Member

Avoid the use of the word "word" here. Say "skip whitespaces".

}

// Returns a move- function that moves the cursor to the beginning of
// the nearest "run" (characters of the same category) to the left.

This comment has been minimized.

Copy link
@xiaq

xiaq Aug 6, 2018

Member

This wording might be better:

Returns a move function that moves the dot to the beginning of the last run of characters of the same non-whitespace category to the left of the dot.

_, w := utf8.DecodeLastRuneInString(ed.buffer[:ed.dot])
ed.dot -= w
}
// Returns a move-function that moves the cursor to the beginning of

This comment has been minimized.

Copy link
@xiaq

xiaq Aug 6, 2018

Member

This might be better:

Returns a move function that moves the dot to the beginning of the next run of characters of the same non-whitespace category to the right of the dot.

(The word "next" already implies skipping over the current run, IMO.)

//
// See `moveDotLeftCategoryFunc`.
func moveDotRightCategoryFunc(categorize func(rune) int) func(string, int) int {
return func(buffer string, dot int) int {

This comment has been minimized.

Copy link
@xiaq

xiaq Aug 6, 2018

Member

Can we use a similar technique as in moveDotLeftCategoryFunc, using strings.TrimLeftFunc instead of strings.IndexFunc?

//
// See `categorizeWord`, `categorizeSmallWord`, and
// `categorizeAlphanumeric` for examples.
func moveDotLeftCategoryFunc(categorize func(rune) int) func(string, int) int {

This comment has been minimized.

Copy link
@xiaq

xiaq Aug 6, 2018

Member

Can we make this a low-order function moveDotLeftCategoryFunc(categorize func(rune) int, buffer string, dot int) int? This will make the definition of the moveDotLeft* functions slightly more complex (did you know that gofmt actually doesn't mind if you define a series functions as one-liners without empty lines between them?), but it is slightly clearer IMO. Ditto for the moveDotRightCategoryFunc.

func (ed *editor) moveDotRightSmallWord() {
ed.moveDotRightSepFunc(func(r rune) bool { return !isAlnum(r) })
}
var (

This comment has been minimized.

Copy link
@xiaq

xiaq Aug 6, 2018

Member

Can we have some unit tests for these functions? :)

@kwshi kwshi force-pushed the kwshi:master branch from 341e218 to 6c4f0cf Aug 7, 2018
@kwshi

This comment has been minimized.

Copy link
Contributor Author

kwshi commented Aug 7, 2018

@xiaq, I have made the changes you suggested in your review. Let me know what you think. Cheers!

😋

@xiaq

This comment has been minimized.

Copy link
Member

xiaq commented Aug 7, 2018

Wonderful!

@xiaq xiaq merged commit f9ef4e6 into elves:master Aug 7, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.