poll: Support read-only poll widget UI.

[PIG208]

The UI follows the webapp until we get a new design.

The dark theme colors were tentatively picked. The TextStyles are the same for both light and dark theme. All the styling are based on values taken from the webapp.

References:

• light theme:

  https://github.com/zulip/zulip/blob/2011e0df760cea52c31914e7b77d9b4e38e9ee74/web/styles/widgets.css#L138-L185 https://github.com/zulip/zulip/blob/2011e0df760cea52c31914e7b77d9b4e38e9ee74/web/styles/dark_theme.css#L358

• dark theme:

  https://github.com/zulip/zulip/blob/2011e0df760cea52c31914e7b77d9b4e38e9ee74/web/styles/dark_theme.css#L966-L987

Fixes #165.

[PIG208]

Preview

light	dark

[PIG208]

Will need to make some changes today to the first commit to get it ready for review!

[chrisbobbe]

Thanks! Small comments below.

[chrisbobbe]

I think this might need a TODO for localizing the parenthesis characters (like to use full-width parens in Chinese).

[chrisbobbe]

Let's add a TODO(i18n) like this:

        // TODO(i18n): List formatting, like you can do in JavaScript:
        //   new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Zixuan'])
        //   // 'Chris、Greg、Alya、Zixuan'

(we have this TODO in a few places already)

[chrisbobbe]

Suggested change

	: Text(zulipLocalizations.pollWidgetQuestionMissing,
	style: textStylePollBold.copyWith(fontStyle: FontStyle.italic));
	: Text(zulipLocalizations.pollWidgetQuestionMissing,
	style: textStylePollBold.copyWith(fontStyle: FontStyle.italic));

[PIG208]

Thanks! Updated the PR.

[chrisbobbe]

Thanks! LGTM except bump on #912 (comment) , and I'll mark this for Greg's review.

[PIG208]

While this is pretty short, I'm not sure if we need a better way to organize ZulipMessageContent, PollContent, and MessageContent. The file lib/model/content.dart is predominantly about our AST for Zulip HTML content.

[PIG208]

Pushed some simplifications to test/widgets/poll_test.dart.

[PIG208]

Update: use spacing when applicable and fix an issue with large vote numbers:

before	after

[gnprice]

Thanks for the revision! Generally this looks good — a few comments below.

I haven't read closely all the details of the build method, but I'll skip doing so; this seems good enough.

Though it looks like @alya hasn't been consulted on this thread yet. Alya, any feedback on the UI in the screenshots above?

The second to last commit summary is intended to be discarded as the final commit squashes into it.

I recommend making the commit messages into a draft of what you'd like to see merged. 🙂 This sort of remark would be easy for me to miss.

In general if there are details you want to say only at review time and don't think belong in the Git history, the PR thread (or a chat thread) is a good venue for those.

In that commit message I think the details are good for the Git history, though:

The UI follows the webapp until we get a new design.

The dark theme colors were tentatively picked. The `TextStyle`s are
the same for both light and dark theme. All the styling are based on
values taken from the webapp.

References:

  - light theme:

    https://github.com/zulip/zulip/blob/2011e0df760cea52c31914e7b77d9b4e38e9ee74/web/styles/widgets.css#L138-L185
    https://github.com/zulip/zulip/blob/2011e0df760cea52c31914e7b77d9b4e38e9ee74/web/styles/dark_theme.css#L358

  - dark theme:

    https://github.com/zulip/zulip/blob/2011e0df760cea52c31914e7b77d9b4e38e9ee74/web/styles/dark_theme.css#L966-L987

They give good context for what we were thinking when we made the particular decisions that appear in that code. That's the sort of thing that's helpful to a later reader who's, for example, thinking about changing the code and wants to know what background they might be missing.

[gnprice]

This should use option.voters.isNotEmpty instead. Or make voterNames nullable, and use null to indicate there were no voters.

As is, by relying on whether a string is empty this makes the condition dependent on whether, say, the one voter managed to have an empty fullName. That probably shouldn't be allowed to happen… but our logic for whether we tell the user there were voters at all shouldn't depend on whether the voters have surprising names.

[gnprice]

These two new cases don't belong in a group that's about handleMessageEvent, because they're not really about that method — in particular they're not any more specific to that method than they are to fetchInitial or fetchOlder.

In general this test file is one where our usual principle of organizing the tests in the same order as the code they test often breaks down: model/message_list.dart is one of our most intricate areas of model code, and might be the one file with the greatest complexity of interrelationships between different parts of its implementation.

You can see that organizing principle already broken down in roughly the latter half of the file: the "regression tests for …" and "stream/topic muting" groups, the tests "showSender is maintained correctly" and "recipient headers are maintained consistently", and the checkInvariants function which underlies most of the tests and describes invariants that cut across the implementation code.

So these can go in a new small group of their own, perhaps just above "recipient headers are maintained consistently" since they're both about logic within _MessageSequence.

Then I think we don't need these commits:
1919c1b msglist test [nfc]: Make a group for MessageEvent tests
99a62b7 msglist test [nfc]: Name test groups after the event handler method
b69f0bf msglist test [nfc]: Move notifyListeners tests to match implementation

[gnprice]

So, the downside of moving these checks-extensions to their own file is that its name message_list_checks.dart starts the same way as the test file message_list_test.dart. That makes it get in the way of tab-completion, when writing the test file's name for a git log or flutter test command.

For that reason I've been keeping the extensions within the test file when they're pretty specific to the one file, or to that and one or two others. I think these are best kept here for that reason.

[alya]

Looks reasonable to me!

I remember making some comments about polls earlier (which I believe have been addressed), but I guess that was on a different PR.

[PIG208]

The second to last commit is to be kept now, because previous those two were in the reverse order because of the way the tests were organized. My (stale) comment on that did make it more confusing!

Pushed an update addressing the review comments.

[gnprice]

Thanks for the revision! This all looks good — merging. Happy to get this feature in.

I took out one new paragraph the latest revision added to a commit message:

-    _reparseContent currently happens when there is content edit, which is
-    forbidden for poll messages, (and you can't turn an existing message
-    into a poll).  In most cases the same `PollContent` instance will be
-    used for each poll message, unlike `ZulipContent`, and a poll message
-    never needs to get reparsed.

It's true that over the lifetime of a given MessageListView, in a release build, a given poll message will be represented by one PollContent instance. But it can get a new PollContent if MessageListView.reassemble is called, which happens routinely in development. And in all builds, if the same message appears in a new message list, it'll get a new PollContent instance there. So there's no guarantee of uniqueness.

From a performance perspective, the main point here is that allocating one of these trivial PollContent wrapper objects is a much cheaper operation than parsing a message's HTML, which is what we do for normal messages in all the same situations where we make a PollContent for polls.