Skip to content
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.

Sign up for GitHub

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

Migrate theme.dart to be null safe. #3633

Merged
merged 43 commits into from
Feb 8, 2022
Merged

Migrate theme.dart to be null safe. #3633

Changes from 1 commit
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
814d53b
Update globals.dart
polina-c Feb 4, 2022
8d5c3d8
ns
polina-c Feb 4, 2022
5044a39
ns
polina-c Feb 4, 2022
455f6c8
comments
polina-c Feb 4, 2022
9e71acb
ns
polina-c Feb 5, 2022
0bd4728
ns
polina-c Feb 5, 2022
59abcf8
Update theme.dart
polina-c Feb 5, 2022
f66fdf7
ignore
polina-c Feb 5, 2022
119d3f9
ideTheme
polina-c Feb 7, 2022
e810175
Update version.dart
polina-c Feb 7, 2022
1e5c40c
ns
polina-c Feb 4, 2022
6815f67
ns
polina-c Feb 5, 2022
a85d131
ns
polina-c Feb 5, 2022
81bc861
Update theme.dart
polina-c Feb 5, 2022
4cc8869
ignore
polina-c Feb 5, 2022
0bc089e
ideTheme
polina-c Feb 7, 2022
bffd848
Update utils.dart
polina-c Feb 7, 2022
b512d5e
Merge branch 'ns2' of github.com:polina-c/devtools into ns2
polina-c Feb 7, 2022
05b719f
Merge branch 'master' of github.com:flutter/devtools into ns2
polina-c Feb 7, 2022
d8a1b1f
Update about_dialog_test.dart
polina-c Feb 7, 2022
45b6625
Update theme.dart
polina-c Feb 7, 2022
5acf384
Update theme.dart
polina-c Feb 7, 2022
0c35da1
Update analytics_prompt_test.dart
polina-c Feb 7, 2022
23cf846
theme
polina-c Feb 7, 2022
c0eee12
Update banner_messages_test.dart
polina-c Feb 7, 2022
6bbd99f
Update code_size_attribution_test.dart
polina-c Feb 7, 2022
8469d8d
Update common_widgets_test.dart
polina-c Feb 7, 2022
0ecae82
Update cpu_profiler_test.dart
polina-c Feb 7, 2022
4f8a488
Update debugger_screen_test.dart
polina-c Feb 7, 2022
9811261
revert utils
polina-c Feb 7, 2022
033cfa0
Update debugger_screen_test.dart
polina-c Feb 7, 2022
f668f94
address comments
polina-c Feb 7, 2022
1f8ab25
Update chart_test.dart
polina-c Feb 7, 2022
03e432e
Update rebuild_counts.dart
polina-c Feb 7, 2022
784fde9
Update ide_theme_stub.dart
polina-c Feb 8, 2022
f8978fb
Update theme.dart
polina-c Feb 8, 2022
54d9999
Update theme.dart
polina-c Feb 8, 2022
ca2dcde
Merge branch 'master' of github.com:flutter/devtools into ns2
polina-c Feb 8, 2022
81a3dbc
Update theme.dart
polina-c Feb 8, 2022
d55c101
fix wrap
polina-c Feb 8, 2022
93a673b
Update theme_test.dart
polina-c Feb 8, 2022
d10eec2
Update vm_flag_widgets_test.dart
polina-c Feb 8, 2022
c280712
Update theme_test.dart
polina-c Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions packages/devtools_app/test/theme_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,6 @@ import 'package:flutter_test/flutter_test.dart';
void main() {
group('Theme', () {
ThemeData theme;
test('can be used without override', () {
theme = themeFor(isDarkTheme: true, ideTheme: null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of removing this test, why don't we just replace null with IdeTheme()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of the test is to verify everything works with null. We do not need this validation any more.
Other tests here cover non-null cases. Makes sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case an empty IdeTheme would be the equivalent of what used to be null, in terms of testing the "without override" case. The "without override" case is where a user runs DevTools via dart devtools instead of using the DevTools embedded in their IDE. So we still want to test the case where IdeTheme does not contain overrides for backgroundColor, foregroundColor, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

expect(theme.brightness, equals(Brightness.dark));
expect(theme.scaffoldBackgroundColor,
equals(ThemeData.dark().scaffoldBackgroundColor));

theme = themeFor(isDarkTheme: false, ideTheme: null);
expect(theme.brightness, equals(Brightness.light));
expect(theme.scaffoldBackgroundColor,
equals(ThemeData.light().scaffoldBackgroundColor));
});

test('can be inferred from override background color', () {
theme = themeFor(
Expand Down