accounts ui: Offer Flutter's [LicensePage], to acknowledge software used by the app

[chrisbobbe]

(EDITED following #101 (review) . Notably, this revision doesn't address #100.)

---

From this new page, the user can tap a button to show [LicensePage],
so resolving #99.

Fixes: #99

[chrisbobbe]

(Fixed a formatting nit I just noticed.)

[gnprice]

Design-wise, I think a standard solution here would be an "overflow menu" at the end of the app bar. See mentions of "overflow" in:
https://api.flutter.dev/flutter/material/AppBar/actions.html
https://api.flutter.dev/flutter/material/PopupMenuButton-class.html
https://m3.material.io/components/top-app-bar/guidelines

That would also have the advantage of imposing less of a constraint on how we lay out the list of accounts. Putting them at the top feels likely to be annoying for the common case of one or two accounts, for people using a phone and having to reach up to the top with their thumb.

[gnprice]

Rather than "help", I think I'd file this under an "about" area. So in full:

• in app bar, overflow menu has an item "About Zulip"
• That item goes to a screen that's just a settings-style page, with a couple of items on it
• One is "Zulip" and a version number or something — just to help it feel less empty
• The other is "Open-source licenses"
• That one uses showLicensePage to go to the Flutter-provided license page.

[gnprice]

This gets pretty long before it becomes clear what the payload is (with for (… in globalStore.accountEntries) and _buildAccountItem). It'd be good to pull out as a local variable.

[chrisbobbe]

Sure, I'll update this soon.

That item goes to a screen that's just a settings-style page, with a couple of items on it

I'm wary of putting too much design work into a new "About" page at this early stage, when showAboutDialog and AboutDialog exist—whether by tailoring it a lot to its specific purpose, or building the right reusable widgets for generic "settings-style pages" we'll undoubtedly have several of eventually. I can try something quick with e.g. a ListView and ListTiles; how does that sound?

[gnprice]

I can try something quick with e.g. a ListView and ListTiles; how does that sound?

Yeah, definitely would want to keep it to something quick. I think that plan sounds like a good one.

If you find that the tools at hand don't seem to give you a quick way to get something reasonable by that approach, then showAboutDialog would be fine. It just feels like a less typical UI in a mobile app. The dialog is also not particularly amenable to adding another item or two for things like a terms of service, whereas once there's a settings-style screen for "About" it's easy to throw one more item in there (and rename to like "About & terms").

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! This UI looks good. Two small comments in the code.

[gnprice]

Let's move this inside the onSelected callback. That way it doesn't add any work to do at build time, and we only do the work when the user has chosen to interact with the menu, which is much less common.

… Hm, in fact: I was about to write that this method is presumably cheap and so it doesn't much matter in this case and it's mostly a matter of maintaining good patterns — but it turns out that [Navigator.of] is actually expensive! From its docs:

This method can be expensive (it walks the element tree).

So yeah, definitely to be avoided in the build phase, and invoked only when handling an actual user interaction that needs it.

[gnprice]

(And I guess that also makes an illustration of why deferring things from the build method that are really only needed in event handlers, even when one presumes they're probably cheap, is a good pattern — if you haven't carefully checked that they're cheap, you might be surprised.)

[chrisbobbe]

Interesting! In the common case of an event handler that just has to walk the tree once (it doesn't need the NavigatorState for two or more tasks), it might make sense to favor Navigator.push(context, …) over Navigator.of(context).push(…), which would prevent the tree-walking work from straying outside the event handler, as it did in this revision.

[gnprice]

Hmm, awkward that this info is async.

If we do find ourselves showing a frame or two where we don't have the version number yet, I think it'd be better to still show this list item and just have some kind of placeholder for the actual value, rather than leave out the whole list item. That should reduce the amount of visible jank.

So e.g.:

Suggested change

	if (_packageInfo != null) ...[
	ListTile(title: const Text('App version'), subtitle: Text(_packageInfo!.version)),
	],
	ListTile(
	title: const Text('App version'),
	subtitle: Text(_packageInfo?.version ?? '(…)')),

[gnprice]

To simulate that situation, you can insert an await Future.delayed(…):

       final result = await PackageInfo.fromPlatform();
+      await Future.delayed(Duration(milliseconds: 20));
       setState(() {
         _packageInfo = result;

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! Looks good; merging.