Allow 80 characters line rule in CodeMirror

[sorawee]

I saw students in 19 write this comment.

# 34567890123456....              (80 characters) ... 90

And make sure that no line goes beyond that because TA would deduct points. This is extremely annoying. It would be nice if we simply allow an option for CodeMirror to show the line warning if any line is longer than 80 characters (or a specified number of characters)

[blerner]

👍 I've wanted to implement this several times, but haven't figured out a convenient way to do so.

[schanzer]

Here's an inefficient implementation. (A smarter one would use the changes object to extract the start and end range of any changes, then pass those to limit eachLines work.)

// given a line handle, set the background of the line to the "tooLong" CSS class
editor.on("changes", function(cm, changes) {
  editor.eachLine(line => {
    if(line.text.length > 80) editor.addLineClass(line, "background", "tooLong");
  });
});

This will assign a class to the background of every line that's too long, but you could just as easily insert a lineWidget, assign a class to the text of the line, etc.

[sorawee]

Alternatively just add https://codemirror.net/demo/rulers.html. We would want it to be adjustable though.

[blerner]

I knew there had to be a plugin that did this for us :)  Let's use that, and use part of Emmanuel's code to turn the warning line on or off depending on whether any lines are overly long, like DrRacket does,

This could be a good first-bug candidate for anyone who hasn't dived in deeply into CPO or pyret-lang codebases yet.  Any takers?

[YanyanR]

I can take a stab. This only involves CPO, not Pyret-lang, right? Cuz it’s just adding a cm plug-in and modifying a css object?

[sorawee]

That's correct! (You might need to modify js file to add Emmanuel's suggestion, but all of these are in CPO)

[YanyanR]

Sorry about the delay. I'm done with adding the vertical ruler (see 79fb839). But I'm having some trouble with highlighting long line. The format is not changing. addLineClass should take in a line and add a css class to it. In cpo-main.js, I added

      editor.cm.on('changes', cm => {
        cm.doc.eachLine(
          line => {
          if(line.text.length > 80) {cm.doc.addLineClass(line, "background", "tooLong")};
        });
      });

and in editor.css, I added

.toolong {
  background: #add8e6 !important; 
  /*!important to override previous format*/
}

I also tried to use markText, since we want to highlight only part of the line. But I couldn't figure out how to get line (line number) for the {line, ch} object.

Any suggestion on which method to use here? Or what I did wrong with addLineClass?

[blerner]

I don't see the code you added to cpo-main.js in the commit you mention, so either you forgot a bit in your commit, or the code isn't actually present and being run... :)

Three other things: (1) I think you need the inverse operation also, to removeLineClass if the lines get short enough again. (2) It would be great to make this configurable, such that the number 80 isn't hardcoded into the set of rulers you construct. (3) It would be even cooler if we could get the rule to appear only when the first overly-long line appears.

[YanyanR]

Yeah I left that out because I want to separate the working code from the failing one. The commit is only about the ruler. (1) I still think markText is a better function to use here, because it takes in ch and can affect only part of the line, while addLineClass and removeLineClass affect the entire line. (2) Could you explain configurable more? Where should I put 80 then? (3) I thought having the ruler be there at the start might be nice, because then the user will know how large their window should be - they can just drag the middle bar until they see a ruler.

[blerner]

If a line is too long, I think of that as a property of the line, not a property of just the suffix that's overly long.

Figuring out where the 80 goes is interesting. I don't quite understand the rulers.js plugin enough yet to know when it gets called, and whether we can cause it to refresh with new ruler locations, or whether it just gets called once at the initialization of the CM instance and never updates after that.

IMO, having a margin line all the time is distracting, and having it appear when needed is ideal. Others like it as a constant reminder. So this seems like something that deserves to be a CPO user option, somehow. (And so should the 80!) We don't have a coherent story yet about user preferences in CPO (e.g. where are they saved, what are they), so I don't have a specific answer for you yet.

[YanyanR]

(1) Oh okay, so we want to highlight the entire line if it's too long, not just the part that's over 80 character. (2) I thought rulers work as the later (called once during initialization), but looks like there's a newer version of rulers.js that contains a clearRuler method: old/currently using vs. new. I'll try using the new one later, and see if I can refresh with new ruler location. (3) Does it help if we set the color to be less obvious, like gray? atom IDE has a gray line at 80 all the time. I also thought about having a button or keyboard shortcut to turn the ruler on. Would that be better?

[blerner]

(2) Careful -- that code is for an ancient version of CodeMirror -- we're on v5.2.4, and that one is from v3. There's no guarantee that it will work with the current version of CM. The actual current version looks like it does wipe out and refresh existing rulers on each refresh event (where it says cm.state.rulerDiv.textContent = ""), so the current version should work. (3) maybe. let's just get something working and implemented, and then Joe and I can figure out the engineering we want to make it customizable/configurable or whatever

[jpolitz]

I'm using pyret-horizon.herokuapp.com right now, and I think I lost the scrollbar on the editor to some change related to this. I see the dotted line, but no clickable scrollbar. I can only scroll with the mouse wheel.

[blerner]

I can't reproduce that lack-of-scrolling that you saw. I did commit https://github.com/brownplt/code.pyret.org/commit/d18c15c9a15aee095d94b99b79a01bb7537d5711, to add DrRacket-style dynamic rulers depending on whether any text is overly long. I don't know how well this will work yet, just making a note of it here.