Skip to content

poll: Support read-only poll widget UI. #912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -545,5 +545,13 @@
"messageIsMovedLabel": "MOVED",
"@messageIsMovedLabel": {
"description": "Label for a moved message. (Use ALL CAPS for cased alphabets: Latin, Greek, Cyrillic, etc.)"
},
"pollWidgetQuestionMissing": "No question.",
"@pollWidgetQuestionMissing": {
"description": "Text to display for a poll when the question is missing"
},
"pollWidgetOptionsMissing": "This poll has no options yet.",
"@pollWidgetOptionsMissing": {
"description": "Text to display for a poll when it has no options"
}
}
4 changes: 3 additions & 1 deletion lib/api/model/submessage.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'dart:convert';

import 'package:flutter/foundation.dart';
import 'package:json_annotation/json_annotation.dart';

import '../../log.dart';
Expand Down Expand Up @@ -354,7 +355,7 @@ class UnknownPollEventSubmessage extends PollEventSubmessage {
/// See also:
/// - https://zulip.com/help/create-a-poll
/// - https://github.com/zulip/zulip/blob/304d948416465c1a085122af5d752f03d6797003/web/shared/src/poll_data.ts
class Poll {
class Poll extends ChangeNotifier {
/// Construct a poll from submessages.
///
/// For a poll Zulip widget, the first submessage's content contains a
Expand Down Expand Up @@ -412,6 +413,7 @@ class Poll {
return;
}
_applyEvent(event.senderId, pollEventSubmessage);
notifyListeners();
}

void _applyEvent(int senderId, PollEventSubmessage event) {
Expand Down
16 changes: 15 additions & 1 deletion lib/model/content.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import 'package:html/dom.dart' as dom;
import 'package:html/parser.dart';

import '../api/model/model.dart';
import '../api/model/submessage.dart';
import 'code_block.dart';

/// A node in a parse tree for Zulip message-style content.
Expand Down Expand Up @@ -73,13 +74,26 @@ mixin UnimplementedNode on ContentNode {
}
}

/// A parsed, ready-to-render representation of Zulip message content.
sealed class ZulipMessageContent {}

/// A wrapper around a mutable representation of a Zulip poll message.
///
/// Consumers are expected to listen for [Poll]'s changes to receive
/// live-updates.
class PollContent implements ZulipMessageContent {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const PollContent(this.poll);

final Poll poll;
}

/// A complete parse tree for a Zulip message's content,
/// or other complete piece of Zulip HTML content.
///
/// This is a parsed representation for an entire value of [Message.content],
/// [Stream.renderedDescription], or other text from a Zulip server that comes
/// in the same Zulip HTML format.
class ZulipContent extends ContentNode {
class ZulipContent extends ContentNode implements ZulipMessageContent {
const ZulipContent({super.debugHtmlNode, required this.nodes});

final List<BlockContentNode> nodes;
Expand Down
6 changes: 2 additions & 4 deletions lib/model/message.dart
Original file line number Diff line number Diff line change
Expand Up @@ -331,10 +331,8 @@ class MessageStoreImpl with MessageStore {
return;
}

// Live-updates for polls should not rebuild the message lists.
// [Poll] is responsible for notifying the affected listeners.
poll.handleSubmessageEvent(event);

for (final view in _messageListViews) {
view.notifyListenersIfMessagePresent(event.messageId);
}
}
}
20 changes: 13 additions & 7 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class MessageListDateSeparatorItem extends MessageListItem {
/// A message to show in the message list.
class MessageListMessageItem extends MessageListItem {
final Message message;
ZulipContent content;
ZulipMessageContent content;
bool showSender;
bool isLastInBlock;

Expand Down Expand Up @@ -98,7 +98,7 @@ mixin _MessageSequence {
///
/// This information is completely derived from [messages].
/// It exists as an optimization, to memoize the work of parsing.
final List<ZulipContent> contents = [];
final List<ZulipMessageContent> contents = [];

/// The messages and their siblings in the UI, in order.
///
Expand Down Expand Up @@ -134,10 +134,16 @@ mixin _MessageSequence {
}
}

ZulipMessageContent _parseMessageContent(Message message) {
final poll = message.poll;
if (poll != null) return PollContent(poll);
Comment on lines +138 to +139
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic should get a test.

I think the way to do that is

  • update checkInvariants as discussed in comments below
  • then add a new small group of test cases in test/model/message_list_test.dart, exercising this logic. The checks will largely be covered by checkInvariants.

return parseContent(message.content);
}

/// Update data derived from the content of the index-th message.
void _reparseContent(int index) {
final message = messages[index];
final content = parseContent(message.content);
final content = _parseMessageContent(message);
contents[index] = content;

final itemIndex = findItemWithMessageId(message.id);
Expand All @@ -154,7 +160,7 @@ mixin _MessageSequence {
void _addMessage(Message message) {
assert(contents.length == messages.length);
messages.add(message);
contents.add(parseContent(message.content));
contents.add(_parseMessageContent(message));
assert(contents.length == messages.length);
_processMessage(messages.length - 1);
}
Expand Down Expand Up @@ -197,7 +203,7 @@ mixin _MessageSequence {
/// If none of [messageIds] are found, this is a no-op.
bool _removeMessagesById(Iterable<int> messageIds) {
final messagesToRemoveById = <int>{};
final contentToRemove = Set<ZulipContent>.identity();
final contentToRemove = Set<ZulipMessageContent>.identity();
for (final messageId in messageIds) {
final index = _findMessageWithId(messageId);
if (index == -1) continue;
Expand All @@ -223,7 +229,7 @@ mixin _MessageSequence {
assert(contents.length == messages.length);
messages.insertAll(index, toInsert);
contents.insertAll(index, toInsert.map(
(message) => parseContent(message.content)));
(message) => _parseMessageContent(message)));
assert(contents.length == messages.length);
_reprocessAll();
}
Expand All @@ -243,7 +249,7 @@ mixin _MessageSequence {
void _recompute() {
assert(contents.length == messages.length);
contents.clear();
contents.addAll(messages.map((message) => parseContent(message.content)));
contents.addAll(messages.map((message) => _parseMessageContent(message)));
assert(contents.length == messages.length);
_reprocessAll();
}
Expand Down
37 changes: 35 additions & 2 deletions lib/widgets/content.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import 'dialog.dart';
import 'icons.dart';
import 'lightbox.dart';
import 'message_list.dart';
import 'poll.dart';
import 'store.dart';
import 'text.dart';

Expand All @@ -41,6 +42,10 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
colorGlobalTimeBorder: const HSLColor.fromAHSL(1, 0, 0, 0.8).toColor(),
colorMathBlockBorder: const HSLColor.fromAHSL(0.15, 240, 0.8, 0.5).toColor(),
colorMessageMediaContainerBackground: const Color.fromRGBO(0, 0, 0, 0.03),
colorPollNames: const HSLColor.fromAHSL(1, 0, 0, .45).toColor(),
colorPollVoteCountBackground: const HSLColor.fromAHSL(1, 0, 0, 1).toColor(),
colorPollVoteCountBorder: const HSLColor.fromAHSL(1, 156, 0.28, 0.7).toColor(),
colorPollVoteCountText: const HSLColor.fromAHSL(1, 156, 0.41, 0.4).toColor(),
colorThematicBreak: const HSLColor.fromAHSL(1, 0, 0, .87).toColor(),
textStylePlainParagraph: _plainParagraphCommon(context).copyWith(
color: const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor(),
Expand All @@ -66,6 +71,10 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
colorGlobalTimeBorder: const HSLColor.fromAHSL(0.4, 0, 0, 0).toColor(),
colorMathBlockBorder: const HSLColor.fromAHSL(1, 240, 0.4, 0.4).toColor(),
colorMessageMediaContainerBackground: const HSLColor.fromAHSL(0.03, 0, 0, 1).toColor(),
colorPollNames: const HSLColor.fromAHSL(1, 236, .15, .7).toColor(),
colorPollVoteCountBackground: const HSLColor.fromAHSL(0.2, 0, 0, 0).toColor(),
colorPollVoteCountBorder: const HSLColor.fromAHSL(1, 185, 0.35, 0.35).toColor(),
colorPollVoteCountText: const HSLColor.fromAHSL(1, 185, 0.35, 0.65).toColor(),
colorThematicBreak: const HSLColor.fromAHSL(1, 0, 0, .87).toColor().withValues(alpha: 0.2),
textStylePlainParagraph: _plainParagraphCommon(context).copyWith(
color: const HSLColor.fromAHSL(1, 0, 0, 0.85).toColor(),
Expand All @@ -90,6 +99,10 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
required this.colorGlobalTimeBorder,
required this.colorMathBlockBorder,
required this.colorMessageMediaContainerBackground,
required this.colorPollNames,
required this.colorPollVoteCountBackground,
required this.colorPollVoteCountBorder,
required this.colorPollVoteCountText,
required this.colorThematicBreak,
required this.textStylePlainParagraph,
required this.codeBlockTextStyles,
Expand All @@ -115,6 +128,10 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
final Color colorGlobalTimeBorder;
final Color colorMathBlockBorder; // TODO(#46) this won't be needed
final Color colorMessageMediaContainerBackground;
final Color colorPollNames;
final Color colorPollVoteCountBackground;
final Color colorPollVoteCountBorder;
final Color colorPollVoteCountText;
final Color colorThematicBreak;

/// The complete [TextStyle] we use for plain, unstyled paragraphs.
Expand Down Expand Up @@ -166,6 +183,10 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
Color? colorGlobalTimeBorder,
Color? colorMathBlockBorder,
Color? colorMessageMediaContainerBackground,
Color? colorPollNames,
Color? colorPollVoteCountBackground,
Color? colorPollVoteCountBorder,
Color? colorPollVoteCountText,
Color? colorThematicBreak,
TextStyle? textStylePlainParagraph,
CodeBlockTextStyles? codeBlockTextStyles,
Expand All @@ -181,6 +202,10 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
colorGlobalTimeBorder: colorGlobalTimeBorder ?? this.colorGlobalTimeBorder,
colorMathBlockBorder: colorMathBlockBorder ?? this.colorMathBlockBorder,
colorMessageMediaContainerBackground: colorMessageMediaContainerBackground ?? this.colorMessageMediaContainerBackground,
colorPollNames: colorPollNames ?? this.colorPollNames,
colorPollVoteCountBackground: colorPollVoteCountBackground ?? this.colorPollVoteCountBackground,
colorPollVoteCountBorder: colorPollVoteCountBorder ?? this.colorPollVoteCountBorder,
colorPollVoteCountText: colorPollVoteCountText ?? this.colorPollVoteCountText,
colorThematicBreak: colorThematicBreak ?? this.colorThematicBreak,
textStylePlainParagraph: textStylePlainParagraph ?? this.textStylePlainParagraph,
codeBlockTextStyles: codeBlockTextStyles ?? this.codeBlockTextStyles,
Expand All @@ -203,6 +228,10 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
colorGlobalTimeBorder: Color.lerp(colorGlobalTimeBorder, other.colorGlobalTimeBorder, t)!,
colorMathBlockBorder: Color.lerp(colorMathBlockBorder, other.colorMathBlockBorder, t)!,
colorMessageMediaContainerBackground: Color.lerp(colorMessageMediaContainerBackground, other.colorMessageMediaContainerBackground, t)!,
colorPollNames: Color.lerp(colorPollNames, other.colorPollNames, t)!,
colorPollVoteCountBackground: Color.lerp(colorPollVoteCountBackground, other.colorPollVoteCountBackground, t)!,
colorPollVoteCountBorder: Color.lerp(colorPollVoteCountBorder, other.colorPollVoteCountBorder, t)!,
colorPollVoteCountText: Color.lerp(colorPollVoteCountText, other.colorPollVoteCountText, t)!,
colorThematicBreak: Color.lerp(colorThematicBreak, other.colorThematicBreak, t)!,
textStylePlainParagraph: TextStyle.lerp(textStylePlainParagraph, other.textStylePlainParagraph, t)!,
codeBlockTextStyles: CodeBlockTextStyles.lerp(codeBlockTextStyles, other.codeBlockTextStyles, t),
Expand All @@ -225,14 +254,18 @@ class MessageContent extends StatelessWidget {
const MessageContent({super.key, required this.message, required this.content});

final Message message;
final ZulipContent content;
final ZulipMessageContent content;

@override
Widget build(BuildContext context) {
final content = this.content;
return InheritedMessage(message: message,
child: DefaultTextStyle(
style: ContentTheme.of(context).textStylePlainParagraph,
child: BlockContentList(nodes: content.nodes)));
child: switch (content) {
ZulipContent() => BlockContentList(nodes: content.nodes),
PollContent() => PollWidget(poll: content.poll),
}));
}
}

Expand Down
116 changes: 116 additions & 0 deletions lib/widgets/poll.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
import 'package:flutter/material.dart';
import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';

import '../api/model/submessage.dart';
import 'content.dart';
import 'store.dart';
import 'text.dart';

class PollWidget extends StatefulWidget {
const PollWidget({super.key, required this.poll});

final Poll poll;

@override
State<PollWidget> createState() => _PollWidgetState();
}

class _PollWidgetState extends State<PollWidget> {
@override
void initState() {
super.initState();
widget.poll.addListener(_modelChanged);
}

@override
void didUpdateWidget(covariant PollWidget oldWidget) {
super.didUpdateWidget(oldWidget);
if (widget.poll != oldWidget.poll) {
oldWidget.poll.removeListener(_modelChanged);
widget.poll.addListener(_modelChanged);
}
}

@override
void dispose() {
widget.poll.removeListener(_modelChanged);
super.dispose();
}

void _modelChanged() {
setState(() {
// The actual state lives in the [Poll] model.
// This method was called because that just changed.
});
}

@override
Widget build(BuildContext context) {
final zulipLocalizations = ZulipLocalizations.of(context);
final theme = ContentTheme.of(context);
final store = PerAccountStoreWidget.of(context);

final textStyleBold = const TextStyle(fontSize: 18)
.merge(weightVariableTextStyle(context, wght: 600));
final textStyleVoterNames = TextStyle(
fontSize: 16, color: theme.colorPollNames);

Text question = (widget.poll.question.isNotEmpty)
? Text(widget.poll.question, style: textStyleBold)
: Text(zulipLocalizations.pollWidgetQuestionMissing,
style: textStyleBold.copyWith(fontStyle: FontStyle.italic));

Widget buildOptionItem(PollOption option) {
// TODO(i18n): List formatting, like you can do in JavaScript:
// new Intl.ListFormat('ja').format(['Chris', 'Greg', 'Alya', 'Zixuan'])
// // 'Chris、Greg、Alya、Zixuan'
final voterNames = option.voters
.map((userId) =>
store.users[userId]?.fullName ?? zulipLocalizations.unknownUserName)
.join(', ');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indented this in a style taken from ListNode.debugDescribeChildren, to avoid going over 80 characters.

  @override
  List<DiagnosticsNode> debugDescribeChildren() {
    return items
      .mapIndexed((i, nodes) =>
        _BlockContentListNode(nodes).toDiagnosticsNode(name: 'item $i'))
      .toList();
  }


return Padding(
padding: const EdgeInsets.only(bottom: 5),
child: Row(
spacing: 5,
crossAxisAlignment: CrossAxisAlignment.baseline,
textBaseline: localizedTextBaseline(context),
children: [
ConstrainedBox(
constraints: const BoxConstraints(minWidth: 25),
child: Container(
height: 25,
padding: const EdgeInsets.symmetric(horizontal: 4),
decoration: BoxDecoration(
color: theme.colorPollVoteCountBackground,
border: Border.all(color: theme.colorPollVoteCountBorder),
borderRadius: BorderRadius.circular(3)),
child: Center(
child: Text(option.voters.length.toString(),
textAlign: TextAlign.center,
style: textStyleBold.copyWith(
color: theme.colorPollVoteCountText, fontSize: 13))))),
Expanded(
child: Wrap(
spacing: 5,
children: [
Text(option.text, style: textStyleBold.copyWith(fontSize: 16)),
if (option.voters.isNotEmpty)
// TODO(i18n): Localize parenthesis characters.
Text('($voterNames)', style: textStyleVoterNames),
])),
]));
}

return Column(
crossAxisAlignment: CrossAxisAlignment.start,
children: [
Padding(padding: const EdgeInsets.only(bottom: 6), child: question),
if (widget.poll.options.isEmpty)
Text(zulipLocalizations.pollWidgetOptionsMissing,
style: textStyleVoterNames.copyWith(fontStyle: FontStyle.italic)),
for (final option in widget.poll.options)
buildOptionItem(option),
]);
}
}
6 changes: 6 additions & 0 deletions test/model/content_checks.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import 'package:checks/checks.dart';
import 'package:checks/context.dart';
import 'package:flutter/foundation.dart';
import 'package:zulip/api/model/submessage.dart';
import 'package:zulip/model/content.dart';

extension ContentNodeChecks on Subject<ContentNode> {
Expand Down Expand Up @@ -115,3 +117,7 @@ Iterable<LinkNode> _findLinkNodes(Iterable<InlineContentNode> nodes) {
return _findLinkNodes(node.nodes);
});
}

extension PollContentChecks on Subject<PollContent> {
Subject<Poll> get poll => has((x) => x.poll, 'poll');
}
Loading