[go: nahoru, domu]

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 · 12 comments
Open

CupertinoTabView is not hot reload friendly #43574

InMatrix opened this issue Oct 26, 2019 · 12 comments
Labels
f: cupertino flutter/packages/flutter/cupertino repository found in release: 3.3 Found to occur in 3.3 found in release: 3.7 Found to occur in 3.7 framework flutter/packages/flutter repository. See also f: labels. from: study Reported in a UX study has reproducible steps The issue has been confirmed reproducible and is ready to work on t: hot reload Reloading code during "flutter run" team-design Owned by Design Languages team triaged-design Triaged by Design Languages team

Comments

@InMatrix
Copy link

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

@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

#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

#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

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

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

Does Navigator 2.0 address this issue?

@pedromassangocode
Copy link

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 has been confirmed reproducible and is ready to work on labels Sep 10, 2020
@fzyzcjy
Copy link
Contributor
fzyzcjy commented Oct 22, 2022

Interesting, is the bug still there for many years?

@maheshmnj
Copy link
Member

Reproducible on stable 3.3 and master 3.7 changing CupertinoTabView.builder's widget does not update unless we hot restart.

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

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

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const CupertinoApp(
      home: HomeScreen(),
    );
  }
}

class HomeScreen extends StatelessWidget {
  const HomeScreen({super.key});

  @override
  Widget build(BuildContext context) {
    return CupertinoTabScaffold(
      backgroundColor: Colors.white,
      tabBar: CupertinoTabBar(items: const [
        BottomNavigationBarItem(
          icon: Icon(CupertinoIcons.home),
          label: 'Home',
        ),
        BottomNavigationBarItem(
          icon: Icon(CupertinoIcons.settings),
          label: 'Settings',
        ),
      ]),
      tabBuilder: (context, index) {
        if (index == 0) {
          return Center(
            child: Text("Home Screen",
                style: Theme.of(context).textTheme.headlineMedium),
          );
        } 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 changed",
                    style: Theme.of(context).textTheme.headlineMedium),
              );
            },
          );
        }
      },
    );
  }
}
flutter doctor -v (mac)
[✓] Flutter (Channel master, 3.7.0-13.0.pre.131, on macOS 13.1 22C65 darwin-arm64, locale en-IN)
    • Flutter version 3.7.0-13.0.pre.131 on channel master at /Users/mahesh/Development/flutter_master
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 0196e6050b (2 days ago), 2022-12-31 11:10:23 -0500
    • Engine revision 932591ec04
    • Dart version 3.0.0 (build 3.0.0-76.0.dev)
    • DevTools version 2.20.0
    • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks
      and upgrades.

[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.0-rc4)
    • Android SDK at /Users/mahesh/Library/Android/sdk
    • Platform android-33, build-tools 33.0.0-rc4
    • ANDROID_HOME = /Users/mahesh/Library/Android/sdk
    • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.12+0-b1504.28-7817840)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 14.0.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 14A400
    • CocoaPods version 1.11.3

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2021.2)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.12+0-b1504.28-7817840)

[✓] IntelliJ IDEA Community Edition (version 2021.2.1)
    • IntelliJ at /Applications/IntelliJ IDEA CE.app
    • Flutter plugin version 61.2.4
    • Dart plugin version 212.5080.8

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

[✓] Connected device (3 available)
    • iPhone 12 Pro (mobile) • 026D5789-9E78-4AD5-B1B2-3F8D4E7F65E4 • ios            • com.apple.CoreSimulator.SimRuntime.iOS-14-5 (simulator)
    • macOS (desktop)        • macos                                • darwin-arm64   • macOS 13.1 22C65 darwin-arm64
    • Chrome (web)           • chrome                               • web-javascript • Google Chrome 108.0.5359.124

[✓] HTTP Host Availability
    • All required HTTP hosts are available
    
• No issues found!
[✓] Flutter (Channel stable, 3.3.9, on macOS 12.6 21G115 darwin-arm, locale en-IN)
    • Flutter version 3.3.9 on channel stable at /Users/mahesh/Development/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision b8f7f1f986 (24 hours ago), 2022-11-23 06:43:51 +0900
    • Engine revision 8f2221fbef
    • Dart version 2.18.5
    • DevTools version 2.15.0

[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.0-rc4)
    • Android SDK at /Users/mahesh/Library/Android/sdk
    • Platform android-33, build-tools 33.0.0-rc4
    • ANDROID_HOME = /Users/mahesh/Library/Android/sdk
    • Java binary at: /Applications/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 11.0.12+0-b1504.28-7817840)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 14.0.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 14A400
    • CocoaPods version 1.11.3

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2021.2)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.12+0-b1504.28-7817840)

[✓] IntelliJ IDEA Community Edition (version 2021.2.1)
    • IntelliJ at /Applications/IntelliJ IDEA CE.app
    • Flutter plugin version 61.2.4
    • Dart plugin version 212.5080.8

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

[✓] Connected device (3 available)
    • iPhone 12 Pro (mobile) • 026D5789-9E78-4AD5-B1B2-3F8D4E7F65E4 • ios            •
      com.apple.CoreSimulator.SimRuntime.iOS-14-5 (simulator)
    • macOS (desktop)        • macos                                • darwin-arm64   • macOS 12.6 21G115 darwin-arm
    • Chrome (web)           • chrome                               • web-javascript • Google Chrome 107.0.5304.110

[✓] HTTP Host Availability
    • All required HTTP hosts are available

• No issues found!

@maheshmnj maheshmnj added found in release: 3.3 Found to occur in 3.3 and removed found in release: 1.20 Found to occur in 1.20 labels Jan 4, 2023
@maheshmnj maheshmnj added the found in release: 3.7 Found to occur in 3.7 label Jan 4, 2023
@InMatrix InMatrix moved this from Call for proposals to Action identified but unscheduled in UX Research Results Jan 4, 2023
@flutter-triage-bot flutter-triage-bot bot added multiteam-retriage-candidate team-design Owned by Design Languages team triaged-design Triaged by Design Languages team labels Jul 8, 2023
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: 3.3 Found to occur in 3.3 found in release: 3.7 Found to occur in 3.7 framework flutter/packages/flutter repository. See also f: labels. from: study Reported in a UX study has reproducible steps The issue has been confirmed reproducible and is ready to work on t: hot reload Reloading code during "flutter run" team-design Owned by Design Languages team triaged-design Triaged by Design Languages team
Projects
Tools - hot reload review
  
Engineer reviewed
UX Research Results
  
Action identified but unscheduled
Development

No branches or pull requests

11 participants