-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[ Widget Preview ] Persist "Filter by Selected File" toggle #176289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This change introduces `PersistentPreferences`, which allows for the widget previewer to save settings to disk. `PersistentPreferences` makes use of the existing `~/.flutter-devtools` directory used by DevTools for the same purpose, writing preferences in JSON format to `~/.flutter-devtools/.widget-preview`.
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.
Code Review
This pull request introduces persistence for widget preview settings by creating a PersistentPreferences class. The implementation is sound, but there are a few areas for improvement regarding error handling, API design, and test suite hygiene. Specifically, I've pointed out a case of silent error handling, a bug in the remove method, an unintuitive API return value, and some duplicated test code. Addressing these points will improve the robustness and maintainability of the new feature.
packages/flutter_tools/lib/src/widget_preview/persistent_properties.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/lib/src/widget_preview/persistent_properties.dart
Show resolved
Hide resolved
packages/flutter_tools/test/commands.shard/permeable/widget_preview/dtd_services_test.dart
Show resolved
Hide resolved
...widget_preview_scaffold.shard/widget_preview_scaffold/test/filter_by_selected_file_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/lib/src/widget_preview/persistent_properties.dart
Outdated
Show resolved
Hide resolved
|
|
||
| try { | ||
| const jsonEncoder = JsonEncoder.withIndent(' '); | ||
| file.writeAsStringSync('${jsonEncoder.convert(_map)}\n'); |
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.
We spawn a widget preview for each "window" in VS Code. If you have multiple windows open (which I don't think is uncommon), they could overwrite each others files here. It's a bit of an edge case, but it might be confusing. I wonder if we should re-read them on-change and then persist with just the one value updated?
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 think the only correct way to handle this is to register a file watcher that's responsive to changes in the preferences file, which then sends DTD events to the widget preview scaffold so it can update its state immediately.
I pulled most of this implementation from DevTools, so it'll have the same issue. Maybe this is something we revisit?
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 agree watching the file is probably overkill, but what about re-reading the file just before modifying, so if another process had saved a value, it's preserved rather than wipe? (just an idea, not suggesting it should be done to land this)
packages/flutter_tools/lib/src/widget_preview/persistent_properties.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/templates/widget_preview_scaffold/lib/src/dtd/dtd_services.dart.tmpl
Show resolved
Hide resolved
packages/flutter_tools/test/commands.shard/permeable/widget_preview/dtd_services_test.dart
Outdated
Show resolved
Hide resolved
|
autosubmit label was removed for flutter/flutter/176289, because - The status or check suite Linux_android_emu native_assets_android has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
autosubmit label was removed for flutter/flutter/176289, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
flutter/flutter@7811e89...65aca36 2025-10-02 [email protected] Roll Skia from 257c1f94afaa to 05c1f5803415 (4 revisions) (flutter/flutter#176402) 2025-10-02 [email protected] [ Widget Preview ] Fix resolution for workspace "hosted" dependencies (flutter/flutter#176358) 2025-10-02 [email protected] Roll Skia from b5d8ae8d3410 to 257c1f94afaa (6 revisions) (flutter/flutter#176389) 2025-10-02 [email protected] Delete Skia-specific performance overlay implementation (flutter/flutter#176364) 2025-10-02 [email protected] Roll Fuchsia Linux SDK from 1Ai6VL4vb_GdGnWhg... to Vnoygds8HtDUvGLCK... (flutter/flutter#176381) 2025-10-01 [email protected] [ Widget Preview ] Persist "Filter by Selected File" toggle (flutter/flutter#176289) 2025-10-01 [email protected] Roll Skia from c44a36470d07 to b5d8ae8d3410 (5 revisions) (flutter/flutter#176367) 2025-10-01 [email protected] Reapply "Update the AccessibilityPlugin::Announce method to account f… (flutter/flutter#176107) 2025-10-01 [email protected] Roll Dart SDK from 8ffec1435cf3 to 4f90a06328cb (3 revisions) (flutter/flutter#176369) 2025-10-01 [email protected] [ Tool / l10n ] Fix issue where localization generator assumed current directory was the target project (flutter/flutter#175881) 2025-10-01 [email protected] Make sure that a DateRangePickerDialog doesn't crash in 0x0 environments (flutter/flutter#173754) 2025-10-01 [email protected] Make sure that a DrawerButton doesn't crash in 0x0 environment (flutter/flutter#172948) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…176289) This change introduces `PersistentPreferences`, which allows for the widget previewer to save settings to disk. `PersistentPreferences` makes use of the existing `~/.flutter-devtools` directory used by DevTools for the same purpose, writing preferences in JSON format to `~/.flutter-devtools/.widget-preview`.
…176289) This change introduces `PersistentPreferences`, which allows for the widget previewer to save settings to disk. `PersistentPreferences` makes use of the existing `~/.flutter-devtools` directory used by DevTools for the same purpose, writing preferences in JSON format to `~/.flutter-devtools/.widget-preview`.
…176289) This change introduces `PersistentPreferences`, which allows for the widget previewer to save settings to disk. `PersistentPreferences` makes use of the existing `~/.flutter-devtools` directory used by DevTools for the same purpose, writing preferences in JSON format to `~/.flutter-devtools/.widget-preview`.
This change introduces
PersistentPreferences, which allows for the widget previewer to save settings to disk.PersistentPreferencesmakes use of the existing~/.flutter-devtoolsdirectory used by DevTools for the same purpose, writing preferences in JSON format to~/.flutter-devtools/.widget-preview.