Skip to content

Show marked/edited markers, without swipe gesture #762

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

Merged
merged 4 commits into from
Jul 17, 2024
Merged

Show marked/edited markers, without swipe gesture #762

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
f42c822
icons [nfc]: Add edited/moved marker icons.
PIG208 Jun 24, 2024
e72f240
theme [nfc]: Extract starColor to DesignVariables.
PIG208 Jun 24, 2024
6e8bfba
msglist: Fix star icon alignment issue.
PIG208 Jul 5, 2024
42d53c1
ui: Add edited/moved marker.
PIG208 Jun 24, 2024
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
Binary file modified assets/icons/ZulipIcons.ttf
View file
Open in desktop
Binary file not shown.
3 changes: 3 additions & 0 deletions assets/icons/edited.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions assets/icons/message_moved.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
74 changes: 74 additions & 0 deletions lib/widgets/edit_state_marker.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import 'package:flutter/material.dart';

import '../api/model/model.dart';
import 'icons.dart';
import 'text.dart';
import 'theme.dart';

class EditStateMarker extends StatelessWidget {
const EditStateMarker({
super.key,
required this.editState,
required this.children,
});

final MessageEditState editState;
final List<Widget> children;

@override
Widget build(BuildContext context) {
final hasMarker = editState != MessageEditState.none;

return Row(
crossAxisAlignment: CrossAxisAlignment.baseline,
textBaseline: localizedTextBaseline(context),
children: [
hasMarker
? _EditStateMarkerPill(editState: editState)
: const SizedBox(width: _EditStateMarkerPill.widthCollapsed),
...children,
],
);
}
}

class _EditStateMarkerPill extends StatelessWidget {
const _EditStateMarkerPill({required this.editState});

final MessageEditState editState;

/// The minimum width of the marker.
// Currently, only the collapsed state of the marker has been implemented,
// where only the marker icon, not the marker text, is visible.
static const double widthCollapsed = 16;

@override
Widget build(BuildContext context) {
final designVariables = DesignVariables.of(context);

final IconData icon;
final Offset offset;
switch (editState) {
case MessageEditState.none:
assert(false);
return const SizedBox(width: widthCollapsed);
case MessageEditState.edited:
icon = ZulipIcons.edited;
// These offsets are chosen ad hoc, but give a good vertical alignment
// of the icons with the first line of the message, when the message
// begins with a paragraph, at default text scaling. See:
// https://github.com/zulip/zulip-flutter/pull/762#issuecomment-2232041922
offset = const Offset(0, 2);
case MessageEditState.moved:
icon = ZulipIcons.message_moved;
offset = const Offset(0, 3);
}

return ConstrainedBox(
constraints: const BoxConstraints(maxWidth: widthCollapsed),
child: Transform.translate(
offset: offset,
child: Icon(
icon, size: 16, color: designVariables.editedMovedMarkerCollapsed)));
}
}
28 changes: 17 additions & 11 deletions lib/widgets/icons.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,38 +42,44 @@ abstract final class ZulipIcons {
/// The Zulip custom icon "clock".
static const IconData clock = IconData(0xf106, fontFamily: "Zulip Icons");

/// The Zulip custom icon "edited".
static const IconData edited = IconData(0xf107, fontFamily: "Zulip Icons");

/// The Zulip custom icon "globe".
static const IconData globe = IconData(0xf107, fontFamily: "Zulip Icons");
static const IconData globe = IconData(0xf108, fontFamily: "Zulip Icons");

/// The Zulip custom icon "group_dm".
static const IconData group_dm = IconData(0xf108, fontFamily: "Zulip Icons");
static const IconData group_dm = IconData(0xf109, fontFamily: "Zulip Icons");

/// The Zulip custom icon "hash_sign".
static const IconData hash_sign = IconData(0xf109, fontFamily: "Zulip Icons");
static const IconData hash_sign = IconData(0xf10a, fontFamily: "Zulip Icons");

/// The Zulip custom icon "language".
static const IconData language = IconData(0xf10a, fontFamily: "Zulip Icons");
static const IconData language = IconData(0xf10b, fontFamily: "Zulip Icons");

/// The Zulip custom icon "lock".
static const IconData lock = IconData(0xf10b, fontFamily: "Zulip Icons");
static const IconData lock = IconData(0xf10c, fontFamily: "Zulip Icons");

/// The Zulip custom icon "message_moved".
static const IconData message_moved = IconData(0xf10d, fontFamily: "Zulip Icons");

/// The Zulip custom icon "mute".
static const IconData mute = IconData(0xf10c, fontFamily: "Zulip Icons");
static const IconData mute = IconData(0xf10e, fontFamily: "Zulip Icons");

/// The Zulip custom icon "read_receipts".
static const IconData read_receipts = IconData(0xf10d, fontFamily: "Zulip Icons");
static const IconData read_receipts = IconData(0xf10f, fontFamily: "Zulip Icons");

/// The Zulip custom icon "star_filled".
static const IconData star_filled = IconData(0xf10e, fontFamily: "Zulip Icons");
static const IconData star_filled = IconData(0xf110, fontFamily: "Zulip Icons");

/// The Zulip custom icon "topic".
static const IconData topic = IconData(0xf10f, fontFamily: "Zulip Icons");
static const IconData topic = IconData(0xf111, fontFamily: "Zulip Icons");

/// The Zulip custom icon "unmute".
static const IconData unmute = IconData(0xf110, fontFamily: "Zulip Icons");
static const IconData unmute = IconData(0xf112, fontFamily: "Zulip Icons");

/// The Zulip custom icon "user".
static const IconData user = IconData(0xf111, fontFamily: "Zulip Icons");
static const IconData user = IconData(0xf113, fontFamily: "Zulip Icons");

// END GENERATED ICON DATA
}
Expand Down
27 changes: 11 additions & 16 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import 'page.dart';
import 'profile.dart';
import 'sticky_header.dart';
import 'store.dart';
import 'edit_state_marker.dart';
import 'text.dart';
import 'theme.dart';

Expand Down Expand Up @@ -896,12 +897,10 @@ class MessageWithPossibleSender extends StatelessWidget {

final MessageListMessageItem item;

// TODO(#95) unchanged in dark theme?
static final _starColor = const HSLColor.fromAHSL(0.5, 47, 1, 0.41).toColor();

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

final message = item.message;
final sender = store.users[message.senderId];
Expand Down Expand Up @@ -964,25 +963,21 @@ class MessageWithPossibleSender extends StatelessWidget {
if (senderRow != null)
Padding(padding: const EdgeInsets.fromLTRB(16, 2, 16, 0),
child: senderRow),
Row(crossAxisAlignment: CrossAxisAlignment.start, children: [
Copy link
Member

Choose a reason for hiding this comment

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

msglist: Fix star icon alignment issue.

By aligning the row using `textBaseline`, we no longer need to apply
the padding to the star.

Neat!

const SizedBox(width: 16),
Expanded(
child: Column(
EditStateMarker(
editState: message.editState,
children: [
Expanded(child: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
children: [
MessageContent(message: message, content: item.content),
if ((message.reactions?.total ?? 0) > 0)
ReactionChipsList(messageId: message.id, reactions: message.reactions!)
])),
SizedBox(width: 16,
child: message.flags.contains(MessageFlag.starred)
// TODO(#157): fix how star marker aligns with message content
// Design from Figma at:
// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=813%3A28817&mode=dev .
? Padding(padding: const EdgeInsets.only(top: 4),
child: Icon(ZulipIcons.star_filled, size: 16, color: _starColor))
: null),
]),
SizedBox(width: 16,
child: message.flags.contains(MessageFlag.starred)
? Icon(ZulipIcons.star_filled, size: 16, color: designVariables.star)
: null),
]),
])));
}
}
Expand Down
18 changes: 18 additions & 0 deletions lib/widgets/theme.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
mainBackground: const Color(0xfff0f0f0),
title: const Color(0xff1a1a1a),
streamColorSwatches: StreamColorSwatches.light,
star: const HSLColor.fromAHSL(0.5, 47, 1, 0.41).toColor(),
editedMovedMarkerCollapsed: const Color.fromARGB(128, 146, 167, 182),
);

DesignVariables.dark() :
Expand All @@ -133,6 +135,10 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
mainBackground: const Color(0xff1d1d1d),
title: const Color(0xffffffff),
streamColorSwatches: StreamColorSwatches.dark,
// TODO(#95) unchanged in dark theme?
star: const HSLColor.fromAHSL(0.5, 47, 1, 0.41).toColor(),
// TODO(#95) need dark-theme color
editedMovedMarkerCollapsed: const Color.fromARGB(128, 146, 167, 182),
);

DesignVariables._({
Expand All @@ -142,6 +148,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
required this.mainBackground,
required this.title,
required this.streamColorSwatches,
required this.star,
required this.editedMovedMarkerCollapsed,
});

/// The [DesignVariables] from the context's active theme.
Expand All @@ -163,6 +171,10 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
// Not exactly from the Figma design, but from Vlad anyway.
final StreamColorSwatches streamColorSwatches;

// Not named variables in Figma; taken from older Figma drafts, or elsewhere.
final Color star;
final Color editedMovedMarkerCollapsed;

@override
DesignVariables copyWith({
Color? bgTopBar,
Expand All @@ -171,6 +183,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
Color? mainBackground,
Color? title,
StreamColorSwatches? streamColorSwatches,
Color? star,
Color? editedMovedMarkerCollapsed,
}) {
return DesignVariables._(
bgTopBar: bgTopBar ?? this.bgTopBar,
Expand All @@ -179,6 +193,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
mainBackground: mainBackground ?? this.mainBackground,
title: title ?? this.title,
streamColorSwatches: streamColorSwatches ?? this.streamColorSwatches,
star: star ?? this.star,
editedMovedMarkerCollapsed: editedMovedMarkerCollapsed ?? this.editedMovedMarkerCollapsed,
);
}

Expand All @@ -194,6 +210,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
mainBackground: Color.lerp(mainBackground, other.mainBackground, t)!,
title: Color.lerp(title, other.title, t)!,
streamColorSwatches: StreamColorSwatches.lerp(streamColorSwatches, other.streamColorSwatches, t),
star: Color.lerp(star, other.star, t)!,
editedMovedMarkerCollapsed: Color.lerp(editedMovedMarkerCollapsed, other.editedMovedMarkerCollapsed, t)!,
);
}
}
Expand Down
94 changes: 94 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 @@ -16,6 +16,7 @@ import 'package:zulip/model/localizations.dart';
import 'package:zulip/model/narrow.dart';
import 'package:zulip/model/store.dart';
import 'package:zulip/widgets/content.dart';
import 'package:zulip/widgets/emoji_reaction.dart';
import 'package:zulip/widgets/icons.dart';
import 'package:zulip/widgets/message_list.dart';
import 'package:zulip/widgets/store.dart';
Expand Down Expand Up @@ -576,6 +577,99 @@ void main() {
});
});

group('EditStateMarker', () {
void checkMarkersCount({required int edited, required int moved}) {
check(find.byIcon(ZulipIcons.edited).evaluate()).length.equals(edited);
check(find.byIcon(ZulipIcons.message_moved).evaluate()).length.equals(moved);
}

testWidgets('no edited or moved messages', (WidgetTester tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

(The explicit type here is fine, but FWIW we most often leave it out in this context — it gets inferred just fine from the testWidgets call, and it's quite predictable for a human reader too.

OTOH in the upstream Flutter tree you'll see these types always, because they use a lint rule to always have explicit types.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I added those in the previous revision because I saw we doing that in the surrounding test cases.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, matching the surrounding code is a good instinct.

Like I said, having the explicit type here is pretty harmless. But maybe I'll do a quick sweep to remove them for testWidgets calls throughout our tests, just so it's uniformly a bit simpler. (From a quick scan, about 1/3 of our testWidgets calls have them.)

final message = eg.streamMessage();
await setupMessageListPage(tester, messages: [message]);
checkMarkersCount(edited: 0, moved: 0);
});

testWidgets('edited and moved messages from events', (WidgetTester tester) async {
final message = eg.streamMessage();
final message2 = eg.streamMessage();
await setupMessageListPage(tester, messages: [message, message2]);
checkMarkersCount(edited: 0, moved: 0);

await store.handleEvent(eg.updateMessageEditEvent(message, renderedContent: 'edited'));
await tester.pump();
checkMarkersCount(edited: 1, moved: 0);

await store.handleEvent(eg.updateMessageMoveEvent(
[message, message2], origTopic: 'old', newTopic: 'new'));
await tester.pump();
checkMarkersCount(edited: 1, moved: 1);

await store.handleEvent(eg.updateMessageEditEvent(message2, renderedContent: 'edited'));
await tester.pump();
checkMarkersCount(edited: 2, moved: 0);
});

List<List<(String, Rect)>> captureMessageRects(
WidgetTester tester,
List<Message> messages,
Message targetMessage,
) {
assert(messages.contains(targetMessage));
final List<List<(String, Rect)>> result = [];
for (final message in messages) {
final baseFinder = find.byWidgetPredicate(
(widget) => widget is MessageWithPossibleSender
&& widget.item.message.id == message.id);

Rect getRectMatching(Finder matching) {
final finder = find.descendant(
of: baseFinder, matching: matching)..tryEvaluate();
check(finder.found, because: '${message.content}: $matching')
.length.equals(1);
return tester.getRect(finder.first);
}

result.add([
('MessageWithPossibleSender', tester.getRect(baseFinder)),
('MessageContent', getRectMatching(find.byType(MessageContent))),
('Paragraph', getRectMatching(find.byType(Paragraph))),
if (message.id == targetMessage.id) ...[
('Star', getRectMatching(find.byIcon(ZulipIcons.star_filled))),
('ReactionChipsList', getRectMatching(find.byType(ReactionChipsList))),
],
]);
}
return result;
}

testWidgets('edit state updates do not affect layout', (WidgetTester tester) async {
final messages = [
eg.streamMessage(),
eg.streamMessage(
reactions: [eg.unicodeEmojiReaction, eg.realmEmojiReaction],
flags: [MessageFlag.starred]),
eg.streamMessage(),
];
final StreamMessage messageWithMarker = messages[1];
await setupMessageListPage(tester, messages: messages);
final rectsBefore = captureMessageRects(tester, messages, messageWithMarker);
checkMarkersCount(edited: 0, moved: 0);

await store.handleEvent(eg.updateMessageMoveEvent(
[messageWithMarker], origTopic: 'old', newTopic: messageWithMarker.topic));
await tester.pump();
check(captureMessageRects(tester, messages, messageWithMarker))
.deepEquals(rectsBefore);
checkMarkersCount(edited: 0, moved: 1);

await store.handleEvent(eg.updateMessageEditEvent(messageWithMarker));
await tester.pump();
check(captureMessageRects(tester, messages, messageWithMarker))
.deepEquals(rectsBefore);
checkMarkersCount(edited: 1, moved: 0);
});
});

group('_UnreadMarker animations', () {
// TODO: Improve animation state testing so it is less tied to
// implementation details and more focused on output, see:
Expand Down