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.

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

action_sheet: Offer "Mark channel as read" in channel action sheet #1317

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
Binary file modified assets/icons/ZulipIcons.ttf
Binary file not shown.
4 changes: 4 additions & 0 deletions assets/icons/message_checked.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions assets/l10n/app_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@
"@permissionsDeniedReadExternalStorage": {
"description": "Message for dialog asking the user to grant permissions for external storage read access."
},
"actionSheetOptionMarkChannelAsRead": "Mark channel as read",
"@actionSheetOptionMarkChannelAsRead": {
"description": "Label for marking a channel as read."
},
"actionSheetOptionMuteTopic": "Mute topic",
"@actionSheetOptionMuteTopic": {
"description": "Label for muting a topic on action sheet."
Expand Down
6 changes: 6 additions & 0 deletions lib/generated/l10n/zulip_localizations.dart
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,12 @@ abstract class ZulipLocalizations {
/// **'To upload files, please grant Zulip additional permissions in Settings.'**
String get permissionsDeniedReadExternalStorage;

/// Label for marking a channel as read.
///
/// In en, this message translates to:
/// **'Mark channel as read'**
String get actionSheetOptionMarkChannelAsRead;

/// Label for muting a topic on action sheet.
///
/// In en, this message translates to:
Expand Down
3 changes: 3 additions & 0 deletions lib/generated/l10n/zulip_localizations_ar.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.';

@override
String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read';

@override
String get actionSheetOptionMuteTopic => 'Mute topic';

Expand Down
3 changes: 3 additions & 0 deletions lib/generated/l10n/zulip_localizations_en.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.';

@override
String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read';

@override
String get actionSheetOptionMuteTopic => 'Mute topic';

Expand Down
3 changes: 3 additions & 0 deletions lib/generated/l10n/zulip_localizations_ja.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.';

@override
String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read';

@override
String get actionSheetOptionMuteTopic => 'Mute topic';

Expand Down
3 changes: 3 additions & 0 deletions lib/generated/l10n/zulip_localizations_nb.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ class ZulipLocalizationsNb extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.';

@override
String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read';

@override
String get actionSheetOptionMuteTopic => 'Mute topic';

Expand Down
3 changes: 3 additions & 0 deletions lib/generated/l10n/zulip_localizations_pl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ class ZulipLocalizationsPl extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'Aby odebrać pliki Zulip musi uzyskać dodatkowe uprawnienia w Ustawieniach.';

@override
String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read';

@override
String get actionSheetOptionMuteTopic => 'Wycisz wątek';

Expand Down
3 changes: 3 additions & 0 deletions lib/generated/l10n/zulip_localizations_ru.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ class ZulipLocalizationsRu extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'Для загрузки файлов, пожалуйста, предоставьте Zulip дополнительные разрешения в настройках.';

@override
String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read';

@override
String get actionSheetOptionMuteTopic => 'Отключить тему';

Expand Down
3 changes: 3 additions & 0 deletions lib/generated/l10n/zulip_localizations_sk.dart
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ class ZulipLocalizationsSk extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.';

@override
String get actionSheetOptionMarkChannelAsRead => 'Mark channel as read';

@override
String get actionSheetOptionMuteTopic => 'Stlmiť tému';

Expand Down
49 changes: 49 additions & 0 deletions lib/widgets/action_sheet.dart
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,55 @@ class ActionSheetCancelButton extends StatelessWidget {
}
}

/// Show a sheet of actions you can take on a channel.
void showChannelActionSheet(BuildContext context, {
required int channelId,
}) {
final pageContext = PageRoot.contextOf(context);
final store = PerAccountStoreWidget.of(pageContext);

final optionButtons = <ActionSheetMenuItemButton>[];
final unreadCount = store.unreads.countInChannelNarrow(channelId);
if (unreadCount > 0) {
optionButtons.add(
MarkChannelAsReadButton(pageContext: pageContext, streamId: channelId));
}
if (optionButtons.isEmpty) {
// TODO(a11y): This case makes a no-op gesture handler; as a consequence,
// we're presenting some UI (to people who use screen-reader software) as
// though it offers a gesture interaction that it doesn't meaningfully
// offer, which is confusing. The solution here is probably to remove this
// is-empty case by having at least one button that's always present,
// such as "copy link to channel".
return;
}
_showActionSheet(pageContext, optionButtons: optionButtons);
}

class MarkChannelAsReadButton extends ActionSheetMenuItemButton {
const MarkChannelAsReadButton({
super.key,
required this.streamId,
required super.pageContext
});

final int streamId;

@override
IconData get icon => ZulipIcons.message_checked;

@override
String label(ZulipLocalizations zulipLocalizations) {
return zulipLocalizations.actionSheetOptionMarkChannelAsRead;
}

@override
void onPressed() async {
final narrow = ChannelNarrow(streamId);
await ZulipAction.markNarrowAsRead(pageContext, narrow);
}
Comment on lines +208 to +212
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see; the reasoning for ZulipAction, in its dartdoc, makes sense.

I see that ZulipAction.markNarrowAsRead takes care of giving UI feedback, so I agree a try / catch isn't needed here.

It still doesn't give one specific kind of feedback I was hoping for, though 🙂:

#1317 (comment)

Let's have special handling for if e is ZulipApiException, like other buttons that make API requests

Could you make that adjustment, in a separate commit, for both values of useLegacy? For useLegacy false, that means adjusting updateMessageFlagsStartingFromAnchor, which is where the UI-feedback code is in that case.

}

/// Show a sheet of actions you can take on a topic.
///
/// Needs a [PageRoot] ancestor.
Expand Down
14 changes: 12 additions & 2 deletions lib/widgets/actions.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import 'dart:async';

import 'package:flutter/material.dart';

import '../api/exception.dart';
import '../api/model/model.dart';
import '../api/model/narrow.dart';
import '../api/route/messages.dart';
Expand Down Expand Up @@ -33,9 +34,13 @@ abstract final class ZulipAction {
return;
} catch (e) {
if (!context.mounted) return;
final message = switch (e) {
ZulipApiException() => zulipLocalizations.errorServerMessage(e.message),
_ => e.toString(),
};
showErrorDialog(context: context,
title: zulipLocalizations.errorMarkAsReadFailedTitle,
message: e.toString()); // TODO(#741): extract user-facing message better
message: message); // TODO(#741): extract user-facing message better
return;
}
}
Expand Down Expand Up @@ -191,9 +196,14 @@ abstract final class ZulipAction {
}
} catch (e) {
if (!context.mounted) return false;
final zulipLocalizations = ZulipLocalizations.of(context);
final message = switch (e) {
ZulipApiException() => zulipLocalizations.errorServerMessage(e.message),
_ => e.toString(),
};
showErrorDialog(context: context,
title: onFailedTitle,
message: e.toString()); // TODO(#741): extract user-facing message better
message: message); // TODO(#741): extract user-facing message better
return false;
}
}
Expand Down
29 changes: 16 additions & 13 deletions lib/widgets/icons.dart
Original file line number Diff line number Diff line change
Expand Up @@ -99,44 +99,47 @@ abstract final class ZulipIcons {
/// The Zulip custom icon "menu".
static const IconData menu = IconData(0xf119, fontFamily: "Zulip Icons");

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

/// The Zulip custom icon "message_feed".
static const IconData message_feed = IconData(0xf11a, fontFamily: "Zulip Icons");
static const IconData message_feed = IconData(0xf11b, fontFamily: "Zulip Icons");

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

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

/// The Zulip custom icon "send".
static const IconData send = IconData(0xf11d, fontFamily: "Zulip Icons");
static const IconData send = IconData(0xf11e, fontFamily: "Zulip Icons");

/// The Zulip custom icon "share".
static const IconData share = IconData(0xf11e, fontFamily: "Zulip Icons");
static const IconData share = IconData(0xf11f, fontFamily: "Zulip Icons");

/// The Zulip custom icon "share_ios".
static const IconData share_ios = IconData(0xf11f, fontFamily: "Zulip Icons");
static const IconData share_ios = IconData(0xf120, fontFamily: "Zulip Icons");

/// The Zulip custom icon "smile".
static const IconData smile = IconData(0xf120, fontFamily: "Zulip Icons");
static const IconData smile = IconData(0xf121, fontFamily: "Zulip Icons");

/// The Zulip custom icon "star".
static const IconData star = IconData(0xf121, fontFamily: "Zulip Icons");
static const IconData star = IconData(0xf122, fontFamily: "Zulip Icons");

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

/// The Zulip custom icon "three_person".
static const IconData three_person = IconData(0xf123, fontFamily: "Zulip Icons");
static const IconData three_person = IconData(0xf124, fontFamily: "Zulip Icons");

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

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

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

// END GENERATED ICON DATA
}
Expand Down
16 changes: 15 additions & 1 deletion lib/widgets/inbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,9 @@ abstract class _HeaderItem extends StatelessWidget {
// But that's in tension with the Figma, which gives these header rows
// 40px min height.
onTap: onCollapseButtonTap,
onLongPress: this is _LongPressable
? (this as _LongPressable).onLongPress
: null,
child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [
Padding(padding: const EdgeInsets.all(10),
child: Icon(size: 20, color: designVariables.sectionCollapseIcon,
Expand Down Expand Up @@ -431,7 +434,13 @@ class _DmItem extends StatelessWidget {
}
}

class _StreamHeaderItem extends _HeaderItem {
mixin _LongPressable on _HeaderItem {
// TODO(#1272) move to _HeaderItem base class
// when DM headers become long-pressable; remove mixin
Future<void> onLongPress();
}

class _StreamHeaderItem extends _HeaderItem with _LongPressable {
final Subscription subscription;

const _StreamHeaderItem({
Expand Down Expand Up @@ -464,6 +473,11 @@ class _StreamHeaderItem extends _HeaderItem {
}
}
@override Future<void> onRowTap() => onCollapseButtonTap(); // TODO open channel narrow

@override
Future<void> onLongPress() async {
showChannelActionSheet(sectionContext, channelId: subscription.streamId);
}
}

class _StreamSection extends StatelessWidget {
Expand Down
64 changes: 38 additions & 26 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -395,36 +395,47 @@ class MessageListAppBarTitle extends StatelessWidget {
case ChannelNarrow(:var streamId):
final store = PerAccountStoreWidget.of(context);
final stream = store.streams[streamId];
return _buildStreamRow(context, stream: stream);
return GestureDetector(
behavior: HitTestBehavior.translucent,
onLongPress: () {
showChannelActionSheet(context, channelId: streamId);
},
child: _buildStreamRow(context, stream: stream));

case TopicNarrow(:var streamId, :var topic):
final store = PerAccountStoreWidget.of(context);
final stream = store.streams[streamId];
return SizedBox(
width: double.infinity,
child: GestureDetector(
behavior: HitTestBehavior.translucent,
onLongPress: () {
final someMessage = MessageListPage.ancestorOf(context)
.model?.messages.firstOrNull;
// If someMessage is null, the topic action sheet won't have a
// resolve/unresolve button. That seems OK; in that case we're
// either still fetching messages (and the user can reopen the
// sheet after that finishes) or there aren't any messages to
// act on anyway.
assert(someMessage == null || narrow.containsMessage(someMessage));
showTopicActionSheet(context,
channelId: streamId,
topic: topic,
someMessageIdInTopic: someMessage?.id);
},
child: Column(
crossAxisAlignment: willCenterTitle ? CrossAxisAlignment.center
: CrossAxisAlignment.start,
children: [
_buildStreamRow(context, stream: stream),
_buildTopicRow(context, stream: stream, topic: topic),
])));
final alignment = willCenterTitle
? Alignment.center
: AlignmentDirectional.centerStart;
return Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
children: [
GestureDetector(
behavior: HitTestBehavior.translucent,
onLongPress: () {
showChannelActionSheet(context, channelId: streamId);
},
child: Align(alignment: alignment,
child: _buildStreamRow(context, stream: stream))),
GestureDetector(
behavior: HitTestBehavior.translucent,
onLongPress: () {
final someMessage = MessageListPage.ancestorOf(context)
.model?.messages.firstOrNull;
// If someMessage is null, the topic action sheet won't have a
// resolve/unresolve button. That seems OK; in that case we're
// either still fetching messages (and the user can reopen the
// sheet after that finishes) or there aren't any messages to
// act on anyway.
assert(someMessage == null || narrow.containsMessage(someMessage));
showTopicActionSheet(context,
channelId: streamId,
topic: topic,
someMessageIdInTopic: someMessage?.id);
},
child: Align(alignment: alignment,
child: _buildTopicRow(context, stream: stream, topic: topic)))]);

case DmNarrow(:var otherRecipientIds):
final store = PerAccountStoreWidget.of(context);
Expand Down Expand Up @@ -1083,6 +1094,7 @@ class StreamMessageRecipientHeader extends StatelessWidget {
onTap: () => Navigator.push(context,
MessageListPage.buildRoute(context: context,
narrow: ChannelNarrow(message.streamId))),
onLongPress: () => showChannelActionSheet(context, channelId: message.streamId),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs test coverage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've noticed an existing bug with the gesture handling here. I filed it as #1368; no need to fix it in this PR (but feel free to claim the issue if you'd like to work on it as a followup).

child: Row(
crossAxisAlignment: CrossAxisAlignment.center,
children: [
Expand Down
2 changes: 2 additions & 0 deletions lib/widgets/subscription_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import '../api/model/model.dart';
import '../generated/l10n/zulip_localizations.dart';
import '../model/narrow.dart';
import '../model/unreads.dart';
import 'action_sheet.dart';
import 'icons.dart';
import 'message_list.dart';
import 'store.dart';
Expand Down Expand Up @@ -230,6 +231,7 @@ class SubscriptionItem extends StatelessWidget {
MessageListPage.buildRoute(context: context,
narrow: ChannelNarrow(subscription.streamId)));
},
onLongPress: () => showChannelActionSheet(context, channelId: subscription.streamId),
child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [
const SizedBox(width: 16),
Padding(
Expand Down
Loading