Add personal user_id & full_name data, updating latter via events & in UI

[neiljp]

I managed to get the dev environment set up today, and this seemed to be a good starting point after digging around to wrap my head around how some of it works.

The commits as they stand should be fairly clean, but any feedback on Dart/Flutter style is appreciated.

In particular, there may well be code layout issues, since I'm not using an IDE right now, and tried a dart format at one point which may have made some unintentional changes - I'm not entirely sure if the linting is the same as in Flutter?

On a personal note, it's always rewarding to see events from a server translate into an update on another device, and finally finding the correct incantation for it to do so in the UI was great here too :)

[gnprice]

Neat, thanks! Taking a look.

[gnprice]

Generally this looks great. Various small things below. I'm particularly curious about that ListenableBuilder.

[gnprice]

nit: in this API type, let's keep the properties in the same order as they appear in the docs, even when that's not the most logical order in itself.

That's helpful for comparing the type to the docs, as we fill in more and as we update it for changes in the API.

For these properties, it looks like that means they go at the end of the list.

[gnprice]

nit:

Suggested change

	// The user's personal data
	final int user_id;
	String full_name;
	final int user_id;
	String full_name;

We can let the names speak for themselves :-)

When there's more here, we might add heading comments like these, but at that point it'll be easier to see what structure of headings we might want.

[neiljp]

I do broadly agree, though these names are used in more complex structures, which is why I originally included the title.

[gnprice]

Ah, I see — to distinguish them from the user_id or full_name of other users.

Sure, reasonable to make that explicit. Perhaps like this:

  // This user_id, etc., refer to the self user.

[gnprice]

It looks like this type describes a realm_user event with 'op': 'update':
https://zulip.com/api/get-events#realm_user-update
but not the other kinds:
https://zulip.com/api/get-events#realm_user-add
https://zulip.com/api/get-events#realm_user-remove

This will be the first event we handle that has the op pattern. I think there is probably not a real benefit to having an intermediate base class for the three realm_user events, so let's keep it simple:

• call this RealmUserUpdateEvent; update dartdoc to say it's for op update
• add an op getter, similar to type, so that toJson works correctly
• in Event.fromJson, write:

      case 'realm_user':
        switch (json['op'] as String) {
          case 'update': return RealmUserUpdateEvent.fromJson(json);
          // TODO add realm_user/add and realm_user/remove events
          default: return UnexpectedEvent.fromJson(json);
        }

Later we'll want to handle the add and remove variants too, but no need for that in this PR.

[gnprice]

Suggested change

	// custom_profile_field has sub-parts to affect which id/value/rendered_value is updated
	// final CustomProfileFieldValueUpdate? custom_profile_field; // TODO handle

(following the format of others in this file like edit_history, and making it a TODO comment so it's distinct from the "ignoring this because it's deprecated" comments)

[gnprice]

Can it? The docs say this is never null if present:

is_billing_admin: boolean

I think this can also drop the TODO-server comment, because I think there isn't anything we'd actually change here when we make that update.

[neiljp]

I think my null comment was related to this being not as tidy as I'd have preferred, and representing the null vs not-present issue.

Re the TODO-server comment, my original understanding was that these were simply notes related to when a field was introduced, but this makes more sense 👍

[gnprice]

Suggested change

	final String? avatar_url;
	final String? avatar_url_medium;
	final String? avatar_source;
	final String? avatar_version;
	// TODO express that all four avatar-related properties will be present if any of them is
	final String? avatar_url;
	final String? avatar_url_medium;
	final String? avatar_source;
	final String? avatar_version;

[gnprice]

nit: I think this feels to me pretty specific to the event, so let's put it in the events.dart file. Can go just below the event type itself, to match the "summary first, details after" pattern that file has so far.

[gnprice]

Suggested change

// TODO Update our other data, and that of other users

No need for a TODO in this spot at the end of the PR; once the event is being fully handled as to our existing data structures, there's nothing actionable to do here.

We'll have more to add here for sure, but that's a task to be done in tandem with adding the further data structures that need to be updated, so it can be driven as a required step in those changes.

(And that's covered by the admonition in this TODO comment above:

  // TODO lots more data.  When adding, be sure to update handleEvent too.

)

[gnprice]

FYI there's an alternate way you can also write this:

Suggested change

	String? new_full_name = event.person.full_name;
	if (new_full_name != null) {
	full_name = new_full_name;
	if (event.person.full_name != null) {
	full_name = event.person.full_name!;

Both are idiomatic Dart, and have small pros and cons. I think both ways are fine here — just figured I'd mention it since I believe you're new to Dart (like all of us) and might not have seen it.

[neiljp]

I did consider both approaches. I decided on the former based on not repeating the dotted event access (ie. DRY). I'm surprised the ! would be necessary given the null check in each case, but that seems a Dart design.

[gnprice]

Cool. Yeah, the reason the ! is needed is that in Dart, every access of the form thing.property is in principle an access through a getter method (and similarly thing.property = … is through a setter method) — if the getter method happens to be implemented in a trivial way by a field on the object, that's an implementation detail of the object.

(The compiler will certainly look through that abstraction to know the difference when it's generating code, in order to make it efficient, but the language semantics maintain the abstraction.)

So from that perspective, there's no guarantee that just because event.person.full_name returned something non-null once, it'll return something non-null the next time.

[gnprice]

Neat!

Is this ListenableBuilder necessary, though? This HomePage.build method already calls PerAccountStoreWidget.of, which in turn calls context.dependOnInheritedWidgetOfExactType<PerAccountStoreWidget>(). That "dependOn…" method should cause HomePage itself to get rebuilt after the store notifies its listeners. So if that wasn't working in your testing, we should investigate (perhaps in chat.)

Formatting nit: generally the "child", "children", "builder", or similar argument that provides the nested bit of UI goes last. So:

Suggested change

	ListenableBuilder(
	builder: (BuildContext context, Widget? child) {
	return Text.rich(TextSpan(
	text: 'You are: ',
	children: [bold(store.full_name)]));},
	listenable: store,
	),
	ListenableBuilder(
	listenable: store,
	builder: (BuildContext context, Widget? child) {
	return Text.rich(TextSpan(
	text: 'You are: ',
	children: [bold(store.full_name)]));
	}),

[gnprice]

I've also just written some docs for that PerAccountStoreWidget.of method: 8a5fb4d in #26. Take a look and see if that helps clarify things.

[neiljp]

I explored various ways of notifying the UI (eg. notifying on individual Strings), before noticing that the whole account was a notifier via inheritance, and that I needed to explicitly notifyListeners() - which possibly is not necessary with the simple values eg. String.

(I'd not reached the point of exploring trimming out the builder itself beyond that; it was more that things were working so wanted to push before spending too much extra time on it, in case going in completely the wrong direction)

I hadn't dug into that .of call. That Widget is intended as a way to easily associate an account with another widget class, and auto-build on data-change?

Re the builder being necessary, apparently it works without it 👍

I'm not sure if they can be optimization to include it inline, if the base class widget always triggers a rebuild?

I can understand the nit of preferring the smaller listenable: first. I was going from the flutter examples of this, which have them last.

[gnprice]

Re the builder being necessary, apparently it works without it +1

Great. :-) If it hadn't, that would have pointed to something we should definitely investigate to revise my understanding of how this works — and though I was pretty sure it should work from reading the docs, we hadn't really exercised it and so I wasn't sure if it would turn out I'd been missing something.

I explored various ways of notifying the UI (eg. notifying on individual Strings), before noticing that the whole account was a notifier via inheritance, and that I needed to explicitly notifyListeners() -

Yeah, that's what I hoped had happened.

I'll start a chat thread for replying to the other questions. -> https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flutter.20live-updating.20UI/near/1524089

[neiljp]

@gnprice It's been a while since I've had a PR reviewed - and not in Python - so I tried using fixup-style commits except for conflicts or to update the main commit text. All points should be addressed.

[gnprice]

Thanks for the revision!

For our review process, it's easiest to re-review if you go ahead and squash the fixups in, and otherwise revise the branch to the new version of the sequence of commits you propose be merged. (On our end, the key tool for this is git range-diff, combined with the tools/reset-to-pull-request script from zulip/zulip so that previous revisions of the PR are recorded in the reviewer's local reflog; the command I then run looks like git range-diff origin pr/25@{1} pr/25@{0}.)

[gnprice]

@neiljp Do you still have plans to return to this PR?

It will need some rebasing, because there have been changes in main. I just tried a quick git merge to see what the conflicts are like, though, and it wasn't too bad. So I think that won't be a lot of work.

The approach I'd recommend is:

• First, use git rebase -i --keep-base to squash the fixups in while staying on top of the same base commit, without yet rebasing onto the new main.
• Then rebase onto main. At this stage, fix merge conflicts that Git sees, but don't worry about higher-level sorts of conflicts.
• Then fix up the branch as needed to match other changes from main so that flutter analyze && flutter test passes.

A couple of the changes in main that will be relevant:

• The naming convention in lib/api has changed: api [nfc]: Use Dart-standard camel case throughout our source code #51

  I recommend the "rename" feature in Android Studio (and probably VS Code can do similarly well), which makes this painless though a bit repetitive: hit Shift+F6 to get the "rename" dialog, edit the name, hit enter to confirm, then hit F2 to get to the next lint warning and repeat.

• processInitialSnapshot has become PerAccountStore.fromInitialSnapshot: store: Have GlobalStore manage one PerAccountStore per account #27

• We now have the user_id on the Account object: login: Fetch all the data we'll want for an accounts table #55.

  I think that makes the first commit in this series unnecessary (along with its fixups), but the rest are still quite relevant.

[neiljp]

@gnprice I plan to return to this, but GSoC and ZT have been busy (I really wanted to start working more with flutter, but no-one else is supporting ZT).

[gnprice]

We ended up adding this functionality as part of #84, so I'm closing this PR.

Thanks again @neiljp for your work on this! I hope it was an interesting introduction to Dart and Flutter. I'd be glad to see another PR from you in the future.