Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contiguous scope #2101

Draft
wants to merge 41 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
fdcd03e
Started working on contiguous scope modifier
AndreasArvidsson Dec 5, 2023
f223d84
Merge branch 'main' into contiguous_scope
AndreasArvidsson Dec 5, 2023
b549ca5
more work
AndreasArvidsson Dec 5, 2023
a71f017
Change continuous scope modifier into continuous scope type
AndreasArvidsson Dec 6, 2023
da8d6ec
Added tests
AndreasArvidsson Dec 6, 2023
a3fa351
[pre-commit.ci lite] apply automatic fixes
pre-commit-ci-lite[bot] Dec 6, 2023
53351e8
Moved tests
AndreasArvidsson Dec 6, 2023
d370690
Added comment
AndreasArvidsson Dec 6, 2023
48f97ee
cleanup
AndreasArvidsson Dec 6, 2023
e2ec5c8
more cleanup
AndreasArvidsson Dec 6, 2023
d1020a8
Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…
AndreasArvidsson Dec 6, 2023
02a295e
Import
AndreasArvidsson Dec 6, 2023
9d75ead
Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…
AndreasArvidsson Dec 6, 2023
48c7a65
Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…
AndreasArvidsson Dec 6, 2023
fa57ab2
Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…
AndreasArvidsson Dec 6, 2023
c943542
Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…
AndreasArvidsson Dec 6, 2023
c26d704
Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…
AndreasArvidsson Dec 6, 2023
e0b6b5b
[pre-commit.ci lite] apply automatic fixes
pre-commit-ci-lite[bot] Dec 6, 2023
2988adb
rename
AndreasArvidsson Dec 6, 2023
c2f942b
Merge
AndreasArvidsson Dec 6, 2023
34cbbb1
update
AndreasArvidsson Dec 6, 2023
8b5a95f
Use proximal and distal
AndreasArvidsson Dec 7, 2023
bd86a64
Continues target
AndreasArvidsson Dec 7, 2023
5a905ea
Added tests
AndreasArvidsson Dec 7, 2023
6612818
cleanup
AndreasArvidsson Dec 7, 2023
2852e37
Merge branch 'main' into contiguous_scope
AndreasArvidsson Dec 7, 2023
d3eb687
clean up
AndreasArvidsson Dec 8, 2023
fe7e8e5
Merge branch 'contiguous_scope' of github.com:cursorless-dev/cursorle…
AndreasArvidsson Dec 8, 2023
2f15dc6
Only do contiguous for comments
AndreasArvidsson Dec 14, 2023
0ccdcc6
cleanup
AndreasArvidsson Dec 14, 2023
fd93230
Merge branch 'main' into contiguous_scope
AndreasArvidsson Dec 14, 2023
d898f26
cleanup
AndreasArvidsson Dec 14, 2023
ea5a542
Update test
AndreasArvidsson Dec 14, 2023
66731e4
Added predicate
AndreasArvidsson Dec 15, 2023
58bfb9a
Merge branch 'main' into contiguous_scope
AndreasArvidsson Dec 15, 2023
b6ac60f
Added comment facets
AndreasArvidsson Dec 15, 2023
17b0507
Remove pattern argument from contiguous predicate
AndreasArvidsson Feb 26, 2024
6f0de5c
Merge branch 'main' into contiguous_scope
AndreasArvidsson Feb 26, 2024
9f156a6
Update scope fixtures
AndreasArvidsson Feb 26, 2024
38ab206
Merge branch 'main' into contiguous_scope
AndreasArvidsson Jun 14, 2024
883eb1e
[pre-commit.ci lite] apply automatic fixes
pre-commit-ci-lite[bot] Jun 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions cursorless-talon/src/modifiers/scopes.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from typing import Any

from talon import Module

mod = Module()
Expand All @@ -12,28 +14,45 @@
"cursorless_custom_regex_scope_type_plural",
desc="Supported plural custom regular expression scope types",
)
mod.list(
"cursorless_contiguous_scope_type",
desc="Cursorless contiguous scope type",
)


@mod.capture(
rule="{user.cursorless_scope_type} | {user.cursorless_custom_regex_scope_type}"
rule="[{user.cursorless_contiguous_scope_type}] ({user.cursorless_scope_type} | {user.cursorless_custom_regex_scope_type})"
)
def cursorless_scope_type(m) -> dict[str, str]:
def cursorless_scope_type(m) -> dict[str, Any]:
"""Cursorless scope type singular"""
try:
return {"type": m.cursorless_scope_type}
scope_type = {"type": m.cursorless_scope_type}
except AttributeError:
return {"type": "customRegex", "regex": m.cursorless_custom_regex_scope_type}
scope_type = {
"type": "customRegex",
"regex": m.cursorless_custom_regex_scope_type,
}

try:
return {"type": m.cursorless_contiguous_scope_type, "scopeType": scope_type}
except AttributeError:
return scope_type


@mod.capture(
rule="{user.cursorless_scope_type_plural} | {user.cursorless_custom_regex_scope_type_plural}"
rule="[{user.cursorless_contiguous_scope_type}] ({user.cursorless_scope_type_plural} | {user.cursorless_custom_regex_scope_type_plural})"
)
def cursorless_scope_type_plural(m) -> dict[str, str]:
def cursorless_scope_type_plural(m) -> dict[str, Any]:
"""Cursorless scope type plural"""
try:
return {"type": m.cursorless_scope_type_plural}
scope_type = {"type": m.cursorless_scope_type_plural}
except AttributeError:
return {
scope_type = {
"type": "customRegex",
"regex": m.cursorless_custom_regex_scope_type_plural,
}

try:
return {"type": m.cursorless_contiguous_scope_type, "scopeType": scope_type}
except AttributeError:
return scope_type
1 change: 1 addition & 0 deletions cursorless-talon/src/spoken_forms.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
"visible": "visible"
},
"simple_scope_modifier": { "every": "every" },
"contiguous_scope_type": { "fat": "contiguous" },
"interior_modifier": {
"inside": "interiorOnly"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,17 @@ export interface OneOfScopeType {
scopeTypes: ScopeType[];
}

export interface ContiguousScopeType {
type: "contiguous";
scopeType: ScopeType;
}

export type ScopeType =
| SimpleScopeType
| SurroundingPairScopeType
| CustomRegexScopeType
| OneOfScopeType;
| OneOfScopeType
| ContiguousScopeType;

export interface ContainingSurroundingPairModifier
extends ContainingScopeModifier {
Expand Down
14 changes: 14 additions & 0 deletions packages/common/src/util/itertools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,17 @@ export function isEmptyIterable(iterable: Iterable<unknown>): boolean {

return true;
}

/**
* Returns the first element of the given iterable, or `undefined` if the
* iterable is empty
* @param iterable The iterable to get the first element of
* @returns The first element of the iterable, or `undefined` if the iterable
* is empty
*/
export function next<T>(generator: Iterable<T>): T | undefined {
for (const value of generator) {
return value;
}
return undefined;
}
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,12 @@ export class PrimitiveTargetSpokenFormGenerator {
switch (scopeType.type) {
case "oneOf":
throw new NoSpokenFormError(`Scope type '${scopeType.type}'`);

case "contiguous": {
const scope = this.handleScopeType(scopeType.scopeType);
return [this.spokenFormMap.modifierExtra.contiguousScope, scope];
}

case "surroundingPair": {
const pair = this.spokenFormMap.pairedDelimiter[scopeType.delimiter];
if (scopeType.forceDirection != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
import {
ContiguousScopeType,
Direction,
Position,
Range,
ScopeType,
TextEditor,
next,
} from "@cursorless/common";
import { ScopeHandlerFactory } from ".";
import { Target } from "../../../typings/target.types";
import { constructScopeRangeTarget } from "../constructScopeRangeTarget";
import { BaseScopeHandler } from "./BaseScopeHandler";
import type { TargetScope } from "./scope.types";
import type {
CustomScopeType,
ScopeHandler,
ScopeIteratorRequirements,
} from "./scopeHandler.types";

export class ContiguousScopeHandler extends BaseScopeHandler {
protected readonly isHierarchical = false;
private readonly scopeHandler: ScopeHandler;

constructor(
private scopeHandlerFactory: ScopeHandlerFactory,
public scopeType: ContiguousScopeType,
languageId: string,
) {
super();
const handler = scopeHandlerFactory.create(scopeType.scopeType, languageId);
if (handler == null) {
throw new Error(
`No available scope handler for '${scopeType.scopeType.type}'`,
);
}
this.scopeHandler = handler;
}

get iterationScopeType(): ScopeType | CustomScopeType {
return this.scopeHandler.iterationScopeType;
}

*generateScopeCandidates(
editor: TextEditor,
position: Position,
direction: Direction,
AndreasArvidsson marked this conversation as resolved.
Show resolved Hide resolved
_hints: ScopeIteratorRequirements,
): Iterable<TargetScope> {
let targetRangeOpposite = next(
generateTargetRangesInDirection(
this.scopeHandler,
editor,
position,
direction === "forward" ? "backward" : "forward",
),
);

const targetRangesIter = generateTargetRangesInDirection(
this.scopeHandler,
editor,
position,
direction,
);

for (const targetRange of targetRangesIter) {
if (
targetRangeOpposite != null &&
isAdjacent(targetRangeOpposite.proximal, targetRange.proximal)
) {
yield combineScopes(targetRangeOpposite.distal, targetRange.distal);
targetRangeOpposite = undefined;
} else {
yield combineScopes(targetRange.proximal, targetRange.distal);
}
}
}
}

function combineScopes(scope1: TargetScope, scope2: TargetScope): TargetScope {
if (scope1.domain.isRangeEqual(scope2.domain)) {
return scope1;
}

return {
editor: scope1.editor,
domain: scope1.domain.union(scope2.domain),
getTargets: (isReversed) => {
return constructScopeRangeTarget(isReversed, scope1, scope2);
},
};
}

function* generateTargetRangesInDirection(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this function is correct. Eg corner cases like single scope not adjacent to others that's last in the file. To me it looks like if you've already yielded anything, then the final yield after the loop body won't fire.

Some proper unit tests for ContiguousScopeHandler would help. See https://github.com/cursorless-dev/cursorless/blob/main/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/BaseScopeHandler.test.ts if you're not sure how to do unit tests for this kinda thing. There's a limit to how far you can get using existing scopes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually might make sense to hold off on unit tests for the following reasons:

  1. Your code actually looks at document text, unlike the test I linked, so might be harder to mock that properly
  2. On second thought, I think your code is probably correct
  3. I'm actually not convinced of the utility of this contiguous scope thing 😅. The only example I saw that was actually useful was "comment", and tbh I'd be tempted to just automatically merge adjacent single-line comments, as we could target a single one using "line". And I worry users will try "fat arg" to get something like Support "all" modifier #473, and then be surprised when it breaks if there's a comment between args. So maybe we ship this with slightly fewer tests, maybe marked private? idk

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update from discussion today:

  • We agreed "comment" is the only use case we can come up with, and we probably just want that to be default behaviour for line comments
  • If @josharian wants to fight for supporting "fat" for arbitrary scopes, then we might keep it; otherwise it dies
  • We potentially could reuse this code to make contiguous be default behaviour for comment

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @josharian wants to fight for supporting "fat" for arbitrary scopes, then we might keep it; otherwise it diesau

looking at these test cases, i think it should die

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Any opinion on making single-line comments behave this way by default? (Ie auto expand to all comments that aren't separated by an empty line)

Copy link
Member

@pokey pokey Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update from meet-up: let's do the following:

(
  (comment) @comment
  (#match? @comment "^//")
  (#contiguous! @comment)
)
(comment) @comment

We both don't love this option or the option in this PR, but it's the best we can do with what we have today. Fwiw we would prefer something like

(
  (comment) @comment
  ($if
    (#match? @comment "^//")
    (#contiguous! @comment)
  )
)

but that's a bit out of scope here 😅

scopeHandler: ScopeHandler,
editor: TextEditor,
position: Position,
direction: Direction,
): Iterable<{ proximal: TargetScope; distal: TargetScope }> {
let proximal, distal: TargetScope | undefined;

const generator = scopeHandler.generateScopes(editor, position, direction, {
allowAdjacentScopes: true,
skipAncestorScopes: true,
});

for (const scope of generator) {
if (proximal == null) {
proximal = scope;
}

if (distal != null) {
if (!isAdjacent(distal, scope)) {
yield { proximal, distal };
proximal = scope;
}
}

distal = scope;
}

if (proximal != null && distal != null) {
yield { proximal, distal };
}
}

function isAdjacent(scope1: TargetScope, scope2: TargetScope): boolean {
if (scope1.domain.isRangeEqual(scope2.domain)) {
return true;
}

const [startTarget, endTarget] = getTargetsInDocumentOrder(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

scope1.getTargets(false)[0],
scope2.getTargets(false)[0],
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to throw an error if there's more than one target? Should we have a utility function function ensureSingleTarget(scope: Scope): Target that checks there's only one target and returns it? That worked out well for ensureSingleEditor I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reused our existing and shore single target. They should really never trigger with our current implementation so am fine with using an error message that might not be perfect.


const leadingRange =
startTarget.getTrailingDelimiterTarget()?.contentRange ??
startTarget.contentRange;
const trailingRange =
endTarget.getLeadingDelimiterTarget()?.contentRange ??
endTarget.contentRange;

if (leadingRange.intersection(trailingRange) != null) {
return true;
}

// Non line targets are excluded if they are separated by more than one line
if (
!startTarget.isLine &&
trailingRange.start.line - leadingRange.end.line > 1
) {
return false;
}

// Finally targets are excluded if there is non whitespace text between them
const rangeBetween = new Range(leadingRange.end, trailingRange.start);
const text = startTarget.editor.document.getText(rangeBetween);
return /^\s*$/.test(text);
}

function getTargetsInDocumentOrder(
target1: Target,
target2: Target,
): [Target, Target] {
return target1.contentRange.start.isBefore(target2.contentRange.start)
? [target1, target2]
: [target2, target1];
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
import type { ScopeType } from "@cursorless/common";
import { LanguageDefinitions } from "../../../languages/LanguageDefinitions";
import { CharacterScopeHandler } from "./CharacterScopeHandler";
import { CustomRegexScopeHandler } from "./RegexScopeHandler";
import { ContiguousScopeHandler } from "./ContiguousScopeHandler";
import { DocumentScopeHandler } from "./DocumentScopeHandler";
import { IdentifierScopeHandler } from "./IdentifierScopeHandler";
import { LineScopeHandler } from "./LineScopeHandler";
import { NonWhitespaceSequenceScopeHandler } from "./RegexScopeHandler";
import { OneOfScopeHandler } from "./OneOfScopeHandler";
import { ParagraphScopeHandler } from "./ParagraphScopeHandler";
import {
CustomRegexScopeHandler,
NonWhitespaceSequenceScopeHandler,
UrlScopeHandler,
} from "./RegexScopeHandler";
import { ScopeHandlerFactory } from "./ScopeHandlerFactory";
import { SentenceScopeHandler } from "./SentenceScopeHandler/SentenceScopeHandler";
import { TokenScopeHandler } from "./TokenScopeHandler";
import { UrlScopeHandler } from "./RegexScopeHandler";
import { WordScopeHandler } from "./WordScopeHandler/WordScopeHandler";
import { LanguageDefinitions } from "../../../languages/LanguageDefinitions";
import type { CustomScopeType, ScopeHandler } from "./scopeHandler.types";

/**
Expand Down Expand Up @@ -72,6 +75,8 @@ export class ScopeHandlerFactoryImpl implements ScopeHandlerFactory {
return new CustomRegexScopeHandler(this, scopeType, languageId);
case "custom":
return scopeType.scopeHandler;
case "contiguous":
return new ContiguousScopeHandler(this, scopeType, languageId);
case "instance":
// Handle instance pseudoscope with its own special modifier
throw Error("Unexpected scope type 'instance'");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,9 @@ function isLanguageSpecific(scopeType: ScopeType): boolean {
case "customRegex":
return false;

case "contiguous":
AndreasArvidsson marked this conversation as resolved.
Show resolved Hide resolved
return isLanguageSpecific(scopeType.scopeType);

case "oneOf":
throw Error(
`Can't decide whether scope type ${JSON.stringify(
Expand Down
3 changes: 2 additions & 1 deletion packages/cursorless-engine/src/spokenForms/SpokenFormType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ type ModifierExtra =
| "previous"
| "next"
| "forward"
| "backward";
| "backward"
| "contiguousScope";

export type SpokenFormType = keyof SpokenFormMapKeyTypes;
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ export const defaultSpokenFormMapCore: DefaultSpokenFormMapDefinition = {
next: "next",
forward: "forward",
backward: "backward",
contiguousScope: "fat",
AndreasArvidsson marked this conversation as resolved.
Show resolved Hide resolved
},

customRegex: {},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
languageId: plaintext
command:
version: 6
spokenForm: change fat block
action:
name: clearAndSetSelection
target:
type: primitive
modifiers:
- type: containingScope
scopeType:
type: contiguous
scopeType: {type: paragraph}
usePrePhraseSnapshot: true
initialState:
documentContents: |-
aaa
aaa

bbb
selections:
- anchor: {line: 0, character: 0}
active: {line: 0, character: 0}
marks: {}
finalState:
documentContents: ""
selections:
- anchor: {line: 0, character: 0}
active: {line: 0, character: 0}
Loading