New license page. #57588

TonicArtos · 2020-05-19T18:02:37Z

Description

This is a PR addressing #57226 - Proposal: New UI for Licenses Page.

This PR replaces the previous single panel license page with one that uses a master/detail flow (MDFlow) to display packages and their respective licenses.

Existing behaviour

The existing license page is a single list of all licenses collected from the packages linked in the application. Also, at the top of the list is an entry which displays some information about the application.

Issues with existing behaviour

It is hard to navigate.
Because all versions of a license must be displayed, some packages have many tens of similar entries in this list, often with little more than a date change. Should a user wish to see licenses for a particular package, they must hunt through the list to find the package and its licenses.
It is not polished.
The existing license page does not adjust for the size of the screen and can present extremely long strings of legalese running across the screen.

How this PR addresses the above (Changes)

The License Page API remains unchanged. The logic for processing the license data is kept largely the same. This PR changes how the licenses are displayed, by introducing a responsive UI using the master/detail UI pattern. For now I am calling it Master Detail Flow, or MDFlow.

MDFlow manifests as two layouts depending on the screen size. On small and medium displays, as determined by the breakpoints given by the Material Design Spec, MDFlow utilises a nested layout. On large displays, MDFlow uses a two panel (lateral) layout. MDFlow is implemented in this PR using a Navigator for the nested layout, and a Stack for the lateral layout. The master and detail views are built using builders. For the interactive component, detail pages are requested from the master view using a proxy obtained by a widget lookup on the build context; MasterDetailFlow.of(context).

Demo of new license page

New License Page showing packages (small and medium displays)

New License Page showing licenses for a package (small and medium displays)

New License Page (large displays)

Existing License Page (all displays)

Related Issues

Resolves #57226
Resolves #18281

Tests

Widget tests for the new UI remain to be updated.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

Yes, this is a breaking change. If not, delete the remainder of this section.
- Design doc waved. See @Hixie New license page. #57588 (comment)
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Migration should not be required

TonicArtos · 2020-05-20T10:28:57Z

@Hixie The new UI require changes to the tests in about_test.dart. The changes involve adding navigation of packages to show the license content, something the tests current assume is unneeded.

Anyway, is it okay to update these tests without writing a design doc?

Hixie · 2020-05-20T21:00:08Z

@Hixie The new UI require changes to the tests in about_test.dart. The changes involve adding navigation of packages to show the license content, something the tests current assume is unneeded.

Anyway, is it okay to update these tests without writing a design doc?

Yeah that should be fine. Thanks for checking.

TonicArtos · 2020-05-21T11:53:14Z

Okay, so I am looking for feedback now. I think these are the right people to tag. @HansMuller, @timsneath, @Hixie.

Hixie · 2020-05-21T21:38:23Z

packages/flutter/lib/src/material/about.dart

+        MaterialLocalizations.of(context);
+    final double gutterSize = _getGutterSize(context);
+
+    if (widget.scrollController == null) {


there's a lot of duplicated content here, might be worth seeing if there's a way to factor it out so the branches are only the pieces that differ

Hixie · 2020-05-21T21:38:37Z

packages/flutter/lib/src/material/about.dart

+typedef _ActionBuilder = List<Widget> Function(
+    BuildContext context,
+    _ActionLevel actionLevel,
+    );


these can all just be on one line

Hixie · 2020-05-21T21:39:54Z

packages/flutter/lib/src/material/about.dart

+    );
+  }
+
+  Widget _buildPackagesView(final BuildContext _, final bool isLateral) {


here and in other places where you have build methods, might be better to factor those out into their own widgets

Hixie · 2020-05-21T21:40:27Z

Overall this looks great. I'll defer to @HansMuller or @goderbauer for more detailed review.

TonicArtos · 2020-05-22T08:35:41Z

Overall this looks great. I'll defer to @HansMuller or @goderbauer for more detailed review.

Okay. I'll follow your guidance once I get their feedback. I don't want to be making changes over the top of what they may be reviewing.

gspencergoog · 2020-05-21T21:49:48Z

packages/flutter/lib/src/material/about.dart

+        expand: false,
+        builder: (BuildContext context, ScrollController controller) {
+          return MouseRegion(
+            // Workaround bug where things behind sheet still get mouse hover events.


Can you turn this into a TODO saying to remove the workaround, and reference the issue that describes the bug (and if there isn't one, please file one)?

packages/flutter_localizations/lib/src/material_localizations.dart

gspencergoog · 2020-05-26T15:44:11Z

packages/flutter_localizations/lib/src/l10n/material_en.arb

@@ -72,6 +72,14 @@
    "description": "The title for the Flutter licenses page."
  },

+  "licensesPackageDetailTextZero": "No licenses",


Shouldn't we just skip (and not display) any packages that have no licenses at all? That way we don't need to do translation for the zero case. You could update the description to say that it isn't allowed to have a value of zero.

TonicArtos · 2020-06-05T12:56:20Z

Sorry, I got caught up this week. I'll make the changes this weekend.

gspencergoog · 2020-06-17T20:33:38Z

@TonicArtos have you had a chance to take a look at the changes?

TonicArtos · 2020-06-18T07:04:09Z

@gspencergoog Sorry for the delay. It is a pretty messed up time. I've gone through the comments and implemented each request. They are referred on the specific commit addressing them. It all looks fine to me on my devices and with the local tests.

However, I don't understand why customer_testing-windows failed on the CI system. The issue looks like an image pixel comparison check. Is there some way to get more detail on this so I can fix it?

gspencergoog

Thanks for the update! It's looking pretty good, I just saw some small things to address.

gspencergoog · 2020-06-19T16:44:00Z

packages/flutter/lib/src/material/about.dart

+    );
+  }
+
+  Widget _packageLicensePage(BuildContext _, Object args, ScrollController sc) {


Use a more descriptive name for sc. (e.g. scrollController).

gspencergoog · 2020-06-19T16:46:06Z

packages/flutter/lib/src/material/about.dart

-                      ),
-                    ),
-                ],
+    final MaterialLocalizations localisations = MaterialLocalizations.of(context);


localisations => localizations. We try to use American English spellings, for consistency.

Ah, the z. It kills me. Yeah, okay, I should have stuck to the US spelling no matter how it does the head in.

gspencergoog · 2020-06-19T16:52:06Z

packages/flutter/lib/src/material/about.dart

@@ -623,3 +982,651 @@ Widget _defaultApplicationIcon(BuildContext context) {
  // TODO(ianh): Get this from the embedder somehow.
  return null;
 }
+
+double _getGutterSize(BuildContext context) => MediaQuery.of(context).size.width >= 720 ? 24 : 12;


We try not to have "magic" numbers. It's best to have a constant for each of the numbers like this, so that they're self-documenting.

For instance:

static const int _narrowGutterWidthLimit = 720; static const int _wideGutterSize = 24; static const int _narrowGutterSize = 12;

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-mysterious-and-magical-numbers-that-lack-a-clear-derivation

You're right. I remember thinking I would fix this when I wrote it. I clearly didn't.

gspencergoog · 2020-06-19T16:58:25Z

packages/flutter/lib/src/material/about.dart

+
+  Widget _packageLicensePage(BuildContext _, Object args, ScrollController sc) {
+    assert(args is _DetailArguments);
+    final _DetailArguments a = args as _DetailArguments;


Use a more descriptive name for a. e.g. detailArguments.

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-abbreviations

Sorry. It looks like I was trying to avoid the stuttering caused by the cast. The style guide should have been followed.

gspencergoog · 2020-06-19T17:00:15Z

packages/flutter/lib/src/material/about.dart

+            style: Theme.of(context).textTheme.bodyText2,
+            textAlign: TextAlign.center,
+          ),
+          Container(height: 18.0),


Here, and the next Container, please define this constant as a separate static const variable to make it self documenting.

Also, consider using a SizedBox instead of a Container: SizedBox can be a const object, and so it's more performant.

gspencergoog · 2020-06-19T17:08:17Z

packages/flutter/lib/src/material/about.dart

+        return LayoutBuilder(
+            builder: (BuildContext context, BoxConstraints constraints) {
+              final double availableWidth = constraints.maxWidth;
+              if (availableWidth >= (widget.breakpoint ?? 840)) {


Make 840 into a constant with a name.

gspencergoog · 2020-06-19T17:09:22Z

packages/flutter/lib/src/material/material_localizations.dart

+  @override
+  String licensesPackageDetailText(int licenseCount) {
+    switch (licenseCount) {
+      case 0:


If you skip packages with zero licenses, this case should assert.

Added assertion licenseCount >= 1, removed case 0.

gspencergoog · 2020-06-19T17:21:36Z

packages/flutter/lib/src/material/about.dart

+  String firstPackage;
+
+  void addLicense(LicenseEntry entry) {
+    for (final String package in entry.packages) {


Perhaps skip packages with zero licenses?

I said below in #57588 (comment) that I would do this. However, going over the code again it really is already the case that each package always has one or more licenses. This is because we only know about a package should it be recorded as a package to which a given license applies.

Obviously the code must not be clear enough as is, so I'll go through this part and added comments to try and clarify it.

packages/flutter/lib/src/material/about.dart

gspencergoog · 2020-06-19T17:29:22Z

packages/flutter_localizations/lib/src/l10n/generated_material_localizations.dart

+  String get licensesPackageDetailTextOther => '\$licenseCount licenses';
+
+  @override
+  String get licensesPackageDetailTextZero => 'No licenses';


All of these "Zero" cases would be eliminated if you skip packages with zero licenses.

The license page is an inversion of the data hierarchy. LicenseRegistry collects licenses and includes the packages from which those licenses came from as a property on each license. As such, there is a many-to-many relationship between licenses and packages with the constraint that each package has at least one license, and each license has at least one package.

So the zero case is not checked for, since by implementation, it should come up. However, it is undeniably more robust to add checking for, and the exclusion of, packages that have zero licenses. So I will do this.

However. I would like to highlight, should you include special dispensation for zero license packages, it may be arguable that it is better to highlight packages with zero licenses by using a special unlicensed, or commons, page. But, I also suggest that it is unneeded and zealous work to add this. We should never normally encounter packages with zero licenses. Unless, of course, there is a design change down the line.

I just realised why this zero case keeps getting regenerated. It is coming from the default text generated for the different locales from when I ran gen_missing_localizations.dart as instructed to do when adding to material_localizations.

gspencergoog · 2020-06-19T17:35:04Z

As for the customer_testing failures, I don't think those were related to your code. If you rebase the the HEAD of the master branch, I think it will pass.

- re: flutter#57588 (comment) - re: flutter#57588 (comment)

TonicArtos · 2020-06-21T12:41:58Z

So I did the rebase. See if this will work. I can redo it with a squash if you like.

I am not entirely sure this is the right way to go about this.

gspencergoog

This looks pretty good! And it looks like the customer_testing is passing now too.

Unless you have any more updates, I can merge this as soon as the tree is green. I'll wait until you say it's ready, though.

gspencergoog · 2020-06-22T18:04:36Z

packages/flutter/test/material/about_test.dart

+    // the safe area is sufficiently respected.
+    expect(
+      tester.getTopLeft(find.text('Licenses')),
+      const Offset(16.0 + safeareaPadding, 18 + safeareaPadding),


Be consistent about doubles: either use 16, or 18.0.

Okay, I'll go through the change list and conform to 18.0 everywhere. On closer inspection it seems to be the convention.

- re: flutter#57588 (comment)

TonicArtos · 2020-06-22T20:48:56Z

This looks pretty good! And it looks like the customer_testing is passing now too.

Okay I addressed your last comment. I also went through the CL one last time and noticed the documentation still mentioned the localised zero case was required, so I fixed that too.

Unless you have any more updates, I can merge this as soon as the tree is green. I'll wait until you say it's ready, though.

I see no reason to hold back, once these last two commits complete the CI process and the tree goes green again.

Oh, if you want to talk about the MDFlow thing and seeing if that can go somewhere, I'd be happy to. I think with work it could become something quite good.

Otherwise, or in addition, I'll move on and see about getting all this done again for the Cupertino side of things too. Over at #57638.

gspencergoog · 2020-06-22T21:51:04Z

Thanks Tonic! Great work, we really appreciate it!

I'll commit this as soon as the tree is green and it passes the CI tests.

TonicArtos · 2020-06-23T09:33:27Z

@gspencergoog Do you want me to squash the PR or are you going to do that?

gspencergoog · 2020-06-23T16:13:40Z

No, no need to squash: it'll do that automatically when I merge. The tree's been extra red: we had an issue in our device lab.

…ve zero licenses.

… that have zero licenses." This reverts commit f1b9fdf.

…ve zero licenses.

This amends #57588 by adding code that also handles the zero license case, and adds translation strings for that.

goderbauer · 2020-07-01T20:48:03Z

packages/flutter/lib/src/material/about.dart

+      onTap: () {
+        print('draggable');
+      },


This print (and probably the entire gesture detector?) should probably be removed?

Ooh, yes, good point. @TonicArtos can you take care of that?

Yes. It is late here. I'll get caught up and get the fix sorted tomorrow.

OK, no huge rush: most of us will be on holiday until Monday (it's Independence Day in the U.S.).

I went through my journal. It turns out the GestureDetector is left from when I was looking at #59741. I forgot to remove it once I figured what was happening.

PR #60730 Remove superfluous GestureDetector.

gspencergoog · 2020-07-01T20:56:46Z

Sure, that would be great.

mono0926 · 2020-08-06T02:56:09Z

packages/flutter/lib/src/material/about.dart

+    if (widget.scrollController == null) {
+      page = Scaffold(
+        appBar: AppBar(
+          title: _PackageLicensePageTitle(title, subtitle, theme.primaryTextTheme),


@TonicArtos

Why did you specify theme.primaryTextTheme?

It may break color system when AppBarTheme is used for example.

Indeed, it broke my background color.

I would have preferred for this to (private) widget to have been factored a little differently - to not have used primaryTextTheme.

If you can file an issue with a simple test case that demonstrates a regression, we'll try and fix it.

@HansMuller

If you can file an issue with a simple test case that demonstrates a regression, we'll try and fix it.

Okay, I've created an issue: #63239.

@HansMuller

I tried to fix at #63249, but I’m welcome to be closed and refactored by your pull request.

I would have preferred for this to (private) widget to have been factored a little differently - to not have used primaryTextTheme.

If you can file an issue with a simple test case that demonstrates a regression, we'll try and fix it.

Sorry about the issue. What do you think the original solution should have been?

@TonicArtos

What do you think the original solution should have been?

I think #63249 is the simplest fix for existing implementation.

This is a PR addressing flutter#57226 - Proposal: New UI for Licenses Page. This PR replaces the previous single panel license page with one that uses a master/detail flow (MDFlow) to display packages and their respective licenses. The License Page API remains unchanged. The logic for processing the license data is kept largely the same. This PR changes how the licenses are displayed, by introducing a responsive UI using the master/detail UI pattern. For now I am calling it Master Detail Flow, or MDFlow. MDFlow manifests as two layouts depending on the screen size. On small and medium displays, as determined by the breakpoints given by the Material Design Spec, MDFlow utilises a nested layout. On large displays, MDFlow uses a two panel (lateral) layout. MDFlow is implemented in this PR using a Navigator for the nested layout, and a Stack for the lateral layout. The master and detail views are built using builders. For the interactive component, detail pages are requested from the master view using a proxy obtained by a widget lookup on the build context; MasterDetailFlow.of(context).

This amends flutter#57588 by adding code that also handles the zero license case, and adds translation strings for that.

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels May 19, 2020

googlebot added the cla: yes label May 19, 2020

fluttergithubbot added the a: internationalization Supporting other languages or locales. (aka i18n) label May 20, 2020

TonicArtos changed the title ~~Draft of new license UI.~~ New license page. May 20, 2020

HansMuller requested a review from gspencergoog May 21, 2020 20:22

Hixie reviewed May 21, 2020

View reviewed changes

gspencergoog reviewed May 26, 2020

View reviewed changes

Hixie mentioned this pull request Jun 3, 2020

LicensePage and LicenseRegistry have many duplicate entries #47274

Closed

TonicArtos marked this pull request as ready for review June 18, 2020 07:01

gspencergoog reviewed Jun 19, 2020

View reviewed changes

Do not use magic numbers.

5a159fc

- re: flutter#57588 (comment) - re: flutter#57588 (comment)

TonicArtos force-pushed the md_license branch from 63b2847 to 5a159fc Compare June 21, 2020 12:40

TonicArtos added 2 commits June 22, 2020 00:15

Update l10n test to match rejected zero case.

9565756

I am not entirely sure this is the right way to go about this.

Use throwsAssertionError instead of matcher.

9197101

gspencergoog approved these changes Jun 22, 2020

View reviewed changes

TonicArtos added 2 commits June 23, 2020 04:30

Append .0 to doubles to distinguish from ints.

ca3d6e6

- re: flutter#57588 (comment)

Fix incorrect documentation of zero case decl.

bc060bb

gspencergoog added waiting for tree to go green and removed work in progress; do not review labels Jun 23, 2020

gspencergoog merged commit 65550d0 into flutter:master Jun 23, 2020

gspencergoog mentioned this pull request Jun 29, 2020

Revert: New license page. #60548

Closed

gspencergoog added a commit to gspencergoog/flutter that referenced this pull request Jun 29, 2020

Revert flutter#57588 because it causes problems with packages that ha…

f1b9fdf

…ve zero licenses.

gspencergoog added a commit to gspencergoog/flutter that referenced this pull request Jun 29, 2020

Revert "Revert flutter#57588 because it causes problems with packages…

658d038

… that have zero licenses." This reverts commit f1b9fdf.

gspencergoog added a commit to gspencergoog/flutter that referenced this pull request Jun 29, 2020

Revert flutter#57588 because it causes problems with packages that ha…

6d90381

…ve zero licenses.

gspencergoog mentioned this pull request Jun 29, 2020

New license page with fix for zero licenses. #60552

Merged

1 task

gspencergoog added a commit that referenced this pull request Jun 30, 2020

New license page with fix for zero licenses. (#60552)

ca71355

This amends #57588 by adding code that also handles the zero license case, and adds translation strings for that.

goderbauer reviewed Jul 1, 2020

View reviewed changes

TonicArtos deleted the md_license branch July 8, 2020 02:11

mono0926 reviewed Aug 6, 2020

View reviewed changes

mono0926 mentioned this pull request Aug 8, 2020

Fix title color of license detail page #63249

Merged

9 tasks

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

New license page with fix for zero licenses. (flutter#60552)

1d4f14f

This amends flutter#57588 by adding code that also handles the zero license case, and adds translation strings for that.

github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021

New license page. #57588

New license page. #57588

Conversation

TonicArtos commented May 19, 2020 • edited

Description

Existing behaviour

Issues with existing behaviour

How this PR addresses the above (Changes)

Related Issues

Tests

Checklist

Breaking Change

TonicArtos commented May 20, 2020

Hixie commented May 20, 2020

TonicArtos commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented May 21, 2020

TonicArtos commented May 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonicArtos commented Jun 5, 2020

gspencergoog commented Jun 17, 2020

TonicArtos commented Jun 18, 2020 • edited

gspencergoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonicArtos Jun 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gspencergoog commented Jun 19, 2020

TonicArtos commented Jun 21, 2020

gspencergoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonicArtos commented Jun 22, 2020

gspencergoog commented Jun 22, 2020

TonicArtos commented Jun 23, 2020

gspencergoog commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonicArtos Jul 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gspencergoog commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mono0926 Aug 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mono0926 Aug 9, 2020 • edited

Choose a reason for hiding this comment

TonicArtos commented May 19, 2020 •

edited

TonicArtos commented May 22, 2020 •

edited

TonicArtos commented Jun 18, 2020 •

edited

TonicArtos Jun 21, 2020 •

edited

TonicArtos Jul 1, 2020 •

edited

mono0926 Aug 8, 2020 •

edited

mono0926 Aug 9, 2020 •

edited