Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Hi, I'm new. #1312

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

So, I'm not really a javascript programmer. I downloaded light table, and saw they had vim bindings. I've been using vim/screen for like 15 years and thought it'd be nice to try something new. First thing I did was open a file and hit ctrl-d to skip through it. Nothing. I monkeypatched the code locally so I could test out functionality. I let Chris Granger know and he advised me to clone Codemirror and apply my patch.

He is using a slightly older version, so I had to patch my code slightly to make it work with your latest. Thanks for implementing ".". That was driving me nuts too, and I was going to fix that next.

Anyway, ctrl-u/d are both custom commands, you should be able to specify the number of lines you want to scroll, but if it's not set it defaults to a half page. I set it up that way, storing the custom value in the global configuration. I did overload your concept of "unit" which you may not like for findPosV.

So, first use of github, first clone, first checkin and first pull request, let me know if it looks ok. Oh and all the existing tests passed, if you want me to write a test, I will.

Ryan Dietrich Added custom scrolling.
ctrl-u and ctrl-d are implemented.
5a8fc68
Owner

marijnh commented Mar 4, 2013

@mightyguava Could you take a look?

Contributor

metatheos commented Mar 4, 2013

@dextius Great to see so many vim users wanting to contribute to improve the keymap. I'm just passing by myself, so my opinion is not as relevant as that of others here, but I'll still throw a few suggestions in.

I'm quite sure you don't really need to touch the core here. Your use-case is probably also not too useful to non-vim-users of codemirror, even less reason to inject it into the core as a hack. If you really want to add it to the core, than maybe by making findPosV accept doubles for amount, but I don't know if this is a good path to take.
What you might want instead is to call cm.getScrollInfo() (see here) and use the result to get a cursor half a screen from the current one. Something like:

moveByScroll: function(cm, motionArgs,vim) {
  var gvim=getVimGlobalState();
  //motionArgs.repeatIsExplicit seems to be set to false on implicit 1, but not (always) to true on explicit repeat
  var repeat = (motionArgs.repeatIsExplicit===false) ? gvim.customScroll : gvim.customScroll=motionArgs.repeat ;
  var scrollbox=cm.getScrollInfo();
  var curEnd=null;
  if(repeat){
    //move cursor by specified range and restore vertical offset
    var orig=cm.charCoords(cm.getCursor(),"local");
    motionArgs.repeat=repeat;
    curEnd=motions.moveByDisplayLines(cm,motionArgs,vim);
    var dest=cm.charCoords(curEnd,"local");
    cm.scrollTo(null, scrollbox.top+dest.top-orig.top);
  }else{
    //no customScroll set and no repeat given, scroll by half-page
    var delta = (motionArgs.forward ? 1 : -1) * (scrollbox.clientHeight/2);
    var orig=cm.charCoords(cm.getCursor(),"page");
    curEnd=cm.coordsChar({left:orig.left+2, top:orig.top+delta+3});
    cm.scrollTo(null, scrollbox.top+delta);
  }
  return curEnd;
},

might work, but is not too robust in this shape. Also the global vim-state would have to listen to resize events and reset customScroll. And once there is some error-handling in place it would probably have to report 'out of document'.

Contributor

mightyguava commented Mar 4, 2013

Took a look at the VIM documentation. It looks like Ctrl-U/D should not support repeat. The amount they are scrolled by is configured with the 'scroll' option. Unfortunately I haven't gotten around to adding options to the keymap.

Nevertheless I agree with @metatheos that you should not be touching the core for this. @metatheos's approach is a pretty good start. I would compute the number of lines equivalent to half a page (this might take a little work) and feed that value into moveByDisplayLines. No need for the if/else block since half-page is being used as the default; in the future when options are added we can just override the value.

Contributor

metatheos commented Mar 4, 2013

By my understanding vim-doc says that repeat sets the value of 'scroll' (stored in gvim.customScroll above). And that it is reset on every resize. I chose to make the default value a special case, since -- in contrast to vim -- in codemirror the height of lines can vary. The alternative should be retrieving it from cm.defaultTextHeight() either on resize or in if(!repeat). Since vim deviated from vi by counting display lines instead of lines, my interpretation of the default value was 'scrollbox.clientHeight/2 pixels' instead of 'scrollbox.clientHeight/(2*cm.defaultTextHeight()) display lines' (though I should better move the bottom edge and not the upper). In the alternative case it should be possible to simplify the if-block to

if(!repeat) repeat=scrollbox.clientHeight/(2*cm.defaultTextHeight());
//original true-branch from here:
var orig=cm.charCoords(cm.getCursor(),"local");
motionArgs.repeat=repeat;
var curEnd=motions.moveByDisplayLines(cm,motionArgs,vim);
var dest=cm.charCoords(curEnd,"local");
cm.scrollTo(null, scrollbox.top+dest.top-orig.top);
return curEnd;
Contributor

mightyguava commented Mar 4, 2013

Ah I see. I apologize for my confusion. I did not look at scroll and missed its special behavior. @metatheos's original suggestion sounds more right then, though for the default case of half page scrolling, the vertical offset needs to be restored too.

Personally I wouldn't mind if you go with @metatheos's second suggestion either as it would be much cleaner...

Contributor

metatheos commented Mar 5, 2013

You're right, it would be more robust to set the scrolling based on the new cursor-position. Just adding scrollbox.clientHeight/2 as I did in the first example might introduce some drifting. Another thing in my example that needs to be checked is how it deals with clipping, namely how to make sure the cursor recovers the way it should.

Also the point of (not) supporting option-setting made me think of registering

CodeMirror.Vim.defineEx('cmset','cmse',function(cm,params){
    var match=params.argString.match(/^\s*(\S+)\s*=\s*(.+)/);
    if(match){
        var option=match[1];
        var value=JSON.parse(match[2]);
        cm.setOption(option,value);
    }
});

after adding some safeguards for invalid values all over the place.

Owner

marijnh commented Mar 5, 2013

after adding some safeguards for invalid values all over the place.

CodeMirror itself will simply crash when given options of invalid types. It will guard against reasonable things, like passing out-of-range positions to public methods, but I don't think adding kilobytes of code (and run-time overhead) to check for bogus inputs is a good trade-off.

Contributor

metatheos commented Mar 5, 2013

Sorry for the confusion, I didn't mean adding any kind of check to the actual setOption or the option-callbacks, 'all over the place' was misleading in that regard. It's not even about guarding against anything malicious or type mismatch. I meant catching JSON-parse exceptions, verify that the option actually exists and generally check if there is anything given at various points in the function above, but nothing really complex.

Contributor

mightyguava commented Mar 5, 2013

One of the considerations I had for a set Ex command is that we would need some kind of object to manage global and local options, both of which are settable at runtime. I think cm.setOption can only set document local options. Do we not have a need for global Vim options?

Owner

marijnh commented Mar 28, 2013

@dextius Will you be updating you patch to address @mightyguava 's concerns?

If not, @mightyguava , is it easy to salvage parts of this, or shall I just close the pull request?

Contributor

mightyguava commented Mar 28, 2013

I think @metatheos's suggestions are reasonable. I will draft up a patch for it Monday if @dextius doesn't before then.

Had a new baby last week. Free time is currently less than negative
infinity.
I just wanted to help, sorry I didn't have the context needed to be of use.

On Thu, Mar 28, 2013 at 8:35 AM, Yunchi Luo notifications@github.comwrote:

I think @metatheos https://github.com/metatheos's suggestions are
reasonable. I will draft up a patch for it Monday if @dextiushttps://github.com/dextiusdoesn't before then.


Reply to this email directly or view it on GitHubhttps://github.com/marijnh/CodeMirror/pull/1312#issuecomment-15591714
.

Contributor

mightyguava commented Mar 29, 2013

I just wanted to help, sorry I didn't have the context needed to be of use.

Np, glad to see enthusiasm about the keymap. Your change is small enough that I should be able to piece it together without too much work.

Congrats on the baby!

Contributor

mightyguava commented Apr 2, 2013

mightyguava/CodeMirror@8435720 implements custom scrolling in the fashion discussed with @metatheos. I also tweaked moveByDisplayLines a bit for edge cases. @marijnh let me know if I should open a separate pull request.

I think there may also be a bug with cm.coordsChar. In an editor instance with a single wrapped line, calling cm.coordsChar(cm.charCoords(pos), 'div'), 'div') with pos being on the second row, I get the corresponding position on the first row. replacing 'div' with 'local' always gets me ch: 0, line: 0. Or it's quite possible I'm not using the arguments properly? Currently arbitrarily using an offset of 8 pixels to compensate in the commit.

Owner

marijnh commented Apr 2, 2013

Wonderful. I've pushed your patch as f0d055b

@marijnh marijnh closed this Apr 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment