Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

For add new selection, use display column#7312

Merged
njx merged 4 commits intomasterfrom
randy/issue-7286
Mar 27, 2014
Merged

For add new selection, use display column#7312
njx merged 4 commits intomasterfrom
randy/issue-7286

Conversation

@redmunds
Copy link
Copy Markdown
Contributor

This is for #7286.

Are there are any other commands that need this same behavior?

/cc @njx

Comment thread src/editor/Editor.js Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Naming nit: I think this would be clearer as something like getCharIndexForColumn(). I know the symmetrical function is named getColOffset(), which isn't very descriptive, but I think getPosOffset() is somewhat less descriptive since it's not really obvious that it has to do with columns or character indices.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also...I wonder if there's some clever way to share code with getColOffset(), since the logic is very similar - the only difference is whether you're returning the char index or the column index, and the loop end condition. (It's slightly obscured by the fact that getColOffset() just gets the portion of the line up to the char index, but you could rewrite it to just get the whole line, and just use a different end condition for the two cases.) That said, it's a fairly small amount of code so it's probably not important to be that clever...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The input is also different. One goes from pos to colOffset and the other goes from colOffset to pos. These functions are opposites, so I don't think complexity added by merging them is worth the few lines of code that are saved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We might be able to have a third function with the for, which would receive the line/range and a stop function (which receives the index and the line length and returns true or false) and returns the column. Each function can then call the third one with the line and the stop function.

@njx
Copy link
Copy Markdown

njx commented Mar 25, 2014

@redmunds Review complete - just a couple of nits.

It would also be good to have a couple of unit tests for this...would you mind adding some?

@redmunds
Copy link
Copy Markdown
Contributor Author

Pushed changes. Ready for another review.

@njx
Copy link
Copy Markdown

njx commented Mar 27, 2014

Looks good, merging. Ran grunt test manually since the cla check is erroneously failing in travis.

njx pushed a commit that referenced this pull request Mar 27, 2014
For add new selection, use display column
@njx njx merged commit bcab0a2 into master Mar 27, 2014
@njx njx deleted the randy/issue-7286 branch March 27, 2014 17:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants