-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[material_ui] Remove unconditional dart:io import from about.dart #12774
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // Copyright 2013 The Flutter Authors | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'dart:io' show Platform; | ||
|
|
||
| /// The file name of the currently running executable. | ||
| /// | ||
| /// Used as the fallback application name by the about dialog widgets when | ||
| /// no `Title` ancestor is available. | ||
| String get executableName => Platform.resolvedExecutable.split(Platform.pathSeparator).last; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| // Copyright 2013 The Flutter Authors | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| /// The file name of the currently running executable. | ||
| /// | ||
| /// There is no executable on the web, so this is always the empty string. | ||
| String get executableName => ''; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| changelog: | | ||
| - Removes the unconditional `dart:io` import from `AboutDialog`, so that `material_ui` is correctly detected as supporting the web platform and `showAboutDialog` no longer throws on the web when no application name is available. | ||
| version: patch |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |||||
| // found in the LICENSE file. | ||||||
|
|
||||||
| import 'dart:async'; | ||||||
| import 'dart:io' show Platform; | ||||||
| import 'dart:ui'; | ||||||
|
|
||||||
| import 'package:cupertino_ui/cupertino_ui.dart'; | ||||||
|
|
@@ -16,6 +17,47 @@ void main() { | |||||
| LicenseRegistry.reset(); | ||||||
| }); | ||||||
|
|
||||||
| Widget buildWithoutTitle(Widget child) { | ||||||
| return MediaQuery( | ||||||
| data: const MediaQueryData(), | ||||||
| child: Localizations( | ||||||
| locale: const Locale('en', 'US'), | ||||||
| delegates: const <LocalizationsDelegate<dynamic>>[ | ||||||
| DefaultMaterialLocalizations.delegate, | ||||||
| DefaultWidgetsLocalizations.delegate, | ||||||
| ], | ||||||
| child: Directionality( | ||||||
| textDirection: TextDirection.ltr, | ||||||
| child: Theme(data: ThemeData(), child: child), | ||||||
| ), | ||||||
| ), | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| testWidgets( | ||||||
| 'AboutDialog defaults the application name to the executable name without a Title ancestor', | ||||||
| (WidgetTester tester) async { | ||||||
| // Regression test for https://github.com/flutter/flutter/issues/191887. | ||||||
| await tester.pumpWidget(buildWithoutTitle(const AboutDialog())); | ||||||
|
|
||||||
| final String expectedName = Platform.resolvedExecutable.split(Platform.pathSeparator).last; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the conditionally imported
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Computing the expected value from |
||||||
| expect(find.text(expectedName), findsOneWidget); | ||||||
| }, | ||||||
| skip: kIsWeb, // [intended] There is no executable name on the web. | ||||||
| ); | ||||||
|
|
||||||
| testWidgets( | ||||||
| 'AboutDialog does not throw on the web without a Title ancestor', | ||||||
| (WidgetTester tester) async { | ||||||
| // Regression test for https://github.com/flutter/flutter/issues/191887. | ||||||
| await tester.pumpWidget(buildWithoutTitle(const AboutDialog())); | ||||||
|
|
||||||
| expect(tester.takeException(), isNull); | ||||||
| expect(find.byType(AboutDialog), findsOneWidget); | ||||||
| }, | ||||||
| skip: !kIsWeb, // [intended] Exercises the web-specific fallback. | ||||||
| ); | ||||||
|
|
||||||
| testWidgets('Material3 has sentence case labels', (WidgetTester tester) async { | ||||||
| await tester.pumpWidget( | ||||||
| MaterialApp( | ||||||
|
|
||||||
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.
Unconditionally importing
dart:ioin a test file will cause compilation failures when running tests on the web (e.g., viaflutter test --platform chrome). Since this PR aims to enable web support, we should avoid unconditionaldart:ioimports in the test file as well. We can use a conditional import to import the platform-specific executable name helper instead.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.
dart:iocompiles on the web (it just throws at runtime), so an unconditional import in a test file does not break web test compilation. This file passes withflutter test --platform chromeas-is, and the only test that touchesPlatformis skipped on the web withskip: kIsWeb. Other tests in this repo (e.g.test/l10n/translations_test.dart) importdart:iothe same way. Importingsrc/_about_io.dartfrom a test would also couple the test to a private implementation file, so I would prefer to keep the plain import.