Prepare for updating accounts in database #526
Conversation
|
The CI failure is a pair of analyzer failures that appear with a Flutter update today. Chris noticed them earlier today and is working on a PR: So this PR is otherwise ready, and should pass CI once it's rebased atop that upcoming other PR. |
| child: RealmContentNetworkImage( | ||
| resolvedSrc, | ||
| filterQuality: FilterQuality.medium, | ||
| width: size, | ||
| height: size, | ||
| )), | ||
| child: resolvedSrc == null ? const SizedBox.shrink() // TODO(log) | ||
| : RealmContentNetworkImage( | ||
| resolvedSrc, | ||
| filterQuality: FilterQuality.medium, | ||
| width: size, | ||
| height: size, | ||
| )), |
There was a problem hiding this comment.
For emoji reactions, if the src doesn't parse, we fall back to showing the emoji name. (We use the same fallback if the src parses but the image fails to load.) This would be nice to do, or we could add a TODO for it.
I wonder if it would be hard to factor the emoji-reaction widget code into something to also use in content. Another thing I notice is that _UnicodeEmoji in widgets/emoji_reaction.dart has careful logic so that the emoji isn't awkwardly small on iOS. (In content, unicode emojis are awkwardly small on iOS.)
There was a problem hiding this comment.
Yeah, it'd be good to reuse the work that went into those reaction emoji so that emoji in content also get the benefit of it. It looks like features that would bring include:
- making Unicode emoji bigger on iOS, counteracting that Flutter-and/or-iOS bug;
- disabling animations based on the platform setting;
- text fallbacks based on the user's emojiset choice;
- text fallbacks when an image doesn't work out.
At the moment we don't have an implementation of emoji-as-text-name at all for content, so I think doing that as a fallback is out of scope for this PR. The URL failing to parse should happen rarely to never — it'd be a bug in the server, and doesn't seem like a real likely kind of bug.
gnprice
force-pushed
the
pr-account-update-prep
branch
from
a91b6da to
9990caf
Compare
We're going to want to update these, when things like the server version or the acked push token change. To avoid endless bugs, it'll then be important that there be just one copy to be updated.
These have been gradually proliferating, and it's time for another round of giving more structure to the list. Also remove a couple of TODO comments that are better expressed in the tracker; and an admonition about handleEvent that is probably no longer helpful because there are enough of these fields that a developer isn't likely to be looking at it when they go to add another one.
The field docs get helpfully propagated by Drift from the table class to the corresponding fields on the record class, which is the one most widely found around the codebase.
This makes use sites a bit shorter and more explicit. We could have made it a getter, but storing the answer directly here on the PerAccountStore is a space-for-time tradeoff: this is something we look up frequently, so the extra word of memory to have a copy directly here seems worth it to save the hash-map lookup each time.
The `message` passed to MessageContent should have the same content that was parsed to produce `content`. In the app we don't insert a Directionality widget of our own, and the GlobalStoreWidget is outside the MaterialApp, not inside.
Now that the caller is getting the Account object by looking it up on the GlobalStore anyway, there's no checking to be gained from passing the whole Account here.
gnprice
force-pushed
the
pr-account-update-prep
branch
from
9990caf to
abc65a2
Compare
|
Did you mean to comment further or merge this? I see you force-pushed after the discussion above. |
|
Ah, those pushes were just rebasing and resolving conflicts. I didn't make any changes in content, just replied to your comment above. Shall we merge this? |
|
Sure, please go ahead! |
Currently we insert
Accountrecords into the database on login, and never update them. We'll need to update them in order to do:and other things.
This branch doesn't yet make any new changes in the database, but it does some refactoring on PerAccountStore and friends which we'll need in order to do so. It also takes care of various things that became noticeable along the way:
store.selfUserIdandself.realmUrl;Uri.parserejects) in the HTML or as a user's avatar;The main commit is this one near the beginning:
store [nfc]: Keep just one copy of Account object
We're going to want to update these, when things like
the server version or the acked push token change.
To avoid endless bugs, it'll then be important that there
be just one copy to be updated.