MacosApp builder never gets used 🐛

[GroovinChip]

Description

Thanks to @imtoori for discovering this bug.

Compare https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/material/app.dart#L804 to https://github.com/GroovinChip/macos_ui/blob/dev/lib/src/macos_app.dart#L335 and you can see that we do not user the builder.

[GroovinChip]

I've written the following fix:

return MacosTheme(
  data: theme,
  child: DefaultTextStyle(
    style: TextStyle(color: theme.typography.body.color),
    child: widget.builder != null
        // See the MaterialApp source code for the explanation for
        // wrapping a builder in a builder
        ? Builder(
            builder: (context) {
              return widget.builder!(context, child);
            },
          )
        : child ?? const SizedBox.shrink(),
  ),
);

When I change our example app to return the Demo() in the builder rather than as the home, like so:

          builder: (context, child) {
            return Demo();
          },
          //home: Demo(),

I get the following error:

======== Exception caught by widgets library =======================================================
The following assertion was thrown building MacosTooltip(dirty, dependencies: [_InheritedMacosTheme], state: _MacosTooltipState#58bfd(ticker inactive)):
No Overlay widget found.

MacosTooltip widgets require an Overlay widget ancestor for correct operation.

The most common way to add an Overlay to an application is to include a MaterialApp or Navigator widget in the runApp() call.

The specific widget that failed to find an overlay was: MacosTooltip
The relevant error-causing widget was: 
  MacosTooltip file:///Users/groov/development/flutter_projects/packages_and_plugins/macos_ui/example/lib/pages/colors_page.dart:20:13
When the exception was thrown, this was the stack: 
#0      Overlay.of.<anonymous closure> (package:flutter/src/widgets/overlay.dart:306:9)
#1      Overlay.of (package:flutter/src/widgets/overlay.dart:309:6)
#2      _MacosTooltipState.build (package:macos_ui/src/labels/tooltip.dart:315:20)
#3      StatefulElement.build (package:flutter/src/widgets/framework.dart:4691:27)
#4      ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:4574:15)
...
====================================================================================================

@bdlukaa you wrote the tooltip widget, do you have any insight on why using the builder would mean there is no Overlay?

[bdlukaa]

you wrote the tooltip widget, do you have any insight on why using the builder would mean there is no Overlay?

So, the builder method is to build something above Navigator and below MediaQuery in the tree, such as ScrollConfiguration, so that it applies to all the routes. The Overlay widget is added to the tree by the Navigator widget, but since it's not available in the Demo's context, the error is thrown.

[GroovinChip]

So any widget built in the builder needs to have its own Navigator and Overlay?

EDIT: Here's the solution:

            ? Builder(
                builder: (context) {
                  return Overlay(
                    initialEntries: [
                      OverlayEntry(
                        builder: (context) => widget.builder!(context, child),
                      )
                    ],
                  );
                },
              )
            : child ?? const SizedBox.shrink(),

[bdlukaa]

I don't think it's a good solution. The builder callback shouldn't be to add the home page, that's why there's the home, initialRoute and routes property. The builder callback should be to set things above all the routes (such as a ScrollConfiguration or a custom MacosTheme), not to generate the route.

There shouldn't be an Overlay widget on the build method because it is already implemented by Navigator. If the dev wants the overlay on the builder widget, he/she can add it.

[GroovinChip]

I don't think it's a good solution. The builder callback shouldn't be to add the home page, that's why there's the home, initialRoute and routes property. The builder callback should be to set things above all the routes (such as a ScrollConfiguration or a custom MacosTheme), not to generate the route.

There shouldn't be an Overlay widget on the build method because it is already implemented by Navigator. If the dev wants the overlay on the builder widget, he/she can add it.

Yeah I only returned the Demo in the builder to quickly see if it the builder would work.