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.

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

CupertinoTabView is not hot reload friendly #43574

Open
InMatrix opened this issue Oct 26, 2019 · 10 comments
Open

CupertinoTabView is not hot reload friendly #43574

InMatrix opened this issue Oct 26, 2019 · 10 comments
Labels
f: cupertino flutter/packages/flutter/cupertino repository. found in release: 1.20 Found to occur in 1.20 framework flutter/packages/flutter repository. See also f: labels. from: study Reported in a UX study. has reproducible steps The issue is ready to work on. t: hot reload Reloading code during "flutter run".

Comments

@InMatrix
Copy link

InMatrix commented Oct 26, 2019

Hot reload won't apply changes made to the builder of a CupertinoTabView. A hot restart is required to see the effect of those changes. This was hit by participants in a UX study and no one had any clue about why hot reload stopped working. The code below can reproduce the issue:

import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

void main() => runApp(MyApp());

class MyApp extends StatelessWidget {
  Widget build(BuildContext context) {
    return CupertinoApp(
      home: HomeScreen(),
    );
  }
}

class HomeScreen extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return CupertinoTabScaffold(
      tabBar: CupertinoTabBar(items: [
        BottomNavigationBarItem(
          icon: Icon(CupertinoIcons.home),
          title: Text('Home'),
        ),
        BottomNavigationBarItem(
          icon: Icon(CupertinoIcons.settings),
          title: Text('Settings'),
        ),
      ]),
      tabBuilder: (context, index) {
        if (index == 0) {
          return Center(
            child: Text("Home Screen",
                style: Theme.of(context).textTheme.display1),
          );
        } else {
          return CupertinoTabView(
            builder: (context) {
              return Center(
                // Changing the text style of the Text widget below requires a
                // hot restart to see its effect.
                child: Text("Settings Screen",
                    style: Theme.of(context).textTheme.display1),
              );
            },
          );
        }
      },
    );
  }
}

Cc: @jacob314 @Hixie

@InMatrix InMatrix added t: hot reload Reloading code during "flutter run". from: study Reported in a UX study. f: cupertino flutter/packages/flutter/cupertino repository. labels Oct 26, 2019
@InMatrix InMatrix added this to Call for proposals in UX Research Results Oct 26, 2019
@jacob314
Copy link
Contributor

jacob314 commented Oct 27, 2019

@pq here is the quick fix that we could suggest any time there is a closure within a build method to keep users writing code that will hot reload well even if there are bugs in widgets like there was for CupertinoTabView. This code pattern is also reasonable to suggest in general as it helps keep build methods from getting too large.
Before:
Screen Shot 2019-10-26 at 6 58 13 PM

After:
Screen Shot 2019-10-26 at 7 00 59 PM

@jacob314
Copy link
Contributor

jacob314 commented Oct 27, 2019

@DaveShuckerow you've been recommending this sort of pattern of refactors for unrelated style reasons while reviewing the DevTools in Flutter code. Would you like a general lint and quick fix for this case?

@jacob314
Copy link
Contributor

jacob314 commented Oct 27, 2019

#43585
I've added a pull request that makes @InMatrix's original code hot reload properly.
Users who use CupertinoPageRoute directly can still end up with code that won't hot reload but I'm having trouble figuring out how I would fix that case.

@InMatrix
Copy link
Author

InMatrix commented Oct 28, 2019

This quick fix is certainly helpful, but it's hard to discover. Is there something we can do from the framework side or the VM side to make code in such a closure reloadable?

Edit: I missed the pull request in my initial comment. I wonder how common a problem this is in other builder closures.

@jacob314
Copy link
Contributor

jacob314 commented Oct 29, 2019

#43585
would eliminate the need for any changes on the user side for this case. Hot reloads on the builder would just work as expected.
On the other hand, closures passed to the onGenerateRoute named parameter to CupertinoTabView will not hot reload and as far as I understand it from talking to @xster will never hot reload which is by design with the current routing model.
In general, I think a reasonable mental model to give users is that closures defining routing are not hot-reloadable but it is reasonable to expect that closures passed to arguments named builder are hot reloadable. We should audit all methods named builder in the framework to verify that they are hot reloadable and for cases where they cannot be hot reloadable we should deprecate the names and provide a different name that makes it less surprising that hot reload is not possible. Another option would be to provide an annotation on arguments to widgets that are not hot reloadable make it easy to provide lints that explain exactly what went wrong and what the fixes.

@jacob314
Copy link
Contributor

jacob314 commented Oct 29, 2019

Fyi @goderbauer I hear you are investigating some enhancements to routing that might make some code like this more amenable to hot reload.

@jacob314
Copy link
Contributor

jacob314 commented Oct 29, 2019

https://docs.google.com/document/d/1Q0jx0l4-xymph9O6zLaOY4d_f7YFpNWX_eGbzYxr9wY/edit#heading=h.l6kdsrb6j9id <-- navigator proposal that might enable code like this to be more easily hot-reloadable.

@xster
Copy link
Member

xster commented Nov 15, 2019

Seems like the reason is each CupertinoTabView is a parallel navigation stack whose first page is really following

https://api.flutter.dev/flutter/widgets/ModalRoute/buildPage.html

This method is only called when the route is first built, and rarely thereafter. In particular, it is not automatically called again when the route's state changes unless it uses ModalRoute.of. For a builder that is called every time the route's state changes, consider buildTransitions. For widgets that change their behavior when the route's state changes, consider ModalRoute.of to obtain a reference to the route; this will cause the widget to be rebuilt each time the route changes state.

On the one hand, it's very similar to other edge triggered imperative calls like showDialog or showModalBottomSheet where you you rebuilt the 'parent' to the caller and changed the builder closure instance given to the call to [showDialog] it, nothing happens. The closure is only run once in an edge triggered way. (These route builders have a builder instead of taking a child to make Navigator.of(context) get the right context underneath it). Similarly, MaterialPageRoute.builder and CupertinoPageRoute.builder don't change if the builder closure changed.

On the other hand, from the outside, this builder kinda looks very similar to CupertinoTabScaffold.tabBuilder which has no routes involved and does hot reload if the closure changes because it's entirely level-triggered.

The fact that some builders are invoked once (indirectly) imperatively and some declaratively is indeed not obvious at all and confusing. CupertinoTabView.builder has some dartdoc but it's likely not enough.

I think I'm leaning towards the change @jacob314 is proposing. Perhaps in addition, we should deprecate the builder arg and call it defaultRouteBodyBuilder (don't know what would be a good name) or some such to make it sound more level-triggered-y?

@Hixie Hixie added the framework flutter/packages/flutter repository. See also f: labels. label Nov 26, 2019
@jmagman jmagman added the tool Affects the "flutter" command-line tool. See also t: labels. label Dec 3, 2019
@jonahwilliams jonahwilliams removed the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 9, 2020
@jmagman jmagman added this to Awaiting triage in Tools - hot reload review Jan 10, 2020
@jonahwilliams jonahwilliams moved this from Awaiting triage to Engineer reviewed in Tools - hot reload review Jan 14, 2020
@xster xster moved this from Awaiting triage to Engineer reviewed in iOS Platform - Cupertino widget review Mar 12, 2020
@InMatrix
Copy link
Author

InMatrix commented Apr 17, 2020

Does Navigator 2.0 address this issue?

@pedromassangocode
Copy link

pedromassangocode commented Sep 10, 2020

I was able to reproduce this on latest stable channel.

flutter doctor -v
[✓] Flutter (Channel stable, 1.20.3, on Mac OS X 10.15.6 19G2021, locale en)
    • Flutter version 1.20.3 at /Users/pedromassango/dev/SDKs/flutter_stable
    • Framework revision 216dee60c0 (9 days ago), 2020-09-01 12:24:47 -0700
    • Engine revision d1bc06f032
    • Dart version 2.9.2

[✓] Android toolchain - develop for Android devices (Android SDK version 30.0.1)
    • Android SDK at /Users/pedromassango/Library/Android/sdk
    • Platform android-30, build-tools 30.0.1
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 11.7)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 11.7, Build version 11E801a
    • CocoaPods version 1.9.3

[!] Android Studio (version 4.0)
    • Android Studio at /Applications/Android Studio.app/Contents
    ✗ Flutter plugin not installed; this adds Flutter specific functionality.
    ✗ Dart plugin not installed; this adds Dart specific functionality.
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)

[✓] IntelliJ IDEA Community Edition (version 2020.2.1)
    • IntelliJ at /Applications/IntelliJ IDEA CE.app
    • Flutter plugin version 48.1.4
    • Dart plugin version 202.7206

[✓] VS Code (version 1.48.2)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.13.2

[✓] Connected device (1 available)
    • sdk gphone x86 arm (mobile) • emulator-5556 • android-x86 • Android 11 (API 30) (emulator)

! Doctor found issues in 1 category.
Process finished with exit code 0

@pedromassangocode pedromassangocode added found in release: 1.20 Found to occur in 1.20 has reproducible steps The issue is ready to work on. labels Sep 10, 2020
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. found in release: 1.20 Found to occur in 1.20 framework flutter/packages/flutter repository. See also f: labels. from: study Reported in a UX study. has reproducible steps The issue is ready to work on. t: hot reload Reloading code during "flutter run".
Projects
Tools - hot reload review
  
Engineer reviewed
UX Research Results
  
Call for proposals
Development

No branches or pull requests

9 participants
@xster @InMatrix @Hixie @jmagman @jacob314 @jonahwilliams @pedromassangocode and others