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

Add missing param#6770

Merged
njx merged 2 commits intocmv4from
randy/cmv4-issue-6718
Feb 5, 2014
Merged

Add missing param#6770
njx merged 2 commits intocmv4from
randy/cmv4-issue-6718

Conversation

@redmunds
Copy link
Copy Markdown
Contributor

@redmunds redmunds commented Feb 5, 2014

This is for #6718

cc @njx

@njx
Copy link
Copy Markdown

njx commented Feb 5, 2014

I'm not sure this is right. I think the Boolean(rev) is intended to be the first argument to getCursorPos() (the second argument is new and optional). But I'm actually not sure why it would pass true in the original code for Find Previous, since I would assume we always want to use the real cursor position, not the one that expands tabs.

@njx
Copy link
Copy Markdown

njx commented Feb 5, 2014

It does seem to fix it, though...but I don't understand why. Looking...

@njx
Copy link
Copy Markdown

njx commented Feb 5, 2014

(Edited for clarity...)

Oh, I see. It was the "which end of the selection" argument to the original CodeMirror getCursor(), but we changed this to Editor.getCursorPos(), which takes that as the second argument.

Two things to change though:

  • CM doesn't document using true/false for that arg, and we don't either, so we should pass "start" and "end" instead (i.e., rev ? "start" : "end").
  • The first argument you added should be false, not true (we want it without tab expansion, I think - true should only be used for displaying the column number to the user; internally tabs should just be considered one character when dealing with CM positions).

@njx
Copy link
Copy Markdown

njx commented Feb 5, 2014

Just edited my previous comment to clarify it (hopefully)...

@redmunds
Copy link
Copy Markdown
Contributor Author

redmunds commented Feb 5, 2014

Changes pushed. I realized that the expand tabs arg was wrong while I was at lunch. I originally had that change for the other param, but figured I'd minimize the change -- I like it more explicit like this.

@njx
Copy link
Copy Markdown

njx commented Feb 5, 2014

Looks good. Can't tell if it breaks any unit tests since there are a bunch that were already broken :), but seems fine.

njx pushed a commit that referenced this pull request Feb 5, 2014
@njx njx merged commit 5ce3f1a into cmv4 Feb 5, 2014
@njx njx deleted the randy/cmv4-issue-6718 branch February 5, 2014 21:44
@njx
Copy link
Copy Markdown

njx commented Feb 5, 2014

Oops, I forgot that we wanted to use "from" and "to" instead of "start" and "end" here (both work, but "from" and "to" are the official ones listed in the CM docs). I'll fix that later.

@njx
Copy link
Copy Markdown

njx commented Feb 5, 2014

Ignore that last comment, we're keeping "start" and "end".

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.

2 participants