Wip/add comment string when clauses

[crispinb]

What has Changed?

• introduced couple of functions to extensions.ts, called from onDidChangeTextEditorSelection & onDidChangeActiveTextEditor subscriptions
• I presume these could be the basis for any further context-type additions, pulled out into a new file
• the approach taken in 'setNewContext' would be unwieldy if you were to add many more contexts, in which case that function might need something fancier
• added a global 'lastContext'. Not sure what else to do there
• left a couple of console.logs in there temporarily - it's hard otherwise to see the context changes (though vscode does have a 'developer inspect context keys' action which helps)

Fixes ##1023

My Calva PR Checklist

I have:

• [x ] Read How to Contribute.
• [x ] Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• [x ] Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
• [x ] Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • [x ] Tested the particular change
• [x ] Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • [x ] If I am fixing the issue, I have used GitHub's fixes/closes syntax

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

• Made sure the PR is directed at the dev branch (unless reasons).
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and tested it there if so.
• Read the source changes.
• Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
• Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse, @bpringe

[bpringe]

Thanks for taking the time on this! I just requested a few small changes, but I also think this code should probably be moved out of extension.ts. I'm not sure where it should go, though. Let's see what @PEZ thinks.

[crispinb]

Thanks - I'll make those changes here (sorry for the clumsy leavings!), but presumably forestall refreshing the PR 'till we hear back from Pez on the structure issue.

[crispinb]

Oops. Pushed to my fork on autopilot. Anyway, those minor changes done.

[PEZ]

Please keep pushing to your fork. We'll treat this as WIP until it's done and it is much easier if we all can look at and test the same thing.

[crispinb]

Yep it's all up to date with @bpringe's requested change

[PEZ]

Looks good to me. The only thing that had me thinking was the withinComment function in the tokenCursor. Two things about it:

1. Does it belong in the TokenCursor? It can be argued it does, but so far it hasn't been needed and the only user is this cursor context janitor. It might be something that should be done there. That means exploiting a bit of the internals of the token cursor (that we know the token type it uses), which is horrible, but something we do all over the code base. (It's a refactoring of its own to use enums or maybe just plain constant strings for that). I'm not sure which way is best here, and I am fine with both. But if this rambling makes you think one or the other is best, @crispinb , then choose whichever you think is best.
2. ”within” is to me different than, say ”adjacent”. The check that is done now checks if it's either within or adjacent.

This second thing then touches the name of the context. I see two ways to deal with it:

1. We name the context something like calva:cursorTouchesComment.
2. We create more contexts for it. calva:cursorTouchesComment, calva:cursorLeftOfComment, calva:cursorInComment, calva:cursorRightOfComment

Option 2 would allow to create keybindings that use structural behaviour in one direction and plain text behaviour in the other direction. Option 1 leaves option 2 open so I would pull this PR regardless which option you choose.

I think a good place to put this code is in a file at the root named when-contexts.ts. Then you can put the config constants in there as well. That config module is another room for Calva refactoring that we should deal with some day. But otherwise we most often just place constants in the modules themselves, and as long as they are well isolated in what they deal with, this seems to work well.

Once we have this in its own module we can get a bit fancier in future PRs. 😄 I've previously been a bit frustrated that I can't read the contexts from the code. So been thinking that maybe some Virtual DOM inspired approach would make sense. But that's for later! Haha.

[crispinb]

A few thoughts about the above @PEZ

• as to where the 'withinComment' function belongs, I'll take your advice on that. You know TokenCursor's raison d'etre in a way I don't (I just raided the existing withinString function, like grabbing a googled SO answer!). So I've moved it out.
• on the 'adjacent/within/touches' comment issue. I'll push back a bit on this, but quite happy to defer if you still don't agree. I think having multiple comment-related contexts, at least now (in advance of being begged for them) is going to confuse matters for users and contributors. It could make for a horrible mess of a shortcuts JSON file, and if we followed that pattern elsewhere the contexts could proliferate madly.
• you're right that the withinComments function checks for being within and 1 token beyond, but that was just to cope with EOLs . If you're editing a line comment, it feels very strange for keybindings to change when the cursor's at a line end (I tried it). How about this: in bae3eff I've make the EOL inclusion more explicit. To the user (if not the token cursor or clojure reader??), a line comment would seem to include the EOL.
• constants also moved out as requested.

As I say, quite happy to revise any of that if you disagree, but I've pushed as-is so you can have a look.

[PEZ]

Sorry for late reply. This PR is going to make Calva a lot better, I think.

About the cursorInComment context. You make valid points. My initial take on it gets messy. However, I still have this concern:

|a|(= (deref an-atom)
     @an-atom|b|)|c|
  |d|;; It's a common mistake to forget to|e| deref|f|
  [g|(|h|first an-atom)|i|
  (first @an-atom)

I just made up a syntax where there are cursor positions, |a|, |b|, etcetera. 😄 I hope it will be possible to follow this. I am thinking that the default keybindings should make things behave liks this:

• |c| forwardSexpr => |i|
• |d| forwardSexpr => |i|
• |d+1| forwardSexpr => |d+2|
• |f| forwardSexpr => |i|
• |d| backwardSexpr => |a|
• |f| backwardSexpr => |e|
• |g| backwardSexpr => |a|

I don't have the time to test this build right now, but I think that it won't allow us to create this behaviour. I do think I know a much less messy way (than my last idea) to achieve it. I could be wrong, but my hunch says that if we have the contexts cursorAtStartOfLine and cursorAtEndOfLine in addition to a version of cursorInComment that doesn't include newline we are home. We'll need to define cursorAtStartOfLine such that |d| is considered start of line, but I think that makes sense. Same for |f| and end of line, if there are trailing spaces. What'ya'say?

FYI, there is a property on the cursor named rowCol that might help you find leading whitespace. And... I think I might have created some code somewhere to get the text of the current line...

[PEZ]

This should be covered with some unit tests, I think. To make that possible we need for the functions under test do not require vscode knowledge, and even live in a module that does not import vscode. Without browsing the code base I am not sure what it entails. But if the non-vscode parts of this lives in the cursor-doc folder. I think the context-figuring functions should work an something implementing EditableDocument (which is defined in cursor-doc/model.ts). So the mirrorDoc you are creating already should work for this. The unit tests have some utility function that can make MockDocuments from a string. Iirc these implement EditableDocuments.

[crispinb]

@PEZ - thanks for spending the time to think that through. I think your cursorAtStartOfLine/cursorAtEndOfLine/cursorInComment notion is a good one. It gets around a central confusion between comments & lines in the current implementation (& yes, you're right that the behaviour you illustrate won't work currently). It's also atomic enough to probably be useful in conjunction with other contexts should they be added in future.

I find it hard to think comprehensively about this kind of thing in the abstract so I'll implement your approach here & try using it in working on some clojure stuff later today.

On unit tests - I haven't had a look through your existing test suite yet. Will do soon & then I'll take note of your comments here.

[crispinb]

(That commit's not quite working as expected yet - still a wip)

[PEZ]

Is it that either the token or the previous token is comment maybe? So both |a| and |b| here would be inComment:

|a|;; foo|b|<nl>

[crispinb]

My mistake - misinterpretation of what we had decided.

[crispinb]

Excuse my absence here - I haven't forgotten about this. I'll be back to it sometime this week.

[crispinb]

Sorry it took a while to get back to this. It's still a wip. 2 remaining issues in addition to anything you spot:

1. still to address unit tests

2. the idea had been to use the new when contexts in customised shortcuts, adding clauses along lines like: (!calva:cursorInComment || calva:cursorAtEndOfLine || calva:cursorAtStartOfLine)
   Unfortunately it turns out this won't work, as (unintuitively!) when clauses don't support parentheses.
   I guess until that's addressed we'll have to supply a few chosen composite when clauses (eg calva:cursorWithinCommentLine, which would be present when calva:cursorInComment is true and calva:cursorAtEndOfLine and calva:cursorAtStartOfLine are both false)?

[crispinb]

2\. we'll  have to supply a few chosen composite when clauses

I've added a POC of this - it adds 2 'compound' where clauses, calva:cursorInCommentExcludingSOL and cursorInCommentExcludingEOL. These can be used in keybindings like the following to achieve the behaviour @PEZ you described above:

{
        "key": "ctrl+left",
        "command": "paredit.backwardSexp",
        "when": "calva:keybindingsEnabled && editorTextFocus && editorLangId == 'clojure' && paredit:keyMap =~ /original|strict/ && !calva:cursorInCommentExcludingSOL"
    },
    {
        "key": "ctrl+left",
        "command": "-paredit.backwardSexp",
        "when": "calva:keybindingsEnabled && editorTextFocus && editorLangId == 'clojure' && paredit:keyMap =~ /original|strict/"
    },
    {
        "key": "ctrl+right",
        "command": "paredit.forwardSexp",
        "when": "calva:keybindingsEnabled && editorTextFocus && editorLangId == 'clojure' && paredit:keyMap =~ /original|strict/ && !calva:cursorInCommentExcludingEOL"
    },
    {
        "key": "ctrl+right",
        "command": "-paredit.forwardSexp",
        "when": "calva:keybindingsEnabled && editorTextFocus && editorLangId == 'clojure' && paredit:keyMap =~ /original|strict/"
    },

I haven't been doing much clojure lately so this is imperfectly tested as yet.

It's outdated, right?

[PEZ]

Looks great. I will be testing it today. I'm starting to think the default key bindings should be like the examples you have there.

[PEZ]

Whoo, feels great! There are two quirky things I notice

1. forward from the end of line of a comment, jumps to the end of next sexpr, but backwards from there then jumps to the start of the previous sexpr. I don't think this is wrong, or anything we can really do anything about as long as we don't want to always step through comments. (Which I don't think I want.)
2. Jumping from the end of a comment line that has further comment lines, jump to the next sexpr. My brain expects it to jump past the next word on the next comment line. I think this can be achieved by defining inComment as being in a block of comment lines. So if at a comment-EOL and the next non-whitespace line is also a comment, then we are inComment.

I'll be testing this further today, as I am writing my dram thing mostly in comments I happen to be the perfect tester. 😄

[crispinb]

Thanks for checking that out @PEZ . I had the same reaction re your 2nd point. It's all a bit semantically weird really -in effect kind of jumping between different languages. I haven't played around with the results much yet, but there's scope for using the new contexts with other keybindings (deletes etc).

Anyway let me know what else you come up with. I should be able to have a crack at some fixes/changes Mon and/or Tue.

[PEZ]

Yeah, totally interesting problem with the two languages. I like that with your work here we stop just ignoring that there are languages within the language, even if we'll have some boundary cases left (like my first observation above).

Also agree that the contexts will be of broader use. I've already added that for deletion on my branch. alt+delete is second nature to me, but it is weird to have to do that inside non-structured text

Btw, if I push that branch, will you be able to use that then? I've resolved the merge conflicts too. Might just try it and see what happens. Apologies in advance if it messes things up for you.

[PEZ]

Seems it just became a regular branch. You can pull it in, if you like: https://github.com/BetterThanTomorrow/calva/tree/crispinb-wip/add-comment-string-when-clauses

[bpringe]

alt+delete is second nature to me, but it is weird to have to do that inside non-structured text

I just came across this (again?) recently. Glad we have this work being done by @crispinb!

[PEZ]

I can pick this up from here if you have run out of time to spend on it, @crispinb . It seems close to finished and it is good stuff!

[crispinb]

Hey @PEZ - I did drop the ball a bit there (apologies - I don't want to be the annoying disappearing contributor). I won't get back to it for at least a week so please feel free if you have the time & inclination. If you do I'll see if there's something else I can pick up. Either way works for me.

[PEZ]

Haha, don't worry. You are not an annoying contributor just because you have a life. 😄

We'll see what the week brings. Today I realised I need your changes for fixing an issue, so if I find myself fixing that issue I will first finish this work. You'll see when you come back if the work is still waiting for you.

[PEZ]

Continuing this in #1110. Please have a look, @crispinb and @bpringe . It has been a lot of late night hacking on this and mistakes might have happened.