Skip to content

Conversation

goderbauer
Copy link
Member

Description

CupertinoTabScaffold can now restore the currently selected tab index.

Related Issues

#62916

Tests

I added the following tests:

  • restoration test for CupertinoTabScaffold

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

@flutter-dashboard flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Oct 9, 2020
@google-cla google-cla bot added the cla: yes label Oct 9, 2020
@goderbauer goderbauer requested a review from justinmc October 9, 2020 21:25
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM, just two nits I think: a spelling error and an indentation.

@@ -289,43 +292,80 @@ class CupertinoTabScaffold extends StatefulWidget {
/// Defaults to true and cannot be null.
final bool resizeToAvoidBottomInset;

/// Restoration ID to save and restore the state of the [CupertinoTabScaffold].
///
/// This property only has an affect when no [controller] has been provided:
Copy link
Contributor

Choose a reason for hiding this comment

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

affect => effect

/// will persist and restore the currently selected tab index. If a
/// [controller] has been provided, it is the responsibility of the owner of
/// that controller to persist and restore it, e.g. by using a
/// [RestorableCupertinoTabController].
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there a bunch of new restorable versions of classes now to support state restoration? This is the first state restoration PR I've taken a close look at.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a few. Another example is the RestorableTextEditingController.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks, will study up.

}

_internalController?.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the indentation is off here if I'm seeing this right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good eye!

if (_value != null) {
// Scheduling a microtask for dispose to give other entities a chance
// to remove their listeners first.
scheduleMicrotask(_value!.dispose);
Copy link
Contributor

Choose a reason for hiding this comment

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

I had never heard of scheduleMicrotask before, good to know.

expect(find.text('Content 3'), findsNothing);
});

testWidgets('switch from internal to external controller with state restoration', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for testing the controller swap too! That logic was a bit complicated.

@fluttergithubbot fluttergithubbot merged commit 053ebf2 into flutter:master Oct 12, 2020
@goderbauer goderbauer deleted the cupertinotabscaffold branch November 11, 2020 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants