Skip to content

feat: conditionally apply cursive italics based on .leetcode boolean config#2

Open
ravikiranp123 wants to merge 1 commit into
NikkyAmresh:mainfrom
ravikiranp123:feat/workspace-fonts
Open

feat: conditionally apply cursive italics based on .leetcode boolean config#2
ravikiranp123 wants to merge 1 commit into
NikkyAmresh:mainfrom
ravikiranp123:feat/workspace-fonts

Conversation

@ravikiranp123

Copy link
Copy Markdown

This PR moves the workspace font styling logic out of the global VS Code settings namespace (package.json) and into the project-level .leetcode configuration schema, providing users with much more granular control over their environment.
Key Changes:

  • Config Migration: Removed leetcodePractice.applyWorkspaceFontSettings from VS Code's global settings. It is now read dynamically via getEffectiveConfig().
  • Independent Controls: Added two new properties to the .leetcode schema:
    • editorFontFamily: Allows you to specify the exact font to inject into the workspace (defaults to "Fira Code iScript" to maintain legacy LCEX styling).
    • editorCursiveItalics: A dedicated boolean (defaults to true) that controls whether the cursive italic textMateRules for comments and keywords are applied.
  • Decoupled Styling: The textMate rule injection is no longer hardcoded to trigger based on the font name. Users can now cleanly mix and match settings (e.g., keeping the Fira Code iScript font but disabling the cursive comments by setting "editorCursiveItalics": false).
  • Clean Cleanup: If a user disables the cursive italics, the extension now cleanly wipes those specific textMateRules from the VS Code workspace settings.json rather than leaving behind an empty array.

@NikkyAmresh

Copy link
Copy Markdown
Owner

Thanks for this, the decoupling of font and italics is a good direction and it works in my testing. A few things need fixing before merge though.

Blocking

  1. The config UI wipes these new keys on save. The .leetcode custom editor (LeetcodeConfigEditor.ts) rewrites the entire file with only the fields it knows about, so applyWorkspaceFontSettings, editorFontFamily, and editorCursiveItalics are silently dropped as soon as anyone saves through the UI. This is the same root cause as the path issue in feat: support custom local study plans via json #3, so a single fix helps both PRs: either add these fields to the config editor UI (checkbox, font input, checkbox), or make the editor preserve unknown keys when serializing.

  2. The default silently disables existing behavior. On main, the font and italic rules apply automatically whenever a .leetcode marker exists. With applyWorkspaceFontSettings defaulting to false, every existing user loses that styling after upgrading unless they opt in, which contradicts the "maintain legacy LCEX styling" goal in the description. Please default it to true.

  3. Stale settings are never cleaned up when the feature is off. If a previous version already wrote editor.fontFamily and the textMate rules into workspace settings, the new early return (if (!lcexCfg.applyWorkspaceFontSettings) return;) means they stay there forever. The cleanup you added for editorCursiveItalics: false should also run when applyWorkspaceFontSettings is false: if the current workspace values match what LCEX would have written, remove them.

Minor

  • The new fields are read only from .leetcode with no VS Code settings fallback in getEffectiveConfig(), unlike every other field. Please add the fallback (or note deliberately that these are workspace-file-only).
  • The PR description says leetcodePractice.applyWorkspaceFontSettings was removed from VS Code's global settings, but that setting never existed in package.json. Please update the description.
  • There is a leftover draft comment in LeetcodePracticeEditorSettings.ts ("update with {} clears the properties... wait, updating with undefined deletes the setting entirely"). Please clean it up.

Happy to merge once the blocking items are addressed.

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