zulip-flutter
zulip-flutter copied to clipboard
content: Consolidate message-content base style and apply it more accurately
I have four main goals for this PR:
- Stop applying the Zulip-message text color to things that aren't Zulip-message text (and refactor to make these errors easy to avoid)
- Don't regress on performance in the message list (for example by making a
DefaultTextStylefor every message) - Define and use a consolidated "base message text style" for content widgets to inherit from (which I do by lumping together the text color and the
Paragraphstyles) - Add a
ThemeExtensionto our app'sThemeData, as a home for certain content styles (that we don't want to recompute for each message, or that differ between light and dark theme and will need to belerped for the fade animation)
Along the way, I make some small changes/fixes:
- Message sender name was
hsl(0deg 0% 15%); it now matches web athsl(0deg 0% 20%) - #697 (which I filed just now)
<br>in a block context, a.k.a.LineBreakNode, has the proper/expected font size and line height, instead of values from a Material default. (Inconspicuous and not worth filing an issue for; I don't know how to make one of these except with help from the server bug that causes #641.)- Code blocks, and our basic math-block implementation, now have 1.4x line height instead of 1.43x. (This is to match web instead of a Material default, and is inconspicuous.)
| Before | After |
|---|---|
Fixes: #697 Related: #95
Let's skip the refactor that passes ambientTextStyle everywhere:
ec772121a content [nfc]: Apply default color by passing down ambientTextStyle
because it seems like it makes a lot of the code worse by having to pass around a new parameter that mostly just goes through untouched. Doing that through a widget subtree is exactly the sort of thing that InheritedWidget is designed to replace. And I'm dubious this way is necessarily any faster — it's doing a little more work in all those widget constructors within each message, in place of doing a little more work at the root widget of the message.
I think having one DefaultTextStyle at the top of each MessageContent is fine. Widgets are cheap, as long as they're simple widgets.
Avoiding DefaultTextStyle.merge, in favor of giving some already-computed style directly, will be good for explicitness as well as for potentially a small performance gain. ContentTheme looks like a good strategy for doing that.
Sure, makes sense. Revision pushed.
Thanks for the review! Revision pushed, and here's a fresh batch of screenshots from it. One interesting finding is that the difference between 1.43x and 1.4x line height (in code blocks) doesn't seem to be visible on my iPhone. Maybe there's a step that rounds to the nearest physical pixel, and maybe it would be observable on a device with higher pixel density than mine.
| Before | After |
|---|---|
Cool, thanks for the revision and the screenshots!
I see that sender-name-color difference, though it's awfully subtle — I had to look several times before I spotted it.
In the last screenshot, the difference in unordered list-item markers is pretty noticeable. The new, larger size definitely looks better.
All looks good; merging.