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

Fix for markdown cells #2739

Merged
merged 3 commits into from
Jan 21, 2025
Merged

Fix for markdown cells #2739

merged 3 commits into from
Jan 21, 2025

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Jan 21, 2025

The latest update for the notebook cell handler broke cells in markdown.
Also implements notebook scope handler with a one of scope handler the same way we do for collection item. We can have both language specific implementations as well as the ide notebook api so this is a more proper implementation.

Checklist

  • I have added tests
  • [/] I have updated the docs and cheatsheet
  • [/] I have not broken the cheatsheet

@AndreasArvidsson AndreasArvidsson requested a review from a team as a code owner January 21, 2025 10:03
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

Comment on lines -120 to +60
],
};
}
return OneOfScopeHandler.createFromScopeHandlers(
scopeHandlerFactory,
{
type: "oneOf",
scopeTypes: [
languageScopeHandler.scopeType,
apiScopeHandler.scopeType,
],
},
[languageScopeHandler, apiScopeHandler],
languageId,
);
})();
}
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.

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.

@phillco phillco added this pull request to the merge queue Jan 21, 2025
Merged via the queue into main with commit 4688ad8 Jan 21, 2025
15 checks passed
@phillco phillco deleted the mdCell branch January 21, 2025 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants