flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
165.23k stars 27.26k forks source link

New feature: Need AutomaticKeepAliveClientMixin to work outside of ListView #152840

Open XuanTung95 opened 2 months ago

XuanTung95 commented 2 months ago

Use case

There are two pages for different orientations:

Portrait Mode: Uses PageA, which is complex with many widgets and multiple Navigators. Landscape Mode: Uses PageB, a simpler page.

To optimize performance, when switching to Landscape mode, PageA is completely removed from the widget tree. When switching back to Portrait mode, PageA is re-created. This helps with performance but causes the state of the Navigators and other widgets in PageA lost.

One might consider using Visibility.maintain(), Offstage(), or IndexedStack() to keep PageA in the tree when in Landscape mode. However, I tried all of these methods, and it still affect performance as long as PageA remains in the widget tree. I tried everything, but PageA still performs layout and triggers new frames.

Proposal

I found that ListView + AutomaticKeepAliveClientMixin already implemented similar feature. When keepAlive widget in ListView is out of the viewport, it's state is saved and can be restore when it's visible again. Unfortunately, KeepAlive is only available inside a ListView, I want this feature for normal widget.

New widget:

NewKeepAliveWidget(keepAlive: false, child: MyWidget());

When keepAlive = false the state of MyWidget and it's child is saved, the widget is removed from the widget tree. When keepAlive = true the state of MyWidget is restored. The behavior is just same as AutomaticKeepAliveClientMixin in ListView.

huycozy commented 2 months ago

However, I tried all of these methods, and it still affect performance as long as PageA remains in the widget tree. I tried everything, but PageA still performs layout and triggers new frames.

Can you provide a sample code illustrating this performance issue?

XuanTung95 commented 2 months ago

Here is the sample code to demonstrate the need for the new widget.

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

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

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
      home: const MyAppApp(),
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    bool newWidget = false; /// change me
    return LayoutBuilder(
      builder: (context, size) {
        final orientation = MediaQuery.orientationOf(context);
        int index = orientation == Orientation.portrait ? 0 : 1;
        print("index == $index");
        return IndexedStack(
          index: index,
          children: [
            Visibility(
                maintainState: true,
                visible: index == 0,
                child: Offstage(offstage: index != 0, child: (newWidget && index != 0) ? const SizedBox() : const PageA())),
            const SizedBox.expand(child: PageB()),
          ],
        );
      },
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text("PageB"),
      ),
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text("PageA"),
      ),
      body: const IndexedStack(
        index: 0,
        children: [
          ExpensiveList(),
        ],
      ),
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    return CustomScrollView(
      slivers: [
        SliverGrid(
          delegate: SliverChildBuilderDelegate((context, index) {
            return const Placeholder();
          }),
          gridDelegate: const SliverGridDelegateWithMaxCrossAxisExtent(
            maxCrossAxisExtent: 10,
          ),
        )
      ],
    );
  }
}
huycozy commented 2 months ago

Thanks for your update. I see a comment on your code:

bool newWidget = false; /// change me

When running the sample code with newWidget as false, I see two pages transit normally.

Can you share the steps to reproduce the issue as well? And how do you evaluate the performance? (observe frames via Devtools or something else?) Please be more specific on the use case that will help to make the proposal to be clearer.

I think this is more like an implementation issue. Please also check the given solution at https://github.com/flutter/flutter/issues/11655#issuecomment-563036409 to see if it helps. If you persist with creating a new widget, you need to provide more specifics on how the widget is to be designed and works, maybe you need a design document for it.

XuanTung95 commented 2 months ago

@huycozy

case 1:

  1. newWidget = false
  2. Open app in Portrait mode
  3. Rotate device to Landscape mode

You will notice the performance problem when flutter try to render the Landscape image, a lot of time spending to layout the ExpensiveList.

case 2:

  1. newWidget = true
  2. Open app in Portrait mode
  3. Rotate device to Landscape mode

ExpensiveList does not exists in Landscape mode in this case so it render much faster.

You can notice the difference by eye or use the performance tool to compare. You can increase the number of widget in ExpensiveList to make it build longer so it's easier to see.

If you persist with creating a new widget, you need to provide more specifics on how the widget is to be designed and works, maybe you need a design document for it.

How it works.

It work just like Offstage widget but no more layout, painting or other works. It's just there to keep the state alive.

Sorry I don't have any idea how such a widget can be implemented with the current framework code.

I don't have an idea to implement it, so cannot provide a design doc.

Can you just ask a member of the dev team for this Feature request to see their opinions? Maybe they will consider implementing it in the future.

huycozy commented 2 months ago

You can notice the difference by eye or use the performance tool to compare.

I ran the sample code on both cases (newWidget to true and false) can couldn't see the difference clearly. If you see it, please provide a demo illustrating the difference.

It work just like Offstage widget but no more layout, painting or other works. It's just there to keep the state alive.

You should describe more clearly what Offstage does not meet your needs. This is what Offstage offers currently:

https://github.com/flutter/flutter/blob/b0850beeb25f6d5b10426284f506557f66181b36/packages/flutter/lib/src/widgets/basic.dart#L3206-L3219

Keeping this issue to have framework team thoughts.

XuanTung95 commented 2 months ago

@huycozy

I ran the sample code on both cases (newWidget to true and false) can couldn't see the difference clearly. If you see it, please provide a demo illustrating the difference.

Maybe performance chart is better since you and I do not use the same device.

old_widget

new_wiget

If I x4 the number of widgets in ExpensiveList, the gap is 687ms vs 65ms.

This show that IndexedStack(), Visibility.maintain(), Offstage() do not help in my case (maybe there is other widget that I don't know?)

Delete the expensive widget does solve my performance problem but It cause lost state problem.

So if flutter provide a way to save and restore the state when widget is delete/recreated, it will solve my problem. Or other approach instead of deleting the widget maybe.

XuanTung95 commented 1 month ago

Update. I found that ListView + AutomaticKeepAliveClientMixin already implemented similar feature. When keepAlive widget in ListView is out of the viewport, it's state is saved and can be restore when it's visible again. Unfortunately, KeepAlive is only available inside a ListView, I want this feature for normal widget. For example when set keepAlive = false, the state of the sub-tree is saved and the sub-tree is removed, when keepAlive = true, the state is restored, same as the behavior of AutomaticKeepAliveClientMixin in ListView.