✨ Add decoration manager #897
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| "use strict"; | ||
|
|
||
| /** | ||
| * Configuration keys that this product contributes. | ||
| * These values are relative to `markdown.extension`. | ||
| */ | ||
| type KnownKey = | ||
| | "completion.respectVscodeSearchExclude" | ||
| | "completion.root" | ||
| | "italic.indicator" | ||
| | "katex.macros" | ||
| | "list.indentationSize" | ||
| | "math.enabled" | ||
| | "orderedList.autoRenumber" | ||
| | "orderedList.marker" | ||
| | "preview.autoShowPreviewToSide" | ||
| | "print.absoluteImgPath" | ||
| | "print.imgToBase64" | ||
| | "print.includeVscodeStylesheets" | ||
| | "print.onFileSave" | ||
| | "print.theme" | ||
| | "print.validateUrls" | ||
| | "syntax.decorationFileSizeLimit" // To be superseded. | ||
| | "syntax.plainTheme" // To be superseded. | ||
| | "tableFormatter.enabled" | ||
| | "tableFormatter.normalizeIndentation" | ||
| | "theming.decoration.renderCodeSpan" // <- "syntax.decorations" | ||
| | "theming.decoration.renderHardLineBreak" | ||
| | "theming.decoration.renderLink" | ||
| | "theming.decoration.renderParagraph" | ||
| | "theming.decoration.renderStrikethrough" // <- "syntax.decorations" | ||
| | "theming.decoration.renderTrailingSpace" | ||
| | "toc.downcaseLink" | ||
| | "toc.levels" | ||
| | "toc.omittedFromToc" // To be superseded. | ||
| | "toc.orderedList" | ||
| | "toc.plaintext" | ||
| | "toc.slugifyMode" | ||
| | "toc.unorderedList.marker" | ||
| | "toc.updateOnSave" | ||
| ; | ||
|
|
||
| export default KnownKey; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| "use strict"; | ||
|
|
||
| import * as vscode from "vscode"; | ||
| import { IConfigurationFallbackMap } from "./manager"; | ||
|
|
||
| /** | ||
| * Configuration keys that are no longer supported, | ||
| * and will be removed in the next major version. | ||
| */ | ||
| export const deprecated: readonly string[] = [ | ||
| "syntax.decorations", | ||
| ]; | ||
|
|
||
| export const fallbackMap: IConfigurationFallbackMap = { | ||
|
|
||
| "theming.decoration.renderCodeSpan": (scope): boolean => { | ||
| const config = vscode.workspace.getConfiguration("markdown.extension", scope); | ||
| const old = config.get<boolean | null>("syntax.decorations"); | ||
| if (old === null || old === undefined) { | ||
| return config.get<boolean>("theming.decoration.renderCodeSpan")!; | ||
| } else { | ||
| return old; | ||
| } | ||
| }, | ||
|
|
||
| "theming.decoration.renderStrikethrough": (scope): boolean => { | ||
| const config = vscode.workspace.getConfiguration("markdown.extension", scope); | ||
| const old = config.get<boolean | null>("syntax.decorations"); | ||
| if (old === null || old === undefined) { | ||
| return config.get<boolean>("theming.decoration.renderStrikethrough")!; | ||
| } else { | ||
| return old; | ||
| } | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| "use strict"; | ||
|
|
||
| import * as vscode from "vscode"; | ||
| import type IDisposable from "../IDisposable"; | ||
| import type KnownKey from "./KnownKey"; | ||
| import { deprecated, fallbackMap } from "./defaultFallback"; | ||
|
|
||
| export type IConfigurationFallbackMap = { readonly [key in KnownKey]?: (scope?: vscode.ConfigurationScope) => any; }; | ||
|
|
||
| export interface IConfigurationManager extends IDisposable { | ||
|
|
||
| /** | ||
| * Gets the value that an our own key denotes. | ||
| * @param key The configuration key. | ||
| * @param scope The scope, for which the configuration is asked. | ||
| */ | ||
| get<T>(key: KnownKey, scope?: vscode.ConfigurationScope): T; | ||
|
|
||
| /** | ||
| * Gets the value that an absolute identifier denotes. | ||
| * @param section The dot-separated identifier (usually a setting ID). | ||
| * @param scope The scope, for which the configuration is asked. | ||
| */ | ||
| getByAbsolute<T>(section: string, scope?: vscode.ConfigurationScope): T | undefined; | ||
| } | ||
|
|
||
| /** | ||
| * This is currently just a proxy that helps mapping our configuration keys. | ||
| */ | ||
| class ConfigurationManager implements IConfigurationManager { | ||
|
|
||
| private readonly _fallback: IConfigurationFallbackMap; | ||
|
|
||
| constructor(fallback: IConfigurationFallbackMap, deprecatedKeys: readonly string[]) { | ||
| this._fallback = Object.assign<IConfigurationFallbackMap, IConfigurationFallbackMap>(Object.create(null), fallback); | ||
| this.showWarning(deprecatedKeys); | ||
| } | ||
|
|
||
| public dispose(): void { } | ||
|
|
||
| /** | ||
| * Shows an error message for each deprecated key, to help user migrate. | ||
| * This is async to avoid blocking instance creation. | ||
| */ | ||
| private async showWarning(deprecatedKeys: readonly string[]): Promise<void> { | ||
| for (const key of deprecatedKeys) { | ||
| const value = vscode.workspace.getConfiguration("markdown.extension").get(key); | ||
| if (value !== undefined && value !== null) { | ||
| // We are not able to localize this string for now. | ||
| // 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.`); | ||
|
There was a problem hiding this comment. 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.) There was a problem hiding this comment.
No. Lines 355 to 359 in 17c7bfc I've set the default value of the deprecated setting to
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 There was a problem hiding this comment.
Then this is good.
From my experience most of the users know little about the settings... But for options like There was a problem hiding this comment.
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
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 There was a problem hiding this comment.
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. 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":
There was a problem hiding this comment.
Do you think changing
Windows-1252 was popular in Western Europe.
OK. I'll keep it in mind when writing docs. There was a problem hiding this comment.
Ah, I didn't notice that. I like (just)
Many thanks. #TodayILearned |
||
| } | ||
| } | ||
| } | ||
|
|
||
| public get<T>(key: KnownKey, scope?: vscode.ConfigurationScope): T { | ||
| const fallback = this._fallback[key]; | ||
| if (fallback) { | ||
| return fallback(scope); | ||
| } else { | ||
| return vscode.workspace.getConfiguration("markdown.extension", scope).get<T>(key)!; | ||
| } | ||
| } | ||
|
|
||
| public getByAbsolute<T>(section: string, scope?: vscode.ConfigurationScope): T | undefined { | ||
| if (section.startsWith("markdown.extension.")) { | ||
| return this.get<T>(section.slice(19) as KnownKey, scope); | ||
| } else { | ||
| return vscode.workspace.getConfiguration(undefined, scope).get<T>(section); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export const configManager = new ConfigurationManager(fallbackMap, deprecated); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess we can directly use
...decoration.codeSpanand so onThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😥.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😓.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, it is VS Code's fault. microsoft/vscode#70589
We should not make it worse by introducing custom objects.
TypeScript's settings are also a beast. 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upvoted 👍
Fine, let's just use the simple way (the flat list).