Conversation
pokey
left a comment
There was a problem hiding this comment.
seems like a good implementation approach. left some comments. also, please add some tests with "next" / "previous", and be sure to test boundaries on edges of scopes and between constituent scopes
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Show resolved
Hide resolved
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-engine/src/scopeProviders/ScopeInfoProvider.ts
Outdated
Show resolved
Hide resolved
…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
…andlers/ContiguousScopeHandler.ts Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ContiguousScopeHandler.ts
Outdated
Show resolved
Hide resolved
|
@pokey @josharian The current implementation now only effects the command scope type and there is no spoken form for it. |
| return true; | ||
| } | ||
|
|
||
| const [startTarget, endTarget] = getTargetsInDocumentOrder( |
There was a problem hiding this comment.
This file is now general enough to be used with most scopes. If we wanted to we could remove a bunch of code here and rely on the fact that we only use it for comments. I could go either way.
|
Won't this cause adjacent multiline comments to be merged as well? eg /*
* foo
*/
/*
* bar
*/Looking at this, it feels like not a particularly big deal 😅. But figured I'd point it out as I don't think it quite matches what we discussed |
|
Yes unless we make this more comment specific and check for single line ranges. Ideally we would have single and multi-line as separate scopes and only use this for single line. Or add a new predicate for contiguous scope. That is probably the best solution? |
|
hmm so the predicate would just set a flag on the capture. I think that sounds reasonable? |
|
Yes exactly. |
|
There is no problem really with this implementation except for the fact that in javascript there is no Tree sitter difference between block and line comments. I guess that contiguous predicate could take a boolean that requires single line ranges? Or a regex pattern? edit: Solved with a regex pattern |
|
|
||
| /** | ||
| * Returns true if the given scope type should use a contiguous scope handler. | ||
| */ | ||
| function useContiguousScopeHandler(scopeType: ScopeType): boolean { | ||
| switch (scopeType.type) { | ||
| case "comment": | ||
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
I thought you were using a query predicate for this now
There was a problem hiding this comment.
We use both. I don't know how to check it at this stage. When we actually fetch the matched scope it's to late to inject this handler. Not without a lot of rewrites at least.
There was a problem hiding this comment.
Sorry I'm not following how you would use both. When do you use one vs the other?
There was a problem hiding this comment.
This code decides to use the handler. Then the scope.contiguous is used to determine when to merge two scopes. That way we can ignore block comments.
Fixes #1835
Now implemented as a modifier of the comment scope
I decided to implement this as a scope type instead of a modifier. Now you can say things likechuck fat commentorchanged next fat alpha. This behavior of having alternatives to existing scopes is already something we started withBoundedNonWhitespaceSequenceStagealbeit in a less capacity.Checklist