✨ Add decoration manager #897

Lemmingh · 2021-02-10T15:50:03Z

Summary

Fix Inline code block outline is too dark on light background #734: Add 3 themable colors. Now, decoration colors are fully customizable. See package.json.
Fix Allow enabling/disabling code span and strikethrough decorations separately #757:
- Add a set of theming.decoration.render* settings. See Configuration Schema Change #888.
- Deprecate the syntax.decorations setting.
- Add a set of src/theming/* modules to power a brand new editor decoration manager.
- Deprecate the src/syntaxDecorations.ts module. Effectively stop developing the current "Plain Theme". I think semantic highlighting plus dedicated themes will be a right way.
- Add a configuration manager to smooth the transition. See src/configuration/manager.ts.
Fix Make Whitespaces at Line Ends Visible #881: Add 3 new decoration classes: HardLineBreak, Paragraph, TrailingSpace. See src/theming/constant.ts.

Known issues

Sorry for being late.

I spent lots of time in code span. However, I'm afraid many problems remain in decorationWorkerRegistry.ts. Users may notice that:

Code spans in tables do not get decorations. It's too tricky to locate them.
Hard line breaks show in display math areas. I'm using commonMarkEngine for reliability.
Many links do not get decorations. It's complex too.
Strikethroughs in editor are different from those in preview. I'm aligning with GitHub (cmark-gfm), which appears much stricter than markdown-it.

yzhang-gh

(Will finish the review tomorrow)

yzhang-gh · 2021-02-10T16:35:55Z

package.json

@@ -348,6 +377,42 @@
                    "default": false,
                    "markdownDescription": "%config.tableFormatter.normalizeIndentation.description%"
                },
+                "markdown.extension.theming.decoration.renderCodeSpan": {


Guess we can directly use ...decoration.codeSpan and so on

The verb "render" leaves room for a bunch of other settings that I'm going to add to this section.

Besides, I see many people use verb-noun statement as toggle label, and feel the form makes intention clear.

I'm okay with that. But I was wondering whether we should have used an object-type configuration, otherwise there are too many trivial options (esp. if there will be more). The disadvantage is it becomes more complex for both us and users, but I guess this is the price to have an advanced configuration 😥.

If it is a toggle, then it is supposed to be a toggle.

If it is a leaf, then it is supposed to be a leaf.

IMO, wrapping a group of settings inside an object would introduce an unnecessary layer, harming accessibility, usability, clarity, and validation.

I don't consider "trivial" items as a problem. Users can benefit a lot from visually and directly editing configuration right in Settings UI. And you can see other products also show many "trivial" settings for fine-grained control.

https://code.visualstudio.com/api/references/extension-guidelines#settings

https://docs.microsoft.com/en-us/windows/uwp/design/app-settings/guidelines-for-app-settings

https://developer.apple.com/design/human-interface-guidelines/macos/app-architecture/preferences/

https://developer.android.com/guide/topics/ui/settings/organize-your-settings

The real problem is VS Code doesn't provide setting groups. Ideally it should have a tree-like structure, but what we have now is a big flat list (visually).

(I have a bad experience when I search "python lint" in the Settings UI: there are 41 results from the Python extension, including different providers: Bandit, Flake8, Mypy, Prospector, Pydocstyle etc., each of which further has several options: enabled, path, args etc. This looks really "noisy" and horrible.)

I agree that object-type configuration has many disadvantages as you mentioned. To me it is just a workaround and that's why I'm hesitant. If we only have 5 new settings, it won't be a problem. But if each of which will have, e.g. 2 more settings in the future, it becomes more like the Python case 😓.

VS Code doesn't provide setting groups

IMHO, it is VS Code's fault. microsoft/vscode#70589

We should not make it worse by introducing custom objects.

I have a bad experience when I search "python lint"

TypeScript's settings are also a beast. 😂

VS Code's fault. microsoft/vscode#70589

Upvoted 👍

We should not make it worse by introducing custom objects.

Fine, let's just use the simple way (the flat list).

yzhang-gh · 2021-02-10T16:53:17Z

src/configuration/manager.ts

+                // Our NLS module needs to be configured before using, which is done in the extension entry point.
+                // This module may be directly or indirectly imported by the entry point.
+                // Thus, this module may be loaded before the NLS module is available.
+                vscode.window.showErrorMessage(`The setting 'markdown.extension.${key}' has been deprecated.`);


This is too "aggressive", I mean everyone (millions) will see the pop-up.

I prefer to do a config transition in the background (without proactively notifying the users). (Of course we will say it in the changelog.)

e.g. https://github.com/James-Yu/LaTeX-Workshop/blob/8eb80f46794233c868223e99f5c206449bab5587/src/config.ts#L67

everyone (millions) will see the pop-up.

No.

vscode-markdown/package.json

Lines 355 to 359 in 17c7bfc

"markdown.extension.syntax.decorations": {

"type": "boolean",

"default": null,

"markdownDeprecationMessage": "%config.syntax.decorations.description%"

},

I've set the default value of the deprecated setting to null. Thus, only those that changed the setting will see the notification.

I prefer to do a config transition in the background

But it's the user that knows the state of settings best. A short message to the user can saves you tens of line of code.

As you can see from KnownKey.ts, defaultFallback.ts, and #888, the changes will be complex, involving names, formats, and scopes. You can hardly make it correct programmatically.

only those that changed the setting will see the notification.

Then this is good.

But it's the user that knows the state of settings best.

From my experience most of the users know little about the settings... But for options like syntaxDecorations we can assume they know about it.
I was concerned someday we might change a configuration which affects majority of the users, although it isn't very likely to happen.

most of the users know little about the settings

I mean the settings are under the control of the user. They remember what they did, and what those changes actually mean.

If we tried to act on behalf of them, we would have to inspect() and analyze the possibly huge result. But it still cannot solve all problems. For example, in my plan, decorationFileSizeLimit (binary size) will be superseded by maxDocumentLength (the number of UTF-16 code units). I guess the factor is 1.5 (UTF-8, only BMP), but who knows, what if a user prefers Windows-1252 or Shift-JIS.

someday we might change a configuration which affects majority of the users

Er ... Very likely.

I've identified a good number of flawed configuration keys, but only marked those that I've found a solution for as "To be superseded" in KnownKey.ts.

most of the users know little about the settings

I mean the settings are under the control of the user. They remember what they did, and what those changes actually mean.

Ideally, they should. The reality (from my observation) is, even many CS majoring students (my friends) have few concepts about the settings. It is just because they never pay attention to it.
I believe the fact "Markdown is rendered to HTML" will filter out 80% of the users. (Many users don't know what "decoration" means as it is a VS Code concept. And many many users never read the README.)
For average users, (probably) they just searched somewhere, changed the settings and totally forgot about it several months later. They are not interested in how it should be configured, they want it just works.

The "Windows-1252 and Shift-JIS" thing is a typical example. I would say more than 90% of the users don't know about it, as I have never seen it before 🤣.

Two of my own "rules":

Only show pop-up message if we really want the users to know about it (e.g., exciting new features). And make sure the message is attractive (not like a regular non-exciting message)

Do not assume the users {know well about / are interested in} how {Markdown / VS Code} works. (curse of knowledge)

Many users don't know what "decoration" means as it is a VS Code concept.

Do you think changing theming to editorTheming or editing.theming will help? E.g.

markdown.extension.editorTheming.decoration.renderCodeSpan

Markdown › Extension › Editor Theming › Decoration: Render Code Span

markdown.extension.editing.theming.decoration.renderCodeSpan

Markdown › Extension › Editing › Theming › Decoration: Render Code Span

Windows-1252 and Shift-JIS

Windows-1252 was popular in Western Europe.
Shift-JIS is still common in Japan.

curse of knowledge

OK. I'll keep it in mind when writing docs.

Do you think changing theming to editorTheming or editing.theming will help? E.g.

Ah, I didn't notice that. I like (just) theming or theme as it is very clear 👍, especially after markdown and extension. (It is much better than syntax that we had before)
The problem is at decoration, but I cannot think of a better one... Then I'd rather keep it aligned with VS Code (and "teach" users about it).

Windows-1252 and Shift-JIS

Many thanks. #TodayILearned

yzhang-gh · 2021-02-11T02:23:27Z

No more comments 👌

Lemmingh · 2021-02-12T04:56:22Z

Well, the last change is so small that I guess you will be OK with it.

And let's keep the conversations open. Then visitors can read them easily.

yzhang-gh · 2021-02-12T06:34:09Z

🍻

Lemmingh added 3 commits February 1, 2021 20:55

✨ Add configuration manager

17c7bfc

Show an error message for each deprecated key

cb7388a

✨ decoration manager

00cef09

Lemmingh added Area: Configuration Requiring changes in configuration schema. Area: Editor theming Decorations and highlighting in editor. labels Feb 10, 2021

Lemmingh added this to the Next milestone Feb 10, 2021

Lemmingh requested a review from yzhang-gh February 10, 2021 15:50

Lemmingh self-assigned this Feb 10, 2021

yzhang-gh reviewed Feb 10, 2021

View reviewed changes

Lemmingh mentioned this pull request Feb 11, 2021

Configuration Schema Change #888

Open

Lemmingh added 2 commits February 11, 2021 20:50

Update comments

f89cd18

Update descriptions

bb71688

Lemmingh requested a review from yzhang-gh February 11, 2021 15:07

Lemmingh merged commit bebf0d8 into master Feb 12, 2021

Lemmingh deleted the dev/editor-decoration branch February 12, 2021 05:08

Lemmingh mentioned this pull request Apr 9, 2021

Richer rendering in editor, such as showing link text only (hiding destination) while not under cursor #929

Open

Lemmingh mentioned this pull request Jun 24, 2021

Add print.enableCheckBoxes config option #962

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add decoration manager #897

✨ Add decoration manager #897

Lemmingh commented Feb 10, 2021 •

edited

Loading

yzhang-gh left a comment

yzhang-gh Feb 10, 2021

Lemmingh Feb 10, 2021

yzhang-gh Feb 11, 2021

Lemmingh Feb 11, 2021

Lemmingh Feb 11, 2021

yzhang-gh Feb 11, 2021

Lemmingh Feb 11, 2021

yzhang-gh Feb 11, 2021

yzhang-gh Feb 10, 2021

Lemmingh Feb 10, 2021

yzhang-gh Feb 11, 2021

Lemmingh Feb 11, 2021

yzhang-gh Feb 11, 2021

Lemmingh Feb 11, 2021

yzhang-gh Feb 11, 2021

yzhang-gh commented Feb 11, 2021

Lemmingh commented Feb 12, 2021

yzhang-gh commented Feb 12, 2021

	"markdown.extension.syntax.decorations": {
	"type": "boolean",
	"default": null,
	"markdownDeprecationMessage": "%config.syntax.decorations.description%"
	},

✨ Add decoration manager #897

✨ Add decoration manager #897

Conversation

Lemmingh commented Feb 10, 2021 • edited Loading

Summary

Known issues

yzhang-gh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yzhang-gh commented Feb 11, 2021

Lemmingh commented Feb 12, 2021

yzhang-gh commented Feb 12, 2021

Lemmingh commented Feb 10, 2021 •

edited

Loading