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

🐛 Rewrite heading processing #877

Merged
merged 32 commits into from
Jan 17, 2021
Merged

Conversation

Lemmingh
Copy link
Collaborator

@Lemmingh Lemmingh commented Jan 5, 2021

Summary

@Lemmingh Lemmingh added Area: Table of contents Pertaining to table of contents (TOC generation and detection, related heading operations). Area: Auto completion Code completion and suggestion, aka IntelliSense. labels Jan 5, 2021
@Lemmingh Lemmingh added this to the Next milestone Jan 5, 2021
@Lemmingh Lemmingh requested a review from yzhang-gh January 5, 2021 17:29
@Lemmingh Lemmingh self-assigned this Jan 5, 2021
@Lemmingh Lemmingh marked this pull request as draft January 6, 2021 02:07
Copy link
Owner

@yzhang-gh yzhang-gh left a comment

Choose a reason for hiding this comment

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

in progess

src/toc.ts Show resolved Hide resolved
src/toc.ts Outdated Show resolved Hide resolved
src/toc.ts Outdated Show resolved Hide resolved
src/toc.ts Outdated Show resolved Hide resolved
Copy link
Owner

@yzhang-gh yzhang-gh left a comment

Choose a reason for hiding this comment

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

Thanks for the great effort! ❤

One last question: should we force all the users to adopt the [heading](<link>)-style TOC? It will affect hundreds of thousands of people (creating a change to their git history). I would suggest not adding this when/where it is not necessary.

src/toc.ts Show resolved Hide resolved
src/completion.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
src/test/suite/toc.test.ts Show resolved Hide resolved
@yzhang-gh
Copy link
Owner

The "root" in getAllRootHeading refers to the position in the document tree.

Many block structures accept block structures as their content.

You can create some deeply nested things like:

# Heading A

> + # Heading B ###

But we never care about those non-root headings, right 😳? It is true that they are "headings" tags in terms of the syntax tree, but we won't say they are "headings" in terms of a document/article.

Also, it looks like we didn't make use of the syntax tree at all in this function. So I guess it would be better to move this "root" thing from the name to a piece of comment.

@Lemmingh
Copy link
Collaborator Author

Lemmingh commented Jan 7, 2021

But we never care about those non-root headings

It's just our current architecture cannot handle them. (the same reason as stated above)

We indeed have to consider them!

They play an important role in generating slugs. And implementations differ.


To solve all the problems, I'm privately prototyping version 4, where almost all features are powered by two language servers (one for symbol info and theming, the other for actions), and a plugin system is first class member.

I now get stuck constructing AST from markdown-it Tokens.

@yzhang-gh
Copy link
Owner

yzhang-gh commented Jan 7, 2021

But we never care about those non-root headings

It's just our current architecture cannot handle them. (the same reason as stated above)

We indeed have to consider them!

They play an important role in generating slugs. And implementations differ.

Probably I don't have enough context (about your plan). I was just saying it (assuming heading is rootHeading) is enough for the current feature set (TOC, section numbering). A so-called "heading" in a fenced code block is not a valid heading in our features (not in the TOC, not involved in the section numbering, and not present in the reference links).

Do you mean there are other cases where a heading nested in some blocks should be considered in the current feature set?
Or you plan to use the AST in the getAllRootHeading function soon/later?

@yzhang-gh
Copy link
Owner

Or you plan to use the AST in the getAllRootHeading function soon/later?

Not sure whether we should do it in this PR, but #880 is an example supporting this.

@Lemmingh
Copy link
Collaborator Author

I don't have enough context (about your plan).

I'll email you more details next month.

export type ProviderResult<T> = T | Thenable<T>;

export interface IHeadingInfoProvider<T extends IHeadingInfo = IHeadingInfo> {

    readonly provideHeadingInfo: (document: vscode.TextDocument, options: IHeadingInfoOption, token: vscode.CancellationToken) => ProviderResult<T[]>;

    readonly resolveHeadingInfo: (items: T[], token: vscode.CancellationToken) => ProviderResult<Required<T>[]>;
}

export interface IHeadingInfoOption {
    documentMode: SlugifyMode;
    projectLevelOmittedHeading: [markdownSpec.MarkdownHeadingLevel, string][] | undefined;
    respectMagicCommentOmit: boolean;
}

export interface IHeadingInfo {

    location: vscode.Location;
    level: markdownSpec.MarkdownHeadingLevel;
    rawContent: string;

    canInToc?: boolean;
    slug?: string;
    visibleText?: string;
}

Do you mean there are other cases where a heading nested in some blocks should be considered in the current feature set?

Yes. They play an important role in generating slugs. And I tend to regard the existence of getAllRootHeading() as a compromise. It's best to merge getAllTocEntry() and getAllRootHeading() as one method getAllHeading(), but the current architecture makes it too hard.

Render this example:

# Heading A

> + # Heading B ###

# Heading B

It's evident that GitHub, GitLab, VS Code, Azure DevOps ... all assign anchor ID to all headings no matter where they are.

Then, you have to get all headings to calculate slugs correctly.


Or you plan to use the AST in the getAllRootHeading function soon/later?

Probably in 2022. NOT now. I cannot make it within this month.

I tried to work directly on token stream. But as you can see in detectTocRanges(), it's frustrating. Then I decided to create something similar to Markdig, however, markdown-it appears to erase lots of information, which is useless for rendering but critical for understanding a document. So, stuck.

@yzhang-gh
Copy link
Owner

And I tend to regard the existence of getAllRootHeading() as a compromise. It's best to merge getAllTocEntry() and getAllRootHeading() as one method getAllHeading(), but the current architecture makes it too hard.

Makes sense to me 👍

It's evident that GitHub, GitLab, VS Code, Azure DevOps ... all assign anchor ID to all headings no matter where they are.

Interesting. I didn't know it before.

Or you plan to use the AST in the getAllRootHeading function soon/later?

Probably in 2022. NOT now. I cannot make it within this month.

That is fine.

BTW, my next big plan is to implement the "export as PDF" feature and that will bring us to the next major update (v4.0.0), although I am not sure when it will be done 🤣. Just take your (our) time.

* Clean up logic related to magic comment, since you've clarified the design.

* Tune comments to isolate function logic, that's, don't reveal one function's logic in the comments within another.

* Revert unnecessary change in package-lock.
@Lemmingh Lemmingh marked this pull request as ready for review January 17, 2021 14:47
@Lemmingh Lemmingh requested a review from yzhang-gh January 17, 2021 14:51
src/util.ts Show resolved Hide resolved
@yzhang-gh yzhang-gh merged commit 8828425 into master Jan 17, 2021
@yzhang-gh
Copy link
Owner

🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Auto completion Code completion and suggestion, aka IntelliSense. Area: Link (Reserved) Markdown link processing, URI recognition, slugification. Area: Table of contents Pertaining to table of contents (TOC generation and detection, related heading operations).
Projects
None yet
2 participants