Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Unread markers #317

Merged
merged 4 commits into from
Oct 31, 2023
Merged

Unread markers #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e16bcb
msglist: Go edge-to-edge, for a mobile-oriented design
sirpengi Oct 20, 2023
50d5ea3
msglist: Add animated marker for unread messages
sirpengi Oct 9, 2023
e80db81
msglist [nfc]: Make _MessageSequence.findItemWithMessageId non-private
sirpengi Oct 25, 2023
33dcbf9
msglist: Maintain _UnreadMarker animation state when MessageListView.…
sirpengi Oct 25, 2023
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
4 changes: 2 additions & 2 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ mixin _MessageSequence {
(message, messageId) => message.id.compareTo(messageId));
}

int _findItemWithMessageId(int messageId) {
int findItemWithMessageId(int messageId) {
return binarySearchByKey(items, messageId, _compareItemToMessageId);
}

Expand All @@ -129,7 +129,7 @@ mixin _MessageSequence {
final content = parseContent(message.content);
contents[index] = content;

final itemIndex = _findItemWithMessageId(message.id);
final itemIndex = findItemWithMessageId(message.id);
assert(itemIndex > -1
&& items[itemIndex] is MessageListMessageItem
&& identical((items[itemIndex] as MessageListMessageItem).message, message));
Expand Down
122 changes: 67 additions & 55 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,6 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
color: Colors.white,
// Pad the left and right insets, for small devices in landscape.
child: SafeArea(
// Keep some padding when there are no horizontal insets,
// which is usual in portrait mode.
minimum: const EdgeInsets.symmetric(horizontal: 8),
child: Center(
child: ConstrainedBox(
constraints: const BoxConstraints(maxWidth: 760),
Expand Down Expand Up @@ -258,6 +255,27 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
_ => ScrollViewKeyboardDismissBehavior.manual,
},

// To preserve state across rebuilds for individual [MessageItem]
// widgets as the size of [MessageListView.items] changes we need
// to match old widgets by their key to their new position in
// the list.
//
// The keys are of type [ValueKey] with a value of [Message.id]
// and here we use a O(log n) binary search method. This could
// be improved but for now it only triggers for materialized
// widgets. As a simple test, flinging through All Messages in
// CZO on a Pixel 5, this only runs about 10 times per rebuild
// and the timing for each call is <100 microseconds.
//
// Non-message items (e.g., start and end markers) that do not
// have state that needs to be preserved have not been given keys
// and will not trigger this callback.
findChildIndexCallback: (Key key) {
final valueKey = key as ValueKey;
final index = model!.findItemWithMessageId(valueKey.value);
if (index == -1) return null;
return length - 1 - index;
},
controller: scrollController,
itemCount: length,
// Setting reverse: true means the scroll starts at the bottom.
Expand Down Expand Up @@ -286,6 +304,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
header: header, child: header);
case MessageListMessageItem():
return MessageItem(
key: ValueKey(data.message.id),
Copy link
Member

Choose a reason for hiding this comment

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

This now appears in a different commit from findChildIndexCallback. The two of them work in tandem and make most sense to understand together, so they should appear in the same commit.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I see, this commit uses it in tests.

As I mentioned in chat, I'd be hesitant to add keys just for the sake of tests. Here it's OK because we'll be using it in the app code just a few commits later; so we should just make that clear in the commit message. Can add something like "Also add a key to MessageItem widgets in the message list; we'll want those in an upcoming commit that passes findChildIndexCallback, so we just bring them forward to here for convenience in tests."

trailing: i == 0 ? const SizedBox(height: 8) : const SizedBox(height: 11),
item: data);
}
Expand Down Expand Up @@ -354,62 +373,55 @@ class MessageItem extends StatelessWidget {

@override
Widget build(BuildContext context) {
final store = PerAccountStoreWidget.of(context);
final message = item.message;

Color highlightBorderColor;
Color restBorderColor;
if (message is StreamMessage) {
final subscription = store.subscriptions[message.streamId];
highlightBorderColor = colorForStream(subscription);
restBorderColor = _kStreamMessageBorderColor;
} else if (message is DmMessage) {
highlightBorderColor = _kDmRecipientHeaderColor;
restBorderColor = _kDmRecipientHeaderColor;
} else {
throw Exception("impossible message type: ${message.runtimeType}");
}

// This 3px border seems to accurately reproduce something much more
// complicated on web, involving CSS box-shadow; see comment below.
final recipientBorder = BorderSide(color: highlightBorderColor, width: 3);
final restBorder = BorderSide(color: restBorderColor, width: 1);
Comment on lines -375 to -376
Copy link
Member

Choose a reason for hiding this comment

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

msglist: Remove borders around messages and adjust padding

The original implementation here tried to match the
design of the webapp as it existed at the time.
Since then, the design of web has moved on and

This part is true, but…

the design of mobile is expected to continue on the
same direction of having messages in the chat
expand from screen edge to screen edge.

it's not really connected to this part, nor to what this commit (or PR) is doing.

In particular the design on web still has borders around the messages. If we were following the updated web design, we'd have borders, and we'd have a gray background for the overall message list. (That background would show through between one block of messages and the next, and to the left and right of the blocks of messages.)

Relatedly, having the messages extend from edge to edge of the screen isn't something where mobile will be following in a direction set by web. It's something that zulip-mobile has been doing since forever (because it makes sense for a mobile environment with a narrow screen), and that web doesn't do and has no plans to do.

What's happening when we remove the borders and extend from edge to edge is that we're moving from a design based on web — and yes it was based on the pre-2023 design of web, but this aspect would be exactly the same based on web's design today — to a mobile-specific design. And we're doing so by taking a design element found in the zulip-mobile RN app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I meant what you outlined above, my statement of "the design of web has moved on" meant "the design the original implementation tried to match isn't even the current design of the web" and on top of that mobile had it's own separate design and is expected to stay separate (and so here again we're not sticking with web but following the mobile design evolution). I do think the language was overly verbose, so I've edited it down and hopefully added clarity to the point being made.

Copy link
Member

Choose a reason for hiding this comment

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

msglist: Messages changed to render edge-to-edge and borderless

This is a small change in line with #157 and
moves the implementation closer to the new 2023+
design for mobile.
Messages now span from screen edge to screen edge
and borders around groups of messages (in particular
the left-side recipient border) was removed.

This is much closer, but the first sentence is still inaccurate. The change this is making is in a direction completely orthogonal to #157 or to any new design for mobile — it's about moving from a design based on web to a mobile-specific design. The key design features here are exactly what zulip-mobile has had for many years.

In the summary line, a nit: verb should be in the imperative (or infinitive): https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-summary-part-2

Here's a revised version:

msglist: Go edge-to-edge, for a mobile-oriented design

The existing design here was designed for web; the prototype
copied precisely from web's design as of 2022-12.

But in the zulip-mobile RN app, we've long had a design that's
optimized for a narrow mobile screen instead: notably by using the
full available width (up to a maximum, anyway), with no left or right
borders, and consequently less padding too.  Do that here.

var borderDecoration = ShapeDecoration(
// Web actually uses, for stream messages, a slightly lighter border at
// right than at bottom and in the recipient header: black 10% alpha,
// vs. 88% lightness. Assume that's an accident.
shape: Border(
left: recipientBorder,
right: restBorder,
bottom: item.isLastInBlock ? restBorder : BorderSide.none,
));

return StickyHeaderItem(
allowOverflow: !item.isLastInBlock,
header: RecipientHeader(message: message),
child: Column(children: [
DecoratedBox(
decoration: borderDecoration,
child: MessageWithPossibleSender(item: item)),
if (trailing != null && item.isLastInBlock) trailing!,
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? Seems like this will smush the message blocks much closer to each other (11px).

Probably it's best to split out this and the other padding changes into their own commit, if possible. That will give them their own commit message, making space for explaining why they make sense and are desirable.

Or even two commits — one for the horizontal padding and one for the vertical — if those don't have the same reasoning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll back out this change, and open a discussion on it in CZO. The figma designs don't have any other padding defined but I agree that it's better to have some space, especially after a message that is right before a new topic header.

]));

// Web handles the left-side recipient marker in a funky way:
// box-shadow: inset 3px 0px 0px -1px #c2726a, -1px 0px 0px 0px #c2726a;
// (where the color is the stream color.) That is, it's a pair of
// box shadows. One of them is inset.
//
// At attempt at a literal translation might look like this:
//
// DecoratedBox(
// decoration: ShapeDecoration(shadows: [
// BoxShadow(offset: Offset(3, 0), spreadRadius: -1, color: highlightBorderColor),
// BoxShadow(offset: Offset(-1, 0), color: highlightBorderColor),
// ], shape: Border.fromBorderSide(BorderSide.none)),
// child: MessageWithSender(message: message)),
//
// But CSS `box-shadow` seems to not apply under the item itself, while
// Flutter's BoxShadow does.
child: _UnreadMarker(
isRead: message.flags.contains(MessageFlag.read),
child: Column(children: [
MessageWithPossibleSender(item: item),
if (trailing != null && item.isLastInBlock) trailing!,
])));
}
}

/// Widget responsible for showing the read status of a message.
class _UnreadMarker extends StatelessWidget {
const _UnreadMarker({required this.isRead, required this.child});

final bool isRead;
final Widget child;

@override
Widget build(BuildContext context) {
return Stack(
children: [
child,
Positioned(
top: 0,
left: 0,
bottom: 0,
width: 4,
child: AnimatedOpacity(
opacity: isRead ? 0 : 1,
// Web uses 2s and 0.3s durations, and a CSS ease-out curve.
// See zulip:web/styles/message_row.css .
duration: Duration(milliseconds: isRead ? 2000 : 300),
curve: Curves.easeOut,
child: DecoratedBox(
decoration: BoxDecoration(
// The color hsl(227deg 78% 59%) comes from the Figma mockup at:
// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132-9684
// See discussion about design at:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20unread.20marker/near/1658008
color: const HSLColor.fromAHSL(1, 227, 0.78, 0.59).toColor(),
// TODO(#95): Don't show this extra border in dark mode, see:
// https://github.com/zulip/zulip-flutter/pull/317#issuecomment-1784311663
border: Border(left: BorderSide(
width: 1,
color: Colors.white.withOpacity(0.6))))))),
]);
}
}

Expand Down
5 changes: 5 additions & 0 deletions test/flutter_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ import 'package:checks/checks.dart';
import 'package:flutter/services.dart';
import 'package:flutter/widgets.dart';

extension AnimationChecks<T> on Subject<Animation<T>> {
Subject<AnimationStatus> get status => has((d) => d.status, 'status');
Subject<T> get value => has((d) => d.value, 'value');
}

extension ClipboardDataChecks on Subject<ClipboardData> {
Subject<String?> get text => has((d) => d.text, 'text');
}
Expand Down
109 changes: 109 additions & 0 deletions test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import '../example_data.dart' as eg;
import '../model/binding.dart';
import '../model/message_list_test.dart';
import '../model/test_store.dart';
import '../flutter_checks.dart';
import '../stdlib_checks.dart';
import '../test_images.dart';
import 'content_checks.dart';
Expand Down Expand Up @@ -300,4 +301,112 @@ void main() {
debugNetworkImageHttpClientProvider = null;
});
});

group('_UnreadMarker animations', () {
// TODO: Improve animation state testing so it is less tied to
// implementation details and more focused on output, see:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/robust.20widget.20finders.20in.20tests/near/1671738
Animation<double> getAnimation(WidgetTester tester, int messageId) {
final widget = tester.widget<FadeTransition>(find.descendant(
of: find.byKey(ValueKey(messageId)),
matching: find.byType(FadeTransition)));
return widget.opacity;
}

testWidgets('from read to unread', (WidgetTester tester) async {
final message = eg.streamMessage(flags: [MessageFlag.read]);
await setupMessageListPage(tester, messages: [message]);
check(getAnimation(tester, message.id))
..value.equals(0.0)
..status.equals(AnimationStatus.dismissed);

store.handleEvent(eg.updateMessageFlagsRemoveEvent(
MessageFlag.read, [message]));
await tester.pump(); // process handleEvent
check(getAnimation(tester, message.id))
..value.equals(0.0)
..status.equals(AnimationStatus.forward);

await tester.pumpAndSettle();
check(getAnimation(tester, message.id))
..value.equals(1.0)
..status.equals(AnimationStatus.completed);
});

testWidgets('from unread to read', (WidgetTester tester) async {
final message = eg.streamMessage(flags: []);
await setupMessageListPage(tester, messages: [message]);
check(getAnimation(tester, message.id))
..value.equals(1.0)
..status.equals(AnimationStatus.dismissed);
Comment on lines +339 to +341
Copy link
Member

Choose a reason for hiding this comment

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

Cool — this extra detail in fact serves as helpful color on how Animation works, how the status interacts with the value. Here the value is at 1 while the status is dismissed, whereas in the read->unread case it was at 0 when dismissed and 1 corresponded to completed.


store.handleEvent(UpdateMessageFlagsAddEvent(
id: 1,
flag: MessageFlag.read,
messages: [message.id],
all: false,
));
await tester.pump(); // process handleEvent
check(getAnimation(tester, message.id))
..value.equals(1.0)
..status.equals(AnimationStatus.forward);

await tester.pumpAndSettle();
check(getAnimation(tester, message.id))
..value.equals(0.0)
..status.equals(AnimationStatus.completed);
});

testWidgets('animation state persistence', (WidgetTester tester) async {
// Check that _UnreadMarker maintains its in-progress animation
// as the number of items changes in MessageList. See
// `findChildIndexCallback` passed into [StickyHeaderListView.builder]
// at [_MessageListState._buildListView].
final message = eg.streamMessage(flags: []);
await setupMessageListPage(tester, messages: [message]);
check(getAnimation(tester, message.id))
..value.equals(1.0)
..status.equals(AnimationStatus.dismissed);

store.handleEvent(UpdateMessageFlagsAddEvent(
id: 0,
flag: MessageFlag.read,
messages: [message.id],
all: false,
));
await tester.pump(); // process handleEvent
check(getAnimation(tester, message.id))
..value.equals(1.0)
..status.equals(AnimationStatus.forward);

// run animation partially
await tester.pump(const Duration(milliseconds: 30));
check(getAnimation(tester, message.id))
..value.isGreaterThan(0.0)
..value.isLessThan(1.0)
..status.equals(AnimationStatus.forward);
Comment on lines +381 to +387
Copy link
Member

Choose a reason for hiding this comment

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

Cool, this test is now all nice and clear, with a crisp narrative.


// introduce new message
final newMessage = eg.streamMessage(flags:[MessageFlag.read]);
store.handleEvent(MessageEvent(id: 0, message: newMessage));
await tester.pump(); // process handleEvent
check(find.byType(MessageItem).evaluate()).length.equals(2);
check(getAnimation(tester, message.id))
..value.isGreaterThan(0.0)
..value.isLessThan(1.0)
..status.equals(AnimationStatus.forward);
check(getAnimation(tester, newMessage.id))
..value.equals(0.0)
..status.equals(AnimationStatus.dismissed);

final frames = await tester.pumpAndSettle();
check(frames).isGreaterThan(1);
check(getAnimation(tester, message.id))
..value.equals(0.0)
..status.equals(AnimationStatus.completed);
check(getAnimation(tester, newMessage.id))
..value.equals(0.0)
..status.equals(AnimationStatus.dismissed);
});
});
}