feat (Editor): Add an option to set the editor font family#1078
feat (Editor): Add an option to set the editor font family#1078GT-610 merged 2 commits intolollipopkit:mainfrom
Conversation
| No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated settings and UI to add a persistent Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add the `editorFontFamily` property to the settings storage and implement font family selection functionality on editor-related pages. Also, update all pages that use the editor settings to support the newly added font family option.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/view/page/storage/sftp.dart (1)
20-20:⚠️ Potential issue | 🟡 MinorRemove unused import flagged by pipeline.
The pipeline reports this import as unused. Since all settings now use
Stores.setting(from line 19), this import is no longer needed.🧹 Proposed fix
-import 'package:server_box/data/store/setting.dart';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/view/page/storage/sftp.dart` at line 20, Remove the unused import of server_box/data/store/setting.dart from lib/view/page/storage/sftp.dart; since the code uses Stores.setting (imported on line 19) and no symbols from Setting are referenced, delete the import statement to satisfy the pipeline and avoid unused-import warnings.
🧹 Nitpick comments (4)
.gitmodules (1)
12-12: Thebefore-sqlitebranch exists and can be used.The branch has been verified to exist in the fl_lib repository. Since the PR description already documents the reason for this dependency switch (avoiding an incomplete SQLite migration on main), consider tracking when the main branch is fixed so this can be reverted to the stable branch in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitmodules at line 12, The .gitmodules entry currently pins the fl_lib submodule to branch = before-sqlite; keep this change but add a short TODO comment in the same .gitmodules (or adjacent repo docs) noting why we pin to before-sqlite and a clear follow-up action to revert to the stable/main branch when the SQLite migration is completed (include the PR/issue number or a tracking label); ensure the note references the .gitmodules branch line so reviewers can find it easily and create a follow-up ticket if one does not exist.lib/view/page/storage/sftp.dart (1)
363-369: Redundant fetch ofeditorFontFamily.The value is fetched twice - once for the
.isEmptycheck and again for the value. Store in a local variable for efficiency.♻️ Proposed fix
+ final fontFamily = Stores.setting.editorFontFamily.fetch(); await EditorPage.route.go( context, args: EditorPageArgs( path: localPath, onSave: (_) async { // ... onSave implementation }, closeAfterSave: Stores.setting.closeAfterSave.fetch(), softWrap: Stores.setting.editorSoftWrap.fetch(), enableHighlight: Stores.setting.editorHighlight.fetch(), - fontFamily: Stores.setting.editorFontFamily.fetch().isEmpty - ? null - : Stores.setting.editorFontFamily.fetch(), + fontFamily: fontFamily.isEmpty ? null : fontFamily, ), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/view/page/storage/sftp.dart` around lines 363 - 369, The code redundantly calls Stores.setting.editorFontFamily.fetch() twice when building the EditorConfig; fetch the value once into a local variable (e.g., var fontFamily = Stores.setting.editorFontFamily.fetch()), then use fontFamily.isEmpty ? null : fontFamily for the fontFamily field. Update the surrounding builder (where closeAfterSave, softWrap, enableHighlight are set) to reference that local variable instead of calling fetch() twice.lib/view/page/setting/entries/app.dart (1)
475-481: Implementation is correct; minor efficiency improvement possible.The settings are consistently accessed via
_setting. Same pattern of double-fetchingeditorFontFamily- consider storing in a local variable.♻️ Proposed fix
+ final fontFamily = _setting.editorFontFamily.fetch(); await EditorPage.route.go( context, args: EditorPageArgs( text: text, lang: ProgLang.json, title: libL10n.setting, onSave: onSave, closeAfterSave: _setting.closeAfterSave.fetch(), softWrap: _setting.editorSoftWrap.fetch(), enableHighlight: _setting.editorHighlight.fetch(), - fontFamily: _setting.editorFontFamily.fetch().isEmpty - ? null - : _setting.editorFontFamily.fetch(), + fontFamily: fontFamily.isEmpty ? null : fontFamily, ), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/view/page/setting/entries/app.dart` around lines 475 - 481, The code double-fetches _setting.editorFontFamily when computing fontFamily; cache it in a local variable (e.g., final fam = _setting.editorFontFamily.fetch()) before the widget build and use that variable in the fontFamily expression to avoid redundant fetch() calls and improve efficiency; keep the other _setting.fetch() calls as-is.lib/view/page/storage/local.dart (1)
370-376: Inconsistent store access and redundant fetch.
Lines 370-372 use
SettingStore.instancewhile lines 373-375 useStores.settingfor accessing the same store. For consistency within this code block, use one or the other.
Stores.setting.editorFontFamily.fetch()is called twice - once for the.isEmptycheck and once for the value. Consider storing the result in a local variable.♻️ Proposed fix
+ final setting = SettingStore.instance; + final fontFamily = setting.editorFontFamily.fetch(); await EditorPage.route.go( context, args: EditorPageArgs( path: file.absolute.path, onSave: (_) { context.showSnackBar(libL10n.saved); setStateSafe(() {}); }, - closeAfterSave: SettingStore.instance.closeAfterSave.fetch(), - softWrap: SettingStore.instance.editorSoftWrap.fetch(), - enableHighlight: SettingStore.instance.editorHighlight.fetch(), - fontFamily: Stores.setting.editorFontFamily.fetch().isEmpty - ? null - : Stores.setting.editorFontFamily.fetch(), + closeAfterSave: setting.closeAfterSave.fetch(), + softWrap: setting.editorSoftWrap.fetch(), + enableHighlight: setting.editorHighlight.fetch(), + fontFamily: fontFamily.isEmpty ? null : fontFamily, ), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/view/page/storage/local.dart` around lines 370 - 376, Use a consistent store accessor and avoid calling editorFontFamily.fetch() twice: choose either SettingStore.instance or Stores.setting and use it for all four properties (closeAfterSave, editorSoftWrap/editorSoftWrap.fetch for softWrap, editorHighlight for enableHighlight, and editorFontFamily). Cache the result of editorFontFamily.fetch() in a local variable (e.g., final editorFont = <storeAccessor>.editorFontFamily.fetch()) and then set fontFamily to null if editorFont.isEmpty else editorFont; update the references to the chosen store accessor throughout this block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Outside diff comments: In `@lib/view/page/storage/sftp.dart`: - Line 20: Remove the unused import of server_box/data/store/setting.dart from lib/view/page/storage/sftp.dart; since the code uses Stores.setting (imported on line 19) and no symbols from Setting are referenced, delete the import statement to satisfy the pipeline and avoid unused-import warnings. --- Nitpick comments: In @.gitmodules: - Line 12: The .gitmodules entry currently pins the fl_lib submodule to branch = before-sqlite; keep this change but add a short TODO comment in the same .gitmodules (or adjacent repo docs) noting why we pin to before-sqlite and a clear follow-up action to revert to the stable/main branch when the SQLite migration is completed (include the PR/issue number or a tracking label); ensure the note references the .gitmodules branch line so reviewers can find it easily and create a follow-up ticket if one does not exist. In `@lib/view/page/setting/entries/app.dart`: - Around line 475-481: The code double-fetches _setting.editorFontFamily when computing fontFamily; cache it in a local variable (e.g., final fam = _setting.editorFontFamily.fetch()) before the widget build and use that variable in the fontFamily expression to avoid redundant fetch() calls and improve efficiency; keep the other _setting.fetch() calls as-is. In `@lib/view/page/storage/local.dart`: - Around line 370-376: Use a consistent store accessor and avoid calling editorFontFamily.fetch() twice: choose either SettingStore.instance or Stores.setting and use it for all four properties (closeAfterSave, editorSoftWrap/editorSoftWrap.fetch for softWrap, editorHighlight for enableHighlight, and editorFontFamily). Cache the result of editorFontFamily.fetch() in a local variable (e.g., final editorFont = <storeAccessor>.editorFontFamily.fetch()) and then set fontFamily to null if editorFont.isEmpty else editorFont; update the references to the chosen store accessor throughout this block. In `@lib/view/page/storage/sftp.dart`: - Around line 363-369: The code redundantly calls Stores.setting.editorFontFamily.fetch() twice when building the EditorConfig; fetch the value once into a local variable (e.g., var fontFamily = Stores.setting.editorFontFamily.fetch()), then use fontFamily.isEmpty ? null : fontFamily for the fontFamily field. Update the surrounding builder (where closeAfterSave, softWrap, enableHighlight are set) to reference that local variable instead of calling fetch() twice. ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: df18508a-04d6-469a-a053-7acaadee783a
📒 Files selected for processing (9)
.gitmoduleslib/data/res/github_id.dartlib/data/store/setting.dartlib/view/page/setting/entries/app.dartlib/view/page/setting/entries/editor.dartlib/view/page/setting/entries/ssh.dartlib/view/page/storage/local.dartlib/view/page/storage/sftp.dartpackages/fl_lib
Standardize the method for retrieving font settings on the editor page to avoid redundant calls to the `fetch()` method. Extract the font retrieval logic into variables or anonymous functions to improve code readability and performance.
Resolve #1022.
Add the
editorFontFamilyproperty to the settings storage and implement font family selection functionality on editor-related pages. Also, update all pages that use the editor settings to support the newly added font family option.Switch
fl_libtobefore-sqliteasmainbranch contains incomplete SQLite migrationSummary by CodeRabbit
New Features
Chores