Thank you for making this prototype!

I think this design makes me feel much more comfortable than the decorator one.

I don't think the const is a big issue. In the end, the delegate will very likely be implemented by (mixin'd into) the state class of the themed widget. For example, I imagine that your example app will more likely be written this way in practice:

/// Flutter code sample for a [RawMenuAnchorAnimationDelegate] that animates a
/// [RawMenuAnchor] with a [SpringSimulation].
void main() {
  runApp(const RawMenuAnchorAnimationDelegateApp());
}

class RawMenuAnchorAnimationDelegateExample extends StatefulWidget {
  const RawMenuAnchorAnimationDelegateExample({super.key});

  @override
  State<RawMenuAnchorAnimationDelegateExample> createState() => _RawMenuAnchorAnimationDelegateExampleState();
}

class _RawMenuAnchorAnimationDelegateExampleState extends State<RawMenuAnchorAnimationDelegateExample>
    with SingleTickerProviderStateMixin, RawMenuAnchorAnimationDelegate {
  late final AnimationController animationController;
  final MenuController menuController = MenuController();

  @override
  void initState() {
    super.initState();
    // Use an unbounded animation controller so that simulations are not clamped.
    animationController = AnimationController.unbounded(vsync: this);
  }

  @override
  void dispose() {
    animationController.dispose();
    super.dispose();
  }

  SpringSimulation get forwardSpring => SpringSimulation(
    SpringDescription.withDampingRatio(mass: 1.0, stiffness: 150, ratio: 0.7),
    animationController.value,
    1.0,
    0.0,
  );
  SpringSimulation get reverseSpring => SpringSimulation(
    SpringDescription.withDampingRatio(mass: 1.0, stiffness: 200, ratio: 0.7),
    animationController.value,
    0.0,
    0.0,
  );

  @override
  void handleMenuOpenRequest({ui.Offset? position}) {
    // Call whenComplete() rather than whenCompleteOrCancel() to avoid marking
    // the menu as opened when the [AnimationStatus] moves from forward to
    // reverse.
    animationController.animateWith(forwardSpring).whenComplete(markMenuOpened);
  }

  @override
  void handleMenuCloseRequest() {
    // Call whenComplete() rather than whenCompleteOrCancel() to avoid marking
    // the menu as closed when the [AnimationStatus] moves from reverse to
    // forward.
    animationController.animateBackWith(reverseSpring).whenComplete(markMenuClosed);
  }

  @override
  Widget build(BuildContext context) {
    return RawMenuAnchor(
      controller: menuController,
      delegate: this,
      overlayBuilder: (BuildContext context, RawMenuOverlayInfo info) {
        // omitted, unchanged
      },
      builder: (BuildContext context, MenuController menuController, Widget? child) {
        // omitted, unchanged
      },
    );
  }
}

class RawMenuAnchorAnimationDelegateApp extends StatelessWidget {
  // omitted, unchanged
}

So in general I would say let's move forward with this direction. Let me see if I can find anyone else's opinion!

@chunhtai

If so, the current code seems to be a weird way to wire up the animation. We let the parent owns the animation controller, and use hook from MenuAnchor to trigger the animation and getting animation status from it.

Which hooks are you referencing?

RawMenuAnchor takes a animationController and a curve or simulation as input (either required parameter or creating a zero duration animation controller if not provided) and the RawMenuAnchor calls forward and or backward when open or close.

So, a few issues.

Considering RawMenuAnchor is only supposed to be used when implementing themed menus (and therefore should be as customizable as possible), a single animation controller is constraining. The examples I gave are trivial to make demos easy to understand, but the api is meant to be flexible.

As an example: what if you want the menu to animate closed with a different animation depending on whether the user scrolled or the user pressed escape? Since RawMenuAnchor would be responding to these events internally, there would be no way to decide which curve should be used.

Because simulations and animations are triggered differently, we'd have to call forward or animateWith, and reverse or animateBackWith, depending on whether a simulation or a curve is added.

We'd also need to either add an assert for only specifying a forward tween or a forward simulation and a reverse tween or reverse simulation, or add two constructors: one for simulations and one for tweens

Last, RawMenuAnchor doesn't animate anything and doesn't need to animate anything, so I'm hesitant to constrain component library developers. It is true that passing Duration.zero would work for immediate closure.

Also, does this mean passing in both MenuController and AnimationController? If we wanted to continue allowing users to pass a MenuController into MenuAnchor, we would need to pass that MenuController to RawMenuAnchor, since MenuAnchor has no way of observing when MenuController.open is called.

Alternatively

We could do something like mwc or the chromium menu system does by allowing the users to pass in an onOpenRequested and onCloseRequested (or something with a better name) to indicate the menu should start opening or closing. Developers could just call MenuController.open/MenuController.close whenever the animations finish.

https://github.com/material-components/material-web/blob/ed14c53568c129fbfcef3939dcc4ef02c330d905/menu/internal/controllers/surfacePositionController.ts#L117-L129

https://github.com/chromium/chromium/blob/9679e6b6fbe9d873115cd5b5e7d9663e0e487b36/ui/views/controls/menu/menu_delegate.h#L207-L211

Nevermind - I remembered this doesn't work because descendents calling MenuController.close immediately close the menu.

Which hooks are you referencing?

handleMenuOpenRequest/handleMenuCloseRequest to trigger the animation
markMenuClosed/markMenuOpened to inform the animation status.

IMO, this API it is a bit confusing unless developer know how they are used internally in RawMenuAnchor.

Also, does this mean passing in both MenuController and AnimationController? If we wanted to continue allowing users to pass a MenuController into MenuAnchor, we would need to pass that MenuController to RawMenuAnchor, since MenuAnchor has no way of observing when MenuController.open is called.

In some of the past example, we let the base class owns animation controller, and expose the animation<double> in the builder so that the toplevel widget can build Transition in the builder method. We assume all the consumer of the base widget will want the animation. Is this not the case for MenuAnchor?

what if you want the menu to animate closed with a different animation depending on whether the user scrolled or the user pressed escape

Can the current approach tell about how animation is triggered?

I am slightly concern we may be over-designing this, do we have use case that we want different curve/simulation? Either way, even if we pass in animation controller or let RawMenuAnchor create one as mentioned above, we can add this flexibility by adding a callback

For example,

enum Cause {
  escape,
  tap,
  scroll,
  ...
}

RawMenuAnchro(
   onGenerateSimulation: (Cause cause, bool isClose) => createSimulation(cause, isClose),
)

So, I'm a bit paranoid of writing an API that imposes any constraints. That's why the RawMenuAnchorAnimationDelegate is an abstract mixin class. Normal developers shouldn't be using this api -- it's meant for developers who are writing themed menus for component libraries.

The only reason why RawMenuAnchor exists at all is so that menus use a common, inherited MenuController, so that's why I'm a bit hesitant to manage animation from within RawMenuAnchor.

In some of the past example, we let the base class owns animation controller, and expose the animation in the builder so that the toplevel widget can build Transition in the builder method. We assume all the consumer of the base widget will want the animation. Is this not the case for MenuAnchor?

I was thinking that the MenuAnchor widget would pass an Animation to its panel builder, but RawMenuAnchor would contain no animation logic. I wrote a dartpad to demonstrate how you could create a builder that passes animations.

In the demo, MenuAnchor behaves how I would expect the current Material MenuAnchor implementation to behave -- its panel is built with a panelBuilder that passes a Animation. RawMenuAnchor doesn't have access to the animation.

Dartpad:

https://dartpad.dev/cc66b6679330c90fc3cf18f7c9ca3dc0

// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

/// @docImport 'package:flutter/material.dart';
library;

import 'dart:ui' as ui;

import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter/scheduler.dart';
import 'package:flutter/services.dart';
import 'package:flutter/physics.dart';

/// Flutter code sample for a [RawMenuAnchorAnimationDelegate] that animates a
/// [RawMenuAnchor] with a [SpringSimulation].
void main() {
  runApp(const App());
}

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

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      theme: ThemeData.from(
        useMaterial3: true,
        colorScheme: ColorScheme.fromSeed(
          seedColor: Colors.blue,
          dynamicSchemeVariant: DynamicSchemeVariant.vibrant,
        ),
      ),
      home: const Scaffold(body: Center(child: Example())),
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    return MenuAnchor(
      panelBuilder: (BuildContext context, animation) {
        final MenuController rootMenuController =
            MenuController.maybeOf(context)!;
        return FadeTransition(
          opacity: animation,
          child: SizeTransition(
            sizeFactor: animation,
            child: Align(
              alignment: Alignment.topRight,
              child: Column(
                children: <Widget>[
                  for (int i = 0; i < 4; i++)
                    MenuAnchor(
                      panelBuilder: (BuildContext context, animation) {
                        final String animationStatus =
                            MenuController.maybeAnimationStatusOf(
                              context,
                            )!.name;
                        return FadeTransition(
                          opacity: animation,
                          child: SizeTransition(
                            sizeFactor: animation,
                            child: SizedBox(
                              height: 120,
                              width: 120,
                              child: Center(
                                child: Text(
                                  'Panel $i:\n$animationStatus',
                                  textAlign: TextAlign.center,
                                ),
                              ),
                            ),
                          ),
                        );
                      },
                      builder: (
                        BuildContext context,
                        MenuController controller,
                      ) {
                        return MenuItemButton(
                          onFocusChange: (bool focused) {
                            if (focused) {
                              rootMenuController.closeChildren();
                              controller.open();
                            }
                          },
                          onPressed: () {
                            if (!controller
                                .animationStatus
                                .isForwardOrCompleted) {
                              rootMenuController.closeChildren();
                              controller.open();
                            } else {
                              controller.close();
                            }
                          },
                          trailingIcon: const Text('▶'),
                          child: Text('Submenu $i'),
                        );
                      },
                    ),
                ],
              ),
            ),
          ),
        );
      },
      builder: (BuildContext context, MenuController controller) {
        return FilledButton(
          onPressed: () {
            if (controller.animationStatus.isForwardOrCompleted) {
              controller.close();
            } else {
              controller.open();
            }
          },
          child: const Text('Menu'),
        );
      },
    );
  }
}

class MenuAnchor extends StatefulWidget {
  const MenuAnchor({
    super.key,
    required this.panelBuilder,
    required this.builder,
  });
  final Widget Function(BuildContext, Animation<double>) panelBuilder;
  final Widget Function(BuildContext, MenuController) builder;

  @override
  State<MenuAnchor> createState() => MenuAnchorState();
}

class MenuAnchorState extends State<MenuAnchor>
    with SingleTickerProviderStateMixin, RawMenuAnchorAnimationDelegate {
  final MenuController menuController = MenuController();
  late final AnimationController animationController;
  late final CurvedAnimation animation;
  bool get isSubmenu => MenuController.maybeOf(context) != null;

  @override
  void initState() {
    super.initState();
    animationController = AnimationController(
      vsync: this,
      duration: const Duration(milliseconds: 200),
    );
    animation = CurvedAnimation(
      parent: animationController,
      curve: Curves.easeOutQuart,
    );
  }

  @override
  void dispose() {
    animationController.dispose();
    animation.dispose();
    super.dispose();
  }

  @override
  void handleMenuOpenRequest({ui.Offset? position}) {
    // Call whenComplete() rather than whenCompleteOrCancel() to avoid marking
    // the menu as opened when the AnimationStatus moves from forward to
    // reverse.
    animationController.forward().whenComplete(markMenuOpened);
  }

  @override
  void handleMenuCloseRequest() {
    // Animate the children of this menu closed.
    menuController.closeChildren();
    animationController.reverse().whenComplete(markMenuClosed);
  }

  @override
  Widget build(BuildContext context) {
    return RawMenuAnchor(
      controller: menuController,
      delegate: this,
      overlayBuilder: (BuildContext context, RawMenuOverlayInfo info) {
        final ui.Offset position =
            isSubmenu ? info.anchorRect.topRight : info.anchorRect.bottomLeft;
        final ColorScheme colorScheme = ColorScheme.of(context);
        return Positioned(
          top: position.dy,
          left: position.dx,
          child: Semantics(
            explicitChildNodes: true,
            scopesRoute: true,
            child: ExcludeFocus(
              // Remove focus while the menu is closing.
              excluding: animation.status == AnimationStatus.reverse,
              child: TapRegion(
                groupId: info.tapRegionGroupId,
                onTapOutside: (PointerDownEvent event) {
                  menuController.close();
                },
                child: FadeTransition(
                  opacity: animation,
                  child: Material(
                    elevation: 8,
                    clipBehavior: Clip.antiAlias,
                    borderRadius: BorderRadius.circular(8),
                    shadowColor: colorScheme.shadow,
                    child: widget.panelBuilder(context, animation),
                  ),
                ),
              ),
            ),
          ),
        );
      },
      builder: (
        BuildContext context,
        MenuController controller,
        Widget? child,
      ) {
        return widget.builder(context, controller);
      },
    );
  }
}

IMO, for menus that have an animation with Duration.zero, it'd be unusual to pass an Animation to their panels.

In some of the past example, we let the base class owns animation controller, and expose the animation in the builder so that the toplevel widget can build Transition in the builder method. We assume all the consumer of the base widget will want the animation. Is this not the case for MenuAnchor?

OH my mistake. I misunderstood. So, you're suggesting RawMenuAnchor would have an internal AnimationController that would respond to open/close calls, and would pass animationController.view (or a tweened animation) to the overlayBuilder.

That makes more sense to me since RawMenuAnchor would control the AnimationController lifecycle.

To your point, there are widgets that pass animations to their children (e.g. PageRouteBuilder). But, the purpose is to let the component developer decide whether to pass an Animation, a progress value (e.g. 0.5), or nothing at all, rather than RawMenuAnchor.

An issue that may come up with only passing an Animation from RawMenuAnchor is that a parent, such as MenuAnchor, has no elegant way of listening to the progress of the animation. If, for example, you want a submenu anchor to animate in response to its submenu opening, or you want to open a menu half way, or if you wanted to focus the menu anchor when the menu finishes opening, there'd be no easy way to do so.

For example, when closing a CupertinoMenuAnchor, tapping an underlying submenu will immediately close the submenu which is currently closing. I'm not sure it there would be a simple way to trigger this behavior without access to the AnimationController.

Can the current approach tell about how animation is triggered?

I am slightly concern we may be over-designing this, do we have use case that we want different curve/simulation? Either way, even if we pass in animation controller or let RawMenuAnchor create one as mentioned above, we can add this flexibility by adding a callback

The delegate approach doesn't tell you how the menu close was requested, but it does tell you when the close is requested, and you can listen for whether "Escape" is pressed or whether a scroll controller passed to the menu panel is registering scrolls to determine if an escape or a scroll occurs. I agree that adding any additional info about how the menu was closed is overkill.

This is an example of an edge case which doesn't have a good solution. The point of the api is to make it so that no menu is impossible to implement without internal access to RawMenuAnchor, because forking RawMenuAnchor eliminates the benefits of a common menu interface.

When I first implemented CupertinoMenuAnchor, I also had multiple issues adapting MenuAnchor to be compatible with CupertinoMenuAnchor due to various quirks with iOS, and I imagine there are behaviors I can't anticipate that will arise with other developers.

The example in the video is an example that I wouldn't be able to replicate without access to the AnimationController.

Another problem that I encountered in the past was that the material menu anchor spec runs multiple animations with different durations. This can be done with a single Animation run through several tweens, but it's much easier to coordinate to trigger all animations from an onOpenRequest/onCloseRequest callback. FYI the spec is internal so I'm not sure what the GooglePlex link says -- I'm basing this off of the implementation.

So, I'm a bit paranoid of writing an API that imposes any constraints, considering RawMenuAnchor is supposed to be for component library developers rather than for end users. That's also why the RawMenuAnchorAnimationDelegate is an abstract mixin class rather than a class that can be instantiated. Normal developers shouldn't be using this api -- it's meant for developers who are writing themed menus for component libraries.

That said, the main inspiration for this api is that it used to be impossible to run a simulation in reverse, but since Tong added animateBackWith, that is one less constraint.

handleMenuOpenRequest/handleMenuCloseRequest to trigger the animation
markMenuClosed/markMenuOpened to inform the animation status.

IMO, this API it is a bit confusing unless developer know how they are used internally in RawMenuAnchor.

You're definitely right about the current implementation being confusing.

We could return a future from handleMenuOpenRequest/handleMenuCloseRequest instead of calling markMenuOpened/Closed. I'm not a huge fan of futures in situations like this because we're dealing with a stream of events rather than two events (i.e. the AnimationStatus could move from dismissed <-> forward, forward <-> reverse, reverse<->dismissed, etc, rather than always moving from dismissed -> forward -> completed -> reverse -> dismissed).

We could also rename the callbacks to something simpler e.g. willMenuOpen/menuOpenRequested/didRequestMenuOpening.

I was also wondering whether we should use "MenuStatus" rather than AnimationStatus so that developers wouldn't conflate animations with menu behavior.

Anyways, sorry for the long response. Let me know your thoughts.

Thanks for the detailed reply. I am actually neutral on whether we should bake animation controller in the RawMenuAnchor or not. so I am ok if we pull animation from RawMenuAnchor completely.

With that in mind, I think the current pr is still in a weird place where it is still in the middle that RawMenuAnchor still some how aware of that an animation may be going on outside itsself, i.e. the markMenuOpen/Close and the _RawMenuAnchorBaseMixin.animationstatus. Can we do it this way?

1. change MenuController to be ChangeNotifier, and will notify when its isOpen is changed.
2. _RawMenuAnchorBaseMixin listens to MenuController and rebuild when isOpen is changed
3. All the gesture/scrolling that causes open/close will call MenuController.open and MenuController.close

The default implementation of MenuController will just change its isOpen state and notify listener.
If you want animation, you override the MenuController.open and MenuController.close that will trigger animationController and only change the isOpen state + notify at the end.

I think this will make it cleaner in terms of APIs and internal implementation. WDYT?

@chunhtai

If I understand the proposal correctly (please correct me if I'm wrong), there are a few problems that may occur with a ChangeNotifier/subclass design

1. If we allowed for subclasses of MenuController, animating the menu would be simple. A ChangeNotifier isn't actually needed. However, I believe a reviewer told me to avoid having multiple subclasses of MenuController. This was in response to me implementing a CupertinoMenuController in order to accomplish a similar design. Having a MenuController/MaterialMenuController/CupertinoMenuController does seem like a large api surface for similar behavior.
2. If we store isOpen state on a MenuController: Passing a MenuController with isOpen == true to a MenuAnchor whose overlay is not shown, or moving a MenuController with isOpen == false to a MenuAnchor whose overlay is shown, would require a post-frame callback, because didUpdateWidget is called during SchedulerPhase.persistentCallbacks. During SchedulerPhase.persistentCallbacks, calling OverlayController.show()/hide() is not allowed. However, using a post-frame callback means MenuController.isOpen will be true for a full frame before the menu is shown.
   • The fact that we are working with overlays, rather than children, is the main reason why we can't use a design like ExpansionTileController.
3. Because RawMenuAnchor closes in response to scrolls and resizes, which triggers didChangeDependencies, MenuController listeners would be notified during persistent callbacks, which may schedule additional builds. I'm not 100% confident this is an issue, because we could just add a post frame callback, but try resizing this example and check the console: https://dartpad.dev/c607d09391baa1ea303ffeeb3cb41f14?channel=main
4. Likewise, MenuController.isOpen would no longer correspond to whether the overlay is open, since MenuController.isOpen would no longer point to OverlayController.isShowing. Right now, MenuController.open() doesn't immediately set isOpen to true. I think this could cause errors for current users of MenuAnchor and RawMenuAnchor who rely on when MenuController.isOpen is true.
5. With a ChangeNotifier, all current and future uses of MenuController would have to be disposed. I'm not sure if there's a good way to automate that behavior.

So, here's an alternative proposal that removes the AnimationStatus parts and some of the complexity of the delegate. MenuControllerDecorator is used instead of the delegate.

The implementation will be a simplified version of the original decorator proposal here. If we don't need to notify children of the animation status, we could remove AnimationStatus from _RawMenuAnchorBaseMixin and the markMenuOpened method. The only method that we need for animation is a way to hide the menu overlay once the menu closing animation is finished. Right now, markMenuClosed hides the overlay, but we could rename it to hideMenu or something simpler. Users of themed menus like MenuAnchor wouldn't have to change anything and could continue using MenuController, while developers using RawMenuAnchor would have the option to add animations or make no changes. Usage would look like:

class _MenuAnimator extends MenuControllerDecorator {
  const _MenuAnimator({required super.menuController, required this.animationController});
  final AnimationController animationController;

  // Called when MenuController.open() is called
  @override
  void handleMenuOpenRequest({ui.Offset? position}) {
    animationController.forward()
  }

  // Called when MenuController.close() is called
  @override
  void handleMenuCloseRequest() {
    animationController.reverse().whenComplete(hideMenu)
  }
}

// An animated menu, like MenuAnchor or CupertinoMenuAnchor, meant to be consumed by end users
class ThemedMenu extends StatefulWidget {
  const Menu({super.key, required this.menuController});

  final MenuController menuController;

  @override
  State<Menu> createState() => _MenuState();
}

class _ThemedMenuState extends State<Menu>
    with SingleTickerProviderStateMixin {
  late final AnimationController animationController;
  late final _MenuAnimator menuAnimator;

  @override
  void initState() {
    super.initState();
    animationController = AnimationController.unbounded(vsync: this);
    menuAnimator = _MenuAnimator(
      menuController: widget.menuController,
      animationController: animationController,
    );
  }

  @override
  void didUpdateWidget(oldWidget) {
    if (oldWidget.menuController != widget.menuController) {
      menuAnimator = _MenuAnimator(
        menuController: widget.menuController,
        animationController: animationController
      );
    }
  }

  @override
  void dispose() {
    animationController.dispose();
    super.dispose();
  }

 @override
  Widget build(BuildContext context) {
    return RawMenuAnchor(
      controller: menuAnimator,  // Pass in the decorated controller
      overlayBuilder: (context, info) {
          // Use your animation controller to animate the menu
      },
      child: widget.child
    );
  }

This is almost the same as subclassing MenuController, except we are calling hideMenu instead of super.close(), and we can use the same MenuController interface across all menus (the decorator is only used by the implementer of the themed menu, but the user just uses MenuController).

Let me know if that makes sense.

I believe a reviewer told me to avoid having multiple subclasses of MenuController

Yes, i dislike subclassing the controllers. but since we are moving the animation out of RawMenuAnchor, some object has to own the trigger the animationcontroller and maintain it. it is either the MenuAnchorState or MenuController. the current pr uses MenuAnchorState which causes multiple point of calling open/close and multiple source of truth. I am also a bit concern that RawMenuAnchor are controlled by both a delegate and a controller. If I have to choose, I leaning toward using MenuController subclass. We also have examples that do this. eg. PageController and ScrollController

MenuController/MaterialMenuController/CupertinoMenuController

I wonder why we need MaterialMenuController and CupertinoMenuController instead just one subclass? Does the MenuController with animation needs to know the design language?

If we store isOpen state on a MenuController, Passing a MenuController with isOpen == true to a MenuAnchor whose overlay is not shown

We already does this, except now it is a getter. but people can subclass it and do anything they want. This is not a strong argument though, we could declare we don't support this use case or like use said add a postframecallback

Because RawMenuAnchor closes in response to scrolls and resizes, which triggers didChangeDependencies, MenuController listeners would be notified during persistent callbacks

yes this is unfortunate. The current pr is fine because the controller can't be listen even though it has a isOpen getter, which is not ideal as well.

During SchedulerPhase.persistentCallbacks, calling OverlayController.show()/hide() is not allowed. However, using a post-frame callback means MenuController.isOpen will be true for a full frame before the menu is shown.

I think we may have the same issue in this pr if the requestOpen calls markMenuOpen synchronously right?

Likewise, MenuController.isOpen would no longer correspond to whether the overlay is open, since MenuController.isOpen would no longer point to OverlayController.isShowing

yes this will be a problem. Do you know what use case is for animationstatus in RawMenuAnchor? looking at the code looks like it was used to shortcircuit the open and close request and as a inherited widget. I think we can infer the short-circuit logic if we let the open()/close() to update the isOpen property synchronously before trigger the animation if we want to, this will also solve the isOpen not syncing with overlayController.isShowing.

If so, the menuController don't need to know the animation at all if the RawMenAnchor don't need to know when the animation finishes

With a ChangeNotifier, all current and future uses of MenuController would have to be disposed. I'm not sure if there's a good way to automate that behavior.

I think this is fine, as long as we agree on what the right way to approach this. I won't see this as a blocker.

@chunhtai

This is a long response so I highlighted any parts that need particular attention. I'm sorry if any of my points are redundant.

I played with the decorator a bit more and it's possible to simplify it down to:

final class _MenuAnimator extends MenuControllerDecorator {
  const _MenuAnimator({required super.menuController, required this.animationController});
  final AnimationController animationController;

  @override
  void handleMenuOpenRequest({ui.Offset? position}) {
    animationController.forward();
  }

  @override
  void handleMenuCloseRequest() {
    animationController.reverse().whenComplete(close);
  }
}

I believe only trade-off compared to using markMenuClosed is that calling close() while the menu is in the process of closing will immediately close the menu. I think this has to be the case to prevent reentrance.

I agree the delegate is awkward, but is there a reason why the decorator can't be used? It's basically like subclassing, but end users could use the same MenuController for regular and animated menus. It doesn't require disposal or multiple public MenuControllers, provides a single controller to RawMenuAnchor, is stateless, and lets a ThemedMenuAnchorState (e.g. CupertinoMenuAnchorState) manage the AnimationController. It would also remove all AnimationStatus logic.

Yes, i dislike subclassing the controllers. but since we are moving the animation out of RawMenuAnchor, some object has to own the trigger the animationcontroller and maintain it. it is either the MenuAnchorState or MenuController.

Wouldn't it make more sense if the themed menu (e.g. MenuAnchorState or CupertinoMenuAnchorState) managed the AnimationController, since vsync is typically obtained from a state mixin?

the current pr uses MenuAnchorState which causes multiple point of calling open/close and multiple source of truth. I am also a bit concern that RawMenuAnchor are controlled by both a delegate and a controller. If I have to choose, I leaning toward using MenuController subclass.

I agree that having both a delegate and a controller is weird. The decorator would solve this, however.

We also have examples that do this. eg. PageController and ScrollController

You're right, but ScrollController and PageController (which is a subclass of ScrollController) model a different behavior.

A Scrollable needs to know the exact pixel offset when drawing the viewport. It has a direct need for the animation value of the ScrollableController.

RawMenuAnchor needs to know whether the overlay is showing or is not showing. It isn't running animations, so it has no use for the AnimationController.

ScrollController is also (necessarily) very complex and reimplements much of the AnimationController api, which I think we should avoid with a MenuController subclass.

Some counter examples that behave like the current MenuController include:

ContextMenuController
OverlayPortalController
TreeSliverController -- also allows animation
ExpansionTileController (before ExpansibleController, this behaved exactly like MenuController) -- also allows animation

The only analogous controller that handles animation is MagnifierController, but that's marked for removal because of the existence of OverlayPortal.

I wonder why we need MaterialMenuController and CupertinoMenuController instead just one subclass? Does the MenuController with animation needs to know the design language?

I completely agree. I wasn't sure whether you were suggesting we introduce a new subclass to widgets.dart (e.g. AnimatedMenuController), or we instruct themed menu developers to override MenuController and close this PR.

I think we may have the same issue in this pr if the requestOpen calls markMenuOpen synchronously right?

You are right, and this is a bug. There used to be lifecycle logic to prevent errors from occurring, but this logic led to other test failures.

But, if we remove MenuController.animationStatusOf(context) from the PR (the updated decorator proposal), we could remove markMenuOpen because we would no longer need to notify when the animation status changed.

If we store isOpen state on a MenuController, passing a MenuController with isOpen == true to a MenuAnchor whose overlay is not shown

We already does this, except now it is a getter. but people can subclass it and do anything they want. This is not a strong argument though, we could declare we don't support this use case or like use said add a postframecallback

To your point, people can override MenuController, but MenuController was written before the final keyword was added to Dart. I was going to propose we add the final keyword to MenuController. Alternative, we could make isOpen nonvirtual.

Also, even if we use a post-frame callback, the isOpen value would still be incorrect for a frame + plus the synchronous time before OverlayController.show()/hide() is called. This could cause strange bugs for any project currently using MenuController

yes this will be a problem. Do you know what use case is for animationstatus in RawMenuAnchor? looking at the code looks like it was used to shortcircuit the open and close request and as a inherited widget. I think we can infer the short-circuit logic if we let the open()/close() to update the isOpen property synchronously before trigger the animation if we want to, this will also solve the isOpen not syncing with overlayController.isShowing. If so, the menuController don't need to know the animation at all if the RawMenAnchor don't need to know when the animation finishes

Two things --

First, the animationStatus on _RawMenuAnchorBaseMixin and MenuController is only used by descendents who want to rebuild when the menu's animation changes. We can completely remove the animation status and markMenuOpened if MenuController.maybeAnimationStatusOf(context) is removed.

I was wondering whether we should use MenuStatus.open/opening/closed/closing instead of AnimationStatus because RawMenuAnchor isn't actually aware of the animation.

That said, one downside of the delegate and the decorator is a potential for reentrance if the user calls close() from handleMenuCloseRequest().

Second, for clarification, AnimationStatus.reverse, AnimationStatus.forward, and AnimationStatus.complete have to occur when isOpen == true. isOpen is true for the entirety of the closing animation. isOpen is only false during AnimationStatus.dismissed.

Here's the full lifecycle of the delegate behavior:

// MenuController.maybeAnimationStatusOf(context) == AnimationStatus.dismissed

MenuController.open() -> _RawMenuAnchorBaseMixin.requestOpen() -> delegate.handleMenuOpenRequest()

// MenuController.maybeAnimationStatusOf(context) == AnimationStatus.forward

markMenuOpened() after animation finishes

// MenuController.maybeAnimationStatusOf(context) == AnimationStatus.complete

MenuController.open() -> _RawMenuAnchorBaseMixin.requestClose() -> delegate.handleMenuCloseRequest()

// MenuController.maybeAnimationStatusOf(context) == AnimationStatus.reverse

markMenuClosed() after animation finishes

// MenuController.maybeAnimationStatusOf(context) == AnimationStatus.dismissed

Sorry I hadn't been participating lately, partly because I was engaged in other issues and also partly because I knew that letting chunhtai handle it would bring in new ideas. Unfortunately I'm also going OOO from tomorrow to the end of the week but I really wanted to check how it goes and contribute to the discussion before I leave. I tried my best to read the discussion so far but I don't think I've fully grasped every details, so forgive me if I missed something that have been discussed (especially since I'm much less experienced on widgets with animations than either of you).

First a small clarification:

Because simulations and animations are triggered differently, we'd have to call forward or animateWith, and reverse or animateBackWith, depending on whether a simulation or a curve is added.

This shouldn't be a big concern. I wrote about the relationship between curve-based animation and simulation-based ones in this comment; in short, theoretically we can (and should) use simulations for everything, because curves can be converted to simulations: Curves are just a special case of simulations that have pre-determined curve and duration.

Now onto the hard part, the API design for animations. I think I now understand better how the decorator design helps. When we don't add animations, the themed menu could simply pass the menu controller received from the app straight to the RawMenuAnchor, which attaches with the controller and interacts with it. However, in order to play animations, the themed menu must be notified when the user opens or closes on the menu anchor, and even delaying RawMenuAnchor's closing status. This requires the themed menu to "intercept" MenuController's calls. Indeed, the decorator design allows maximum flexibility of control, and the delegate design feels weird by requiring the raw menu anchor to bubble the messages back up.

RawMenuAnchor needs to know whether the overlay is showing or is not showing. It isn't running animations, so it has no use for the AnimationController.

I completely agree. Even though during the opening and closing animations the raw menu anchor will be told to display contents that are scaled and faded, it doesn't care why they're like this. The raw menu anchor will simply consider itself opened as long as any pixels are displaying.

But I think we can make it one step further. It seems to me that the ultimate motivation for the decorator design is so that the RawMenuAnchor consider itself closed much later than the MenuController.close() call. So what if the RawMenuAnchor doesn't even share the same opening/closing status as the themed menu anchor? What if they don't even use the same controller?

More specifically, my proposal is:

• No delegates and no decorators.
• The RawMenuAnchor is not aware of animations.
• While the themed menu anchor receives an external MenuController, it doesn't pass it to its raw menu anchor child. Instead it creates and manages an internal menu controller for the raw menu anchor.
• The internal menu controller becomes opened as soon as the external menu controller is opened.
• The internal menu controller becomes closed only when the closing animation is completed.
• The animation controller may be managed by the themed menu anchor, or may be passed in by the user. I don't think it matters much. Either way, it's not passed to the raw menu anchor.

Do you think this will work?

@dkwingsmt

Oh, I forgot that there was one more issue I ran into using the dual menu controller method. When RawMenuAnchor is closed internally by pressing escape or resizing the screen, there was no way to animate the menu closed. This is because the themed menu isn't able to intercept the call to close.

I wasn't able to solve this problem with the original implementation -- I just didn't animate closure caused by these two methods. I'm revisiting the solution now and I'm not sure if there is a good way around those two problems, but I do still really like the simplicity of the solution you proposed. I'll try to think if there is a way around these problems.

(Sorry it took me a few days to get my errands cleared and my thoughts straight.)

Well that makes it more and more complicated. Let me think about it one by one.

When RawMenuAnchor is closed internally by pressing escape or resizing the screen, there was no way to animate the menu closed.

I think this one is the easiest to handle. Action.overridable is designed specifically for this use case. Pressing Esc should yield a DismissIntent, which is registered in an Action.overridable. The themed menu should wrap with another Action that catches the DismissIntent and does whatever it normally does upon externalMenuController.close().

how does the external menu controller notify the themed menu when changes occur? In other words, when close is called on the external controller, how will the themed menu know that the menu should begin animating closed?

I'm not 100% sure where the obstacle is here. My understanding is that the MenuController can not be attached to the themed menu because it is specially designed for _RawMenuAnchorBaseMixin, which can't be used by themed menus. Do you think the following design works:

• Make the menu controller records the BuildContext instead
• Upon .open() and .close(), instead of calling methods, dispatch intents such as DismissIntent onto the BuildContext.

Whoops, I didn't get notified of your message for some reason.

I think this one is the easiest to handle. Action.overridable is designed specifically for this use case. Pressing Esc should yield a DismissIntent, which is registered in an Action.overridable. The themed menu should wrap with another Action that catches the DismissIntent and does whatever it normally does upon externalMenuController.close().

The problem is that externalMenuController.close() would do nothing since it's not being attached to RawMenuAnchor. Calling internalMenuController.close() again would just emit another dismiss intent, so I'm not sure how we would actually close the menu.

I'm not 100% sure where the obstacle is here. My understanding is that the MenuController can not be attached to the themed menu because it is specially designed for _RawMenuAnchorBaseMixin, which can't be used by themed menus. Do you think the following design works:

• Make the menu controller records the BuildContext instead
• Upon .open() and .close(), instead of calling methods, dispatch intents such as DismissIntent onto the BuildContext.

Clarify what you mean by record...

Also, how would the menu actually be closed from the DismissAction? Calling internalMenuController.close() or externalMenuController.close() would just emit another DismissIntent(), but we need a way to actually close the menu.

I've drafted a prototype, #167537, that refactors RawMenuAnchor based on Actions to allow the design I described above:

• Dual animation controller for the themed menu and the raw menu
• No delegates or decorators

Essentially it turns out a bit similar to decorator design. Both designs have something to actuate the changes (requestOpen vs innerController.open) as well as some interface that allows overriding (controller's public methods vs the external controller). The two approaches use different ways to allow MenuController send message to its host menu anchor.

I'm not heavily leaning towards this design though. It wins at leveraging Flutter's existing mechanism instead of introducing a new paradigm that has never been seen in Flutter ("decorator"). But it is equal, if not worse, in terms of verbosity, due to the duplicate code in themed menus to define Actions and attach the menu controller to the context inside this Actions.

I'll post the same comment on this PR and the delegate PR so that viewers of the other PR have some context.

I spent some time playing with the actions PR last night. It's fundamentally similar to the decorator, but I liked your idea of themed menus overriding the actions of their RawMenuAnchor, because it feels like it fits with Flutter paradigms. I ended up with something completely different, but in the spirit of your PR. Here's my thought process:

What I like about the actions proposal:

• It's more understandable than the decorator.
• It avoids the animationStatus errors that I introduced
• I like the concept of placing an intent at a different layer of the tree. I'm used to moving actions around, so this was a cool solution.
• Intent/Actions made the code a lot more explicit

A few issues that I came across:

• Passing BuildContext is usually discouraged because a BuildContext can be changed if a widget is reparented, and there's a risk of memory leaks. Also, exposing the attachment/detachment process increases the risk of memory leaks
• Like you mentioned, it's a lot of code to reimplement for each animated menu, and it's somewhat easy to mess up (e.g. what if you forget an action handler)
• Having MenuController.dismiss(), MenuController.close(), and MenuController.detach() would probably be confusing to someone who is first using MenuController.
• Existing menus (e.g. PopupMenuButton, context menus, etc) should probably be wired up to handle the new intents, otherwise users may be confused.
• Intent bubbling
  • For example, given a root menu and a submenu, if the internal submenu emits a DismissMenuIntent that is not overridden by the themed submenu, but is overridden by the root menu, wouldn't the root menu animate closed instead of the submenu synchronously closing? I'm not sure, but it's a bit confusing for me to think about.
• Less important because I think we were going to remove the animationStatus, but making the animation status mutable by everyone could lead to bugs.

So, I tried a couple different changes to simplify things. For example, we could pass a "inner" MenuController inside of the OpenMenuIntent/CloseMenuIntent that could be used by actions to perform a different opening or closing action. This would remove the need for attachment/detachment and BuildContext references.

However, after thinking about it more, I realized our solutions are basically trying to accomplish the following:

1. We want MenuController.open() and MenuController.close() to animate by default
2. We want to provide a way to show or hide the overlay before and after animations using the MenuController
3. Allowing all users (not just menu developers) to skip menu animations could be useful (e.g. if you wanted to hide the menu immediately when transitioning to a new page)

I'm curious of your thoughts on this but what if skipped actions and decorators, and just added a flag to MenuController.open() and MenuController.close() that chose whether to trigger animation callbacks on RawMenuAnchor. For example, if transition was the flag and it was true by default, MenuController.open() would trigger onOpenRequested and MenuController.open(transition: false) would hide the overlay. The PR is here: #167806.

final MenuController menuController = MenuController;
late final AnimationController animationController = AnimationController(vsync: this, duration: Duration(milliseconds: 200));

void _handleCloseRequest() {
    animationController.reverse().whenComplete(() {
      menuController.close(transition: false); // Hides the overlay without calling onCloseRequest
    });
  }

  void _handleOpenRequest() {
    menuController.open(transition: false); // Shows the overlay without calling onOpenRequested
    animationController.forward();
  }

  // Example function called by the anchor
  void _handleAnchorTapped() {
     if (animationController.isForwardOrCompleted) {
        menuController.close();
      } else {
        menuController.open();
      }
   }

  @override
  Widget build(BuildContext context) {
    return RawMenuAnchor(
      controller: menuController,
      onCloseRequested: _handleCloseRequest,  // Called when MenuController.close() is called
      onOpenRequested: _handleOpenRequest.   // Called when MenuController.open() is called
     ...
  )
)

I initially avoided this approach because I thought that would break the function signature, but it appears optional parameters can be ignored by the type checker (e.g. MenuController.close can be used as a VoidCallback). Let me know what you think!