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.

Sign up for GitHub

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

Fix for markdown cells #2739

Merged
merged 3 commits into from
Jan 21, 2025
+189 −107
Merged

Fix for markdown cells #2739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ccaf574
Fix for markdown cells
AndreasArvidsson Jan 21, 2025
7bad93d
Use one of scope handler
AndreasArvidsson Jan 21, 2025
3297bca
Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…
phillco Jan 21, 2025
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
27 changes: 27 additions & 0 deletions data/fixtures/recorded/languages/markdown/changeCell.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
languageId: markdown
command:
version: 7
spokenForm: change cell
action:
name: clearAndSetSelection
target:
type: primitive
modifiers:
- type: containingScope
scopeType: {type: notebookCell}
Copy link
Member

Choose a reason for hiding this comment

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

NB: notebookCell just means the "cell" scope type. It doesn't actually mean a notebook cell (and in fact here it means a markdown table cell), but we added notebooks first and we don't want to rename it.

usePrePhraseSnapshot: false
initialState:
documentContents: |
```
code
```
selections:
- anchor: {line: 1, character: 0}
active: {line: 1, character: 0}
marks: {}
finalState:
documentContents: |+

selections:
- anchor: {line: 0, character: 0}
active: {line: 0, character: 0}
2 changes: 1 addition & 1 deletion packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/LineScopeHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export class LineScopeHandler extends BaseScopeHandler {
type: "paragraph",
} as const;
protected readonly isHierarchical = false;
public readonly includeAdjacentInEvery: boolean = true;
phillco marked this conversation as resolved.
Show resolved Hide resolved
public readonly includeAdjacentInEvery = true;

constructor(_scopeType: ScopeType, _languageId: string) {
super();
Expand Down
116 changes: 116 additions & 0 deletions ...rsorless-engine/src/processTargets/modifiers/scopeHandlers/NotebookCellApiScopeHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import {
Range,
type Direction,
type NotebookCell,
type Position,
type TextEditor,
} from "@cursorless/common";
import { ide } from "../../../singletons/ide.singleton";
import { NotebookCellTarget } from "../../targets";
import { BaseScopeHandler } from "./BaseScopeHandler";
import type { TargetScope } from "./scope.types";
import type { ScopeIteratorRequirements } from "./scopeHandler.types";

/**
* This is the scope handler for the actual notebook API in the IDE.
*/
export class NotebookCellApiScopeHandler extends BaseScopeHandler {
public readonly scopeType = { type: "notebookCell" } as const;
public readonly iterationScopeType = { type: "document" } as const;
protected isHierarchical = false;

constructor() {
super();
}

*generateScopeCandidates(
editor: TextEditor,
position: Position,
direction: Direction,
hints: ScopeIteratorRequirements,
): Iterable<TargetScope> {
const cells = getNotebookCells(editor, position, direction, hints);

for (const cell of cells) {
yield createTargetScope(cell);
}
}
}

function getNotebookCells(
editor: TextEditor,
position: Position,
direction: Direction,
hints: ScopeIteratorRequirements,
) {
const nb = getNotebook(editor);

if (nb == null) {
return [];
}

const { notebook, cell } = nb;

if (hints.containment === "required") {
return [cell];
}

if (
hints.containment === "disallowed" ||
hints.containment === "disallowedIfStrict"
) {
return direction === "forward"
? notebook.cells.slice(cell.index + 1)
: notebook.cells.slice(0, cell.index).reverse();
}

// Every scope
if (hints.distalPosition != null) {
const searchRange = new Range(position, hints.distalPosition);
if (searchRange.isRangeEqual(editor.document.range)) {
return notebook.cells;
}
}

return direction === "forward"
? notebook.cells.slice(cell.index)
: notebook.cells.slice(0, cell.index + 1).reverse();
}

function getNotebook(editor: TextEditor) {
const uri = editor.document.uri.toString();
for (const notebook of ide().visibleNotebookEditors) {
for (const cell of notebook.cells) {
if (cell.document.uri.toString() === uri) {
return { notebook, cell };
}
}
}
return undefined;
}

function createTargetScope(cell: NotebookCell): TargetScope {
const editor = getEditor(cell);
const contentRange = editor.document.range;
return {
editor,
domain: contentRange,
getTargets: (isReversed: boolean) => [
new NotebookCellTarget({
editor,
isReversed,
contentRange,
}),
],
};
}

function getEditor(cell: NotebookCell) {
const uri = cell.document.uri.toString();
for (const editor of ide().visibleTextEditors) {
if (editor.document.uri.toString() === uri) {
return editor;
}
}
throw new Error("Editor not found notebook cell");
}
150 changes: 44 additions & 106 deletions .../cursorless-engine/src/processTargets/modifiers/scopeHandlers/NotebookCellScopeHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,132 +1,70 @@
import {
Range,
type Direction,
type NotebookCell,
type Position,
type ScopeType,
type TextEditor,
} from "@cursorless/common";
import type { LanguageDefinitions } from "../../../languages/LanguageDefinitions";
import { ide } from "../../../singletons/ide.singleton";
import { NotebookCellTarget } from "../../targets";
import { BaseScopeHandler } from "./BaseScopeHandler";
import { NotebookCellApiScopeHandler } from "./NotebookCellApiScopeHandler";
import { OneOfScopeHandler } from "./OneOfScopeHandler";
import type { TargetScope } from "./scope.types";
import type {
ComplexScopeType,
ScopeHandler,
ScopeIteratorRequirements,
} from "./scopeHandler.types";
import type { ScopeHandlerFactory } from "./ScopeHandlerFactory";

export class NotebookCellScopeHandler implements ScopeHandler {
export class NotebookCellScopeHandler extends BaseScopeHandler {
phillco marked this conversation as resolved.
Show resolved Hide resolved
public readonly scopeType = { type: "notebookCell" } as const;
public readonly iterationScopeType = { type: "document" } as const;
public readonly includeAdjacentInEvery = false;
protected isHierarchical = false;
private readonly scopeHandler: ScopeHandler;

constructor(
private languageDefinitions: LanguageDefinitions,
_scopeType: ScopeType,
private languageId: string,
) {}

*generateScopes(
editor: TextEditor,
position: Position,
direction: Direction,
hints: ScopeIteratorRequirements,
Copy link
Member

Choose a reason for hiding this comment

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

NTS: The actual type of the interface is requirements?: Partial<ScopeIteratorRequirements>. This should really be a tsc compile error but it wasn't. We therefore started to read properties on hints when some of those properties could be undefined which threw an exception

Copy link
Member

Choose a reason for hiding this comment

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

We needed something like what the base class does:

  *generateScopes(
    editor: TextEditor,
    position: Position,
    direction: Direction,
    requirements: Partial<ScopeIteratorRequirements> = {},
  ): Iterable<TargetScope> {
    const hints: ScopeIteratorRequirements = {
      ...DEFAULT_REQUIREMENTS,
      ...requirements,
      distalPosition:
        requirements.distalPosition ??
        (direction === "forward"
          ? editor.document.range.end
          : editor.document.range.start),
    };

Rather than copying the logic it was easier to just rewrite this class to use the base class, which Andreas says should have been done originally, just didn't think of it

): Iterable<TargetScope> {
const scopeHandler = this.languageDefinitions
.get(this.languageId)
?.getScopeHandler(this.scopeType);

if (scopeHandler != null) {
yield* scopeHandler.generateScopeCandidates(
editor,
position,
direction,
hints,
);
}

const cells = getNotebookCells(editor, position, direction, hints);

for (const cell of cells) {
yield createTargetScope(cell);
}
}
}

function getNotebookCells(
editor: TextEditor,
position: Position,
direction: Direction,
hints: ScopeIteratorRequirements,
) {
const nb = getNotebook(editor);

if (nb == null) {
return [];
}

const { notebook, cell } = nb;

if (hints.containment === "required") {
return [cell];
get iterationScopeType(): ScopeType | ComplexScopeType {
return this.scopeHandler.iterationScopeType;
}

if (
hints.containment === "disallowed" ||
hints.containment === "disallowedIfStrict"
constructor(
scopeHandlerFactory: ScopeHandlerFactory,
languageDefinitions: LanguageDefinitions,
_scopeType: ScopeType,
languageId: string,
) {
return direction === "forward"
? notebook.cells.slice(cell.index + 1)
: notebook.cells.slice(0, cell.index).reverse();
}
super();

// Every scope
if (hints.distalPosition != null) {
const searchRange = new Range(position, hints.distalPosition);
if (searchRange.isRangeEqual(editor.document.range)) {
return notebook.cells;
}
}
this.scopeHandler = (() => {
const apiScopeHandler = new NotebookCellApiScopeHandler();

return direction === "forward"
? notebook.cells.slice(cell.index)
: notebook.cells.slice(0, cell.index + 1).reverse();
}
const languageScopeHandler = languageDefinitions
.get(languageId)
?.getScopeHandler(this.scopeType);

function getNotebook(editor: TextEditor) {
const uri = editor.document.uri.toString();
for (const notebook of ide().visibleNotebookEditors) {
for (const cell of notebook.cells) {
if (cell.document.uri.toString() === uri) {
return { notebook, cell };
if (languageScopeHandler == null) {
return apiScopeHandler;
}
}
}
return undefined;
}

function createTargetScope(cell: NotebookCell): TargetScope {
const editor = getEditor(cell);
const contentRange = editor.document.range;
return {
editor,
domain: contentRange,
getTargets: (isReversed: boolean) => [
new NotebookCellTarget({
editor,
isReversed,
contentRange,
}),
],
};
}
return OneOfScopeHandler.createFromScopeHandlers(
scopeHandlerFactory,
{
type: "oneOf",
scopeTypes: [
languageScopeHandler.scopeType,
apiScopeHandler.scopeType,
],
},
[languageScopeHandler, apiScopeHandler],
languageId,
);
})();
}
Comment on lines -120 to +60
Copy link
Member

Choose a reason for hiding this comment

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

OneOfScopeHandler means that sources from the two could be merged, so if you have markdown cells inside of note cells something like next fifth cell could refer to a markdown cell and the second notebook cell, previously this could by referred to one or the other.

(NB: not fifth item; this uses the iteration scope, and the iteration scope is more tightly bound to the data structure that you are in. It doesn't combined sources like that. next fifth... instead queries the scope generator five times, and this use of the scope generator does combine sources.)

Four different types of modifiers:

  • containing
  • every
  • ordinal
  • relative

Every and ordinal use the iteration scope; containing and relative consume from the scope generator but not the iteration scope.

Why don't iteration scopes combine sources? We discussed this and the behavior makes sense when you think about the use cases. When asking for ordinal or every scope ("every item" for example), you are not likely to want a creative interpretation of item that expands across the data structure that you are in.

containing does query the generator (so it could combine sources in theory) but only once so it behaves like every and ordinal.

In the context of notebook cells that might contain markdown cells, the pattern's that "every" never crosses a document boundary. (Imagine if you had conventional editors split and asked to take every markdown cell -- it's only going to act on the current editor.)

In short, the behavior generally works well for our existing use cases but it is not explicitly specified. OneOfScopeHandler will combine multiple sources when yielding items but when you query the iteration scope only one source will be used, because that's how iteration scopes work.


function getEditor(cell: NotebookCell) {
const uri = cell.document.uri.toString();
for (const editor of ide().visibleTextEditors) {
if (editor.document.uri.toString() === uri) {
return editor;
}
generateScopeCandidates(
editor: TextEditor,
position: Position,
direction: Direction,
hints: ScopeIteratorRequirements,
): Iterable<TargetScope> {
return this.scopeHandler.generateScopes(editor, position, direction, hints);
}
throw new Error("Editor not found notebook cell");
}
1 change: 1 addition & 0 deletions ...s/cursorless-engine/src/processTargets/modifiers/scopeHandlers/ScopeHandlerFactoryImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ export class ScopeHandlerFactoryImpl implements ScopeHandlerFactory {
);
case "notebookCell":
return new NotebookCellScopeHandler(
this,
this.languageDefinitions,
scopeType,
languageId,
Expand Down
Loading