Add custom context key for onTabKey/onShiftTabKey key binding.

[takumisoft68]

Purpose

• Improve "when" assign of onTabKey/onShiftTabKey to be more suitable.
• Resolve Tab key confliction with other extensions. (resolve Restrict key binding context to reduce Tab key conflict #1015)

What's done

• Added a custom context key that represents whether the selecting position is in the list.
• Restricted "when" assign of onTabKey/onShiftTabKey by using the custome context key.

[yzhang-gh]

Thanks @takumisoft68. It looks good to me (although I haven't tested how reliable it is). Will try it out soon.

cc @Lemmingh.

[Lemmingh]

Could we have an Editor Context Service

Just like the Configuration Service and the Localization Service, I'd like to have a clear and dedicated module that manages our "when clause contexts".

Moreover, having a proper service layer will make migration convenient if VS Code's Context API (microsoft/vscode#10471) comes true.

Could we name the keys neatly

Currently, our keys are all prefixed by markdown.extension. For example:

• Color: markdown.extension.editor.codeSpan.border
• Command: markdown.extension.toc.create
• Configuration: markdown.extension.theming.decoration.renderCodeSpan

Then, I wonder if we can name our context keys as:

markdown.extension.activated
markdown.extension.editor.cursor.inMarkdownList

Could we short-circuit the process cleverly

We should analyze the events first to avoid unnecessary computation.

Could we use the Markdown engine

Full document parsing is the only way to always get correct information, and usually takes only tens of milliseconds. Handling asynchronous behavior is a tricky part.

[takumisoft68]

@yzhang-gh @Lemmingh
Thank you for reviewing.
I fixed that you pointed out.

• Could we have an Editor Context Service
• Could we name the keys neatly
• Could we short-circuit the process cleverly

But I don't know what is the Markdown engine.

• Could we use the Markdown engine

[takumisoft68]

@yzhang-gh @Lemmingh
I made several Context Services for each context.
Do you expect merging them to one instance?

[yzhang-gh]

Thanks a lot!

I made several Context Services for each context.
Do you expect merging them to one instance?

I prefer to merge them.

But I don't know what is the Markdown engine.

• Could we use the Markdown engine

It is to use a Markdown syntax parser (e.g. markdown-it) to tell us whether the cursor is in a list, math env, and so on. It will be much more accurate than doing it by RegExp.
We don't have to implement it now and can leave it for further PR.

[Lemmingh]

Wow, an impressive change. I'm afraid I don't have much time to carefully review it now.

Just a few thoughts about code style:

Prefer kebab-case in file name

e.g.,

Consider renaming "src/contextService/IContextService.ts" to "src/editor-context-service/i-context-service.ts".

as discussed in:

• https://markdown-all-in-one.github.io/docs/contributing/code-style/file.html

• Naming and casing of identifiers markdown-all-in-one/chat#4

The "use strict" directive is not necessary

• Module code is always strict mode code.

• We already enables alwaysStrict via the strict flag.

[takumisoft68]

@yzhang-gh @Lemmingh
I fixed you pointed at.

• merged context services
  • reduced duplicated callback handler and processing of getting text of document
• changed file name to kebab-case

However the "use strict" is stay. I can't find strict flag in main.yml. I'm not sure that remove is ok.

[Lemmingh]

Nice overall, though I'm not sure whether the when clauses in package.json are combined correctly.

---

The strict flag is in the tsconfig.json file.

I'm going to fix the build system this weekend. 😂
Then, we can run CI and test dev build to validate it.

---

A comment I missed:

Do you expect merging them to one instance?

I prefer to merge them.

I agree, at least for now.

These contexts are connected with the parsing result. Then in principle, they should be managed by the same service backed by one parsing engine.

[takumisoft68]

@yzhang-gh @Lemmingh
Sorry, aaf7802 has a mistake.
I fixed it on 3a0b25e.
Change of inlineSuggestionVisible is not on purpose.

[yzhang-gh]

Thanks for the effort!

Looks like there are still three context services (list, fenced code, and math)? We can have one service for all three contexts, and that is what @Lemmingh means by "one parsing engine" (although we can just keep the regexp-based approach for now).

[takumisoft68]

@yzhang-gh @Lemmingh
I tried.

Unfortunately,
I think, when replacing the regexp-based approach to using markdown-it, we have to change the conditions of availability of some commands.
markdown.extension.onTabKey is an example.
It has difference between the bullets detection by the markdown-it and existing regexp-based judgement of command availability.

So, would you leave it for further PR, as you say.

[Lemmingh]

We don't have to stick to the poor markdown-it, and would be happy if someone can introduce us a Markdown processor with precise source mapping.

Anyway, I'll tackle it later.

---

Would you like me to rebase or three-way-merge when syncing this branch with master?

[takumisoft68]

@Lemmingh
I'm glad for your answer.

Would you like me to rebase or three-way-merge when syncing this branch with master?

Can I leave it to you?

[takumisoft68]

@Lemmingh @yzhang-gh
Thank you for reviewing.
Is there anything more I can do.

[Lemmingh]

Perhaps you can write some verification procedures?

We're updating the build system (#1119), so I'm afraid here is not much substantial work to do now.

Thank you.

[yzhang-gh]

@takumisoft68 Hi, long time no update. Finally I got some time to deal with this.

I've resolved the merge conflicts and created a new branch https://github.com/yzhang-gh/vscode-markdown/tree/dev/context
Could you also update your branch to that one? I plan to test it for a while and then merge it in.

[takumisoft68]

@yzhang-gh Hi, thank you. I'm glad to your update.

I have updated my branch to catch up to dev/context branch. By rebase.
And to fix a conflict, I did cherry pick cb38589 from your master branch.