[State Restoration] RestorableBoolN #71653
Conversation
|
Gold has detected about 62 untriaged digest(s) on patchset 3. |
|
Gold has detected about 30 untriaged digest(s) on patchset 4. |
|
Gold has detected about 49 untriaged digest(s) on patchset 4. |
| @@ -413,7 +413,7 @@ abstract class RestorableProperty<T> extends ChangeNotifier { | |||
| /// [RestorableProperty]. Whenever new restoration data has been provided to | |||
| /// the [RestorationMixin] the property is registered to, either this method | |||
| /// or [fromPrimitives] is called before [initWithValue] is invoked. | |||
| T createDefaultValue(); | |||
| T? createDefaultValue(); | |||
There was a problem hiding this comment.
Why the ? here and below? Wouldn't you just specify a nullable type for T, e.g. class Foo extends RestorableProperty<bool?> { ... }.
There was a problem hiding this comment.
Ahh, didn't realize that I could do that :)
| @@ -125,7 +125,50 @@ abstract class RestorableValue<T> extends RestorableProperty<T> { | |||
|
|
|||
| @mustCallSuper | |||
| @override | |||
| void initWithValue(T value) { | |||
| void initWithValue(T? value) { | |||
There was a problem hiding this comment.
Same question as above, if this needs to be nullable, wouldn't you just specify T as something nullable?
| @@ -185,6 +228,35 @@ class _RestorablePrimitiveValue<T extends Object> extends RestorableValue<T> { | |||
| } | |||
| } | |||
|
|
|||
| // _RestorablePrimitiveValueN and its subclasses allows null values. | |||
| class _RestorablePrimitiveValueN<T extends Object> extends RestorableValueN<T> { | |||
There was a problem hiding this comment.
I believe you can revert all changes you made to widgets/restoration.dart and above this line in this file by just changing this line to:
| class _RestorablePrimitiveValueN<T extends Object> extends RestorableValueN<T> { | |
| class _RestorablePrimitiveValueN<T extends Object?> extends RestorableValue<T> { |
The Object? ensures that T can be null, if necessary.
After you do that, a lot of ? below in this file can also be removed, I think.
There was a problem hiding this comment.
(With that in place, _RestorablePrimitiveValue can probably share some implementation with _RestorablePrimitiveValueN by just making _RestorablePrimitiveValue extend _RestorablePrimitiveValueN.)
| @@ -185,6 +228,35 @@ class _RestorablePrimitiveValue<T extends Object> extends RestorableValue<T> { | |||
| } | |||
There was a problem hiding this comment.
Can you also update the comment above _RestorablePrimitiveValue that explains how we could introduce nullable versions and make it point to _RestorablePrimitiveValueN?
| assert(debugIsSerializableForRestoration(_defaultValue)), | ||
| // _RestorablePrimitiveValueN and its subclasses allows for null values. | ||
| // See [_RestorablePrimitiveValue] for the non-nullable version of this class. | ||
| class _RestorablePrimitiveValueN<T extends Object?> extends RestorableValue<T> { |
There was a problem hiding this comment.
Just for my own understanding, the only reason with it is <T extends Object> here instead of just <T> is because toPrimitives returns an Object and the generic property value has to be at least a subclass of Object?
There was a problem hiding this comment.
I think this just makes it explicit that T may be nullable.
| // anticipation of NNBD (non-nullability by default). | ||
| class _RestorablePrimitiveValue<T extends Object> extends _RestorablePrimitiveValueN<T> { | ||
| _RestorablePrimitiveValue(T _defaultValue) | ||
| : assert(_defaultValue != null), |
There was a problem hiding this comment.
Are the asserts here necessary, since non-nullability is baked into the subclass? One wouldn't be able to instantiate a _RestorablePrimitiveValue with null passed in to begin with. Then, the class would simply be:
// _RestorablePrimitiveValue and its subclasses do not allow null values in
// anticipation of NNBD (non-nullability by default).
class _RestorablePrimitiveValue<T extends Object> extends _RestorablePrimitiveValueN<T> {
_RestorablePrimitiveValue(T _defaultValue) : super(_defaultValue);
}There was a problem hiding this comment.
Hm, I guess I can't think of a way to write a test for this to prevent a regression if someone were to make it nullable by accident, so the asserts act as sort of a line of defense?
There was a problem hiding this comment.
Eventually, we can remove these asserts. For now we are keeping them for people who are not running their app with null-safety enabled.
| // _RestorablePrimitiveValueN and its subclasses allows for null values. | ||
| // See [_RestorablePrimitiveValue] for the non-nullable version of this class. |
There was a problem hiding this comment.
Looks like this comment was accidentally copied unchanged from _RestorablePrimitiveValueN. Maybe update it to reflect that _RestorablePrimitiveValue and subclasses are non-nullable and link to _RestorablePrimitiveValueN for the nullable version?
There was a problem hiding this comment.
Sorry about that, the way that GitHub shows the diffs for this change and how similar the names of both classes were made me really confused, so I ran into some weird hiccups
| assert(value != null); | ||
| super.value = value; | ||
| } | ||
|
|
||
| @override | ||
| T fromPrimitives(Object? serialized) { | ||
| assert(serialized != null); |
There was a problem hiding this comment.
replace the line below this one with return super. fromPrimitives(serialized)?
There was a problem hiding this comment.
Same in toPrimitives: just call super.toPrimitives after the assert?
| /// A [RestorableProperty] that knows how to store and restore a [bool] that is | ||
| /// nullable. | ||
| /// | ||
| /// {@macro flutter.widgets.RestorableNum} |
There was a problem hiding this comment.
Add a "see also" section linking to the non-nullable version?
Similarly, add the same section to RestorableBool to link to this one?
| // anticipation of NNBD (non-nullability by default). | ||
| class _RestorablePrimitiveValue<T extends Object> extends _RestorablePrimitiveValueN<T> { | ||
| _RestorablePrimitiveValue(T _defaultValue) | ||
| : assert(_defaultValue != null), |
There was a problem hiding this comment.
Eventually, we can remove these asserts. For now we are keeping them for people who are not running their app with null-safety enabled.
| assert(debugIsSerializableForRestoration(_defaultValue)), | ||
| // _RestorablePrimitiveValueN and its subclasses allows for null values. | ||
| // See [_RestorablePrimitiveValue] for the non-nullable version of this class. | ||
| class _RestorablePrimitiveValueN<T extends Object?> extends RestorableValue<T> { |
There was a problem hiding this comment.
I think this just makes it explicit that T may be nullable.
Description
Implements a nullable version of the RestorableBool property, which is necessary for widgets like
Checkboxwhere a null value whentristateis true.Related Issues
#62916
Tests
I added the following tests:
Checklist
Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.