implement "visible" scope #1623

josharian · 2023-07-13T00:13:50Z

Some progress! But definitely not working yet.

Very much :confused_dog: here. Would love some help,
ideally live at the next community meeting that I can attend.
No need to look at this in advance.

Things to discuss (notes to self):

seems like containment = null is correct for "visible"
scope, and it seems like that's what the code says,
but...vscode begs to differ. halp?
- we yield multiple ranges but only one gets used
- if primary cursor is offscreen, we get an error
  indicating that the range is not found
definition of visible: i propose we leave it up to
VSCode, which mean does not include folded or partially
visible lines. the value of visible is that it does
exactly what it says on the tin.
testing strategy: use lots of newlines?
did I find all the places that need updating?

Fixes #1607

pokey

Wow great start! Tbh it's almost mergeable as is. But see comment inline

pokey · 2023-07-13T08:49:09Z

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/VisibleScopeHandler.ts

+    _position: Position,
+    _direction: Direction,
+  ): Iterable<TargetScope> {
+    for (const range of editor.visibleRanges) {


This will interact with folding in an interesting way. If you say "take visible" from one side of a fold, you'll only get up until the fold. If you say "take next visible" you'll get the code between the next fold until the following. To refer to all sections between folds, you'd say "take every visible".

I'd be tempted to either

yield a single scope whose range is the union all the visible ranges, or

yield a single scope that returns all the visible regions as a single list in its getTargets. I'd consider making the domain of that scope just be the document. Then saying "take visible" from anywhere in the document will give you all the visible ranges

There's a third option, which is to reject visible as an allowed scope when folds are present. That sidesteps the question of what the correct behavior is, and punts the decision to the future, where we could loosen that constraint.

I lean towards your second option, and will implement that to play with, but am open to guidance. :)

Implemented, played with second option. I'm happy with it. Another use case: fixing variable shadowing. Fold all shadowed inner scopes, work on every instance in what remains visible. (You can do that with take and give and this and that, but folding is more stateful, which can be useful.)

Although that makes me wonder whether visible should be a modifier.

from visible file take every instance air
from visible func fine take every instance air

Nah, probably not.

If you revert to your original approach, then you could say "from every visible funk" to target all visible ranges that touch the current function. Not necessarily suggesting you do that, but just fwiw

That's true! But on balance, I think I prefer the simpler semantics. I'm happy with what is implemented now.

Brain dump:

This is the first example of a scope that's not purely a function of the document itself

Being a scope type here doesn't buy us anything useful over just making it a modifier (eg we can't leverage "every visible", "next visible", etc even tho the grammar now allows them). However that's also true or document scope ("file")

Having it be a mark would preclude ability to say "visible air" to target visible in another split, so we'd prob want it to at least be a modifier

Having it be modifier with intersection as you proposed is interesting but might be unintuitive when you have a nonempty selection and say "take visible": that would then intersect your selection with visible

it might actually be useful to say "next visible" / "first visible" etc. I think the latter does work with your impl fwiw

There is an interesting parallel here to a hypothetical scope for referring to your selections, eg "first selection", or "selection air" to refer to the selection in the split containing air

pokey · 2023-07-13T08:56:06Z

seems like containment = null is correct for "visible"
scope, and it seems like that's what the code says,
but...vscode begs to differ. halp?

What do you mean by containment = null?

we yield multiple ranges but only one gets used

if primary cursor is offscreen, we get an error
indicating that the range is not found

definition of visible: i propose we leave it up to
VSCode, which mean does not include folded or partially
visible lines. the value of visible is that it does
exactly what it says on the tin.

This would be my second proposal in my inline review comment

testing strategy: use lots of newlines?

That's a tough one. We struggle to test visible range stuff because we don't know how big the window is. Probably the best way to test this would be to leverage the fact that the editor is actually an abstract interface, but we haven't done anything like that yet. We will in the future, but I'd be tempted to punt on that. I'd probably just have a test with and without folds and declare victory

did I find all the places that need updating?

I think so. If the test passes you're probably good. In fact you were more thorough than we usually are 😅

pokey · 2023-07-13T09:00:30Z

~~Come to think of it, this probably shouldn't be a scope type at all. I think it probably makes more sense as a mark~~ Discussed with @AndreasArvidsson at meet-up and scope type makes sense, because it's analogous to document, and can be used as an iteration scope if we ever want to

josharian · 2023-07-13T14:44:06Z

What do you mean by containment = null?

You can ignore. I mistakenly thought that the way to return a disjoint/union range was by yielding each of its pieces. Then I noticed that it behaved wrong, tried to figure out why, in my flailing found docs indicating that the wrong containment scope iterator setting would potentially explain the behavior I saw, and assumed that that was wrong.

pokey · 2023-07-13T15:56:54Z

You can ignore. I mistakenly thought that the way to return a disjoint/union range was by yielding each of its pieces. Then I noticed that it behaved wrong, tried to figure out why, in my flailing found docs indicating that the wrong containment scope iterator setting would potentially explain the behavior I saw, and assumed that that was wrong.

Sorry, I didn't understand anything you said after "You can ignore". But I'm happy to leave it there 😄

josharian · 2023-07-13T19:40:17Z

Thanks for all the help. PTAL. Note the new commit message.

josharian · 2023-07-13T19:50:07Z

Realized I failed to add this to the docs. Can't figure out in which section it belong, which is not a good sign. 😅 WDYT?

We rely on VSCode's notion of visibility. Notable, folded code is not considered visible. The testing story is sad. There are two tests that I would like to have included. * Folded code. Unfortunately, the test runner does not recreate initial folded state, and it runs only one command at a time, so you can't fold-then-change. My attempts to upgrade the test runner to recreate initial folded state failed, and given that the test file format is changing anyway, it is probably not a good investment. * Offscreen content. This is the raison d'etre of the visible scope. Unfortunately, we don't know how large the viewport will be so there is no way to write a simple test using the existing infrastructure that will execute correctly. Fixes cursorless-dev#1607

auscompgeek · 2023-07-16T11:55:38Z

docs/user/experimental/keyboard/modal.md

    "ss": "boundedNonWhitespaceSequence",
    "sa": "argumentOrParameter",
    "sl": "url",
+    "vz": "visible",


The v key is already used for the curve shape below, so this will need to use a different key sequence.

auscompgeek · 2023-07-16T11:57:52Z

cursorless-talon/src/modifiers/scopes.py

    "short paint": "boundedNonWhitespaceSequence",
    "link": "url",
    "cell": "notebookCell",
+    "visible": "visible",


visible is being added next to document in other places. The same should be done here for consistency.

pokey · 2023-08-02T10:39:36Z

If I recall correctly, the decision was to implement this one as a modifier for now? Or just close until we understand it better? I'm fine either way

josharian · 2023-08-02T12:40:36Z

yep, we're going to try it as a modifier. Still in my queue!

AndreasArvidsson · 2023-12-05T13:33:51Z

@josharian You want me to take this one? Implementing this as a modifier should be easy.

josharian · 2023-12-05T14:07:15Z

@AndreasArvidsson great! That’d be lovely.

AndreasArvidsson · 2023-12-05T14:40:49Z

@josharian Your wish is my command ;)
#2094

josharian · 2023-12-05T14:51:13Z

@AndreasArvidsson sudo make me a sandwich!

josharian · 2023-12-05T14:51:45Z

Breaking news, ChatGPT has no sense of humor.

josharian · 2023-12-05T14:54:33Z

Closing this. Thanks again, Andreas.

AndreasArvidsson · 2023-12-05T14:55:31Z

You're very welcome :)

pokey reviewed Jul 13, 2023

View reviewed changes

josharian force-pushed the josh/visible branch from 2e7988d to 031cb67 Compare July 13, 2023 19:38

josharian marked this pull request as ready for review July 13, 2023 19:39

josharian force-pushed the josh/visible branch from 031cb67 to a17eb86 Compare July 13, 2023 19:51

auscompgeek reviewed Jul 16, 2023

View reviewed changes

pokey marked this pull request as draft July 18, 2023 15:52

josharian closed this Dec 5, 2023

Uh oh!

implement "visible" scope #1623

implement "visible" scope #1623

Uh oh!

Conversation

josharian commented Jul 13, 2023

Uh oh!

pokey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pokey Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pokey commented Jul 13, 2023

Uh oh!

pokey commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josharian commented Jul 13, 2023

Uh oh!

pokey commented Jul 13, 2023

Uh oh!

josharian commented Jul 13, 2023

Uh oh!

josharian commented Jul 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pokey commented Aug 2, 2023

Uh oh!

josharian commented Aug 2, 2023

Uh oh!

AndreasArvidsson commented Dec 5, 2023

Uh oh!

josharian commented Dec 5, 2023

Uh oh!

AndreasArvidsson commented Dec 5, 2023

Uh oh!

josharian commented Dec 5, 2023

Uh oh!

josharian commented Dec 5, 2023

Uh oh!

josharian commented Dec 5, 2023

Uh oh!

AndreasArvidsson commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pokey left a comment •

edited

Loading

pokey Jul 13, 2023 •

edited

Loading

pokey commented Jul 13, 2023 •

edited

Loading