-
Notifications
You must be signed in to change notification settings - Fork 306
Better management of archived channel names #1344
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
base: main
Are you sure you want to change the base?
Changes from all commits
1ba56c5
781ea95
45cafaf
b29c004
982aefd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -230,19 +230,22 @@ void showTopicActionSheet(BuildContext context, { | |
final pageContext = PageRoot.contextOf(context); | ||
|
||
final store = PerAccountStoreWidget.of(pageContext); | ||
final channel = store.streams[channelId]; | ||
final subscription = store.subscriptions[channelId]; | ||
|
||
final optionButtons = <ActionSheetMenuItemButton>[]; | ||
|
||
final isChannelArchived = channel?.isArchived == true; | ||
// TODO(server-7): simplify this condition away | ||
final supportsUnmutingTopics = store.zulipFeatureLevel >= 170; | ||
// TODO(server-8): simplify this condition away | ||
final supportsFollowingTopics = store.zulipFeatureLevel >= 219; | ||
|
||
final visibilityOptions = <UserTopicVisibilityPolicy>[]; | ||
final visibilityPolicy = store.topicVisibilityPolicy(channelId, topic); | ||
if (subscription == null) { | ||
// Not subscribed to the channel; there is no user topic change to be made. | ||
if (subscription == null || isChannelArchived) { | ||
// Not subscribed to the channel or the channel is archived; | ||
// there is no user topic change to be made. | ||
} else if (!subscription.isMuted) { | ||
// Channel is subscribed and not muted. | ||
switch (visibilityPolicy) { | ||
|
@@ -306,7 +309,8 @@ void showTopicActionSheet(BuildContext context, { | |
// limit for editing topics). | ||
if (someMessageIdInTopic != null | ||
// ignore: unnecessary_null_comparison // null topic names soon to be enabled | ||
&& topic.displayName != null) { | ||
&& topic.displayName != null | ||
&& !isChannelArchived) { | ||
optionButtons.add(ResolveUnresolveButton(pageContext: pageContext, | ||
topic: topic, | ||
someMessageIdInTopic: someMessageIdInTopic)); | ||
|
@@ -564,14 +568,19 @@ void showMessageActionSheet({required BuildContext context, required Message mes | |
final messageListPage = MessageListPage.ancestorOf(pageContext); | ||
final isComposeBoxOffered = messageListPage.composeBoxController != null; | ||
|
||
bool isInArchivedChannel = false; | ||
if (message is StreamMessage) { | ||
final channel = store.streams[message.streamId]; | ||
isInArchivedChannel = channel?.isArchived == true; | ||
} | ||
final isMessageRead = message.flags.contains(MessageFlag.read); | ||
final markAsUnreadSupported = store.zulipFeatureLevel >= 155; // TODO(server-6) | ||
final showMarkAsUnreadButton = markAsUnreadSupported && isMessageRead; | ||
|
||
final optionButtons = [ | ||
ReactionButtons(message: message, pageContext: pageContext), | ||
StarButton(message: message, pageContext: pageContext), | ||
if (isComposeBoxOffered) | ||
if (isComposeBoxOffered && !isInArchivedChannel) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the direct reason that |
||
QuoteAndReplyButton(message: message, pageContext: pageContext), | ||
if (showMarkAsUnreadButton) | ||
MarkAsUnreadButton(message: message, pageContext: pageContext), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1545,8 +1545,10 @@ class _ComposeBoxState extends State<ComposeBox> with PerAccountStoreAwareStateM | |
case ChannelNarrow(:final streamId): | ||
case TopicNarrow(:final streamId): | ||
final channel = store.streams[streamId]; | ||
if (channel == null || !store.hasPostingPermission(inChannel: channel, | ||
user: store.selfUser, byDate: DateTime.now())) { | ||
if (channel == null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: indentation |
||
|| !store.hasPostingPermission(inChannel: channel, | ||
user: store.selfUser, byDate: DateTime.now()) | ||
|| channel.isArchived) { | ||
return _ErrorBanner(getLabel: (zulipLocalizations) => | ||
zulipLocalizations.errorBannerCannotPostInChannelLabel); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -240,6 +240,7 @@ abstract class _HeaderItem extends StatelessWidget { | |||||
|
||||||
String title(ZulipLocalizations zulipLocalizations); | ||||||
IconData get icon; | ||||||
InlineSpan? buildTrailing(BuildContext context) => null; | ||||||
Color collapsedIconColor(BuildContext context); | ||||||
Color uncollapsedIconColor(BuildContext context); | ||||||
Color uncollapsedBackgroundColor(BuildContext context); | ||||||
|
@@ -285,18 +286,24 @@ abstract class _HeaderItem extends StatelessWidget { | |||||
: uncollapsedIconColor(context), | ||||||
icon), | ||||||
const SizedBox(width: 5), | ||||||
Expanded(child: Padding( | ||||||
padding: const EdgeInsets.symmetric(vertical: 4), | ||||||
child: Text( | ||||||
style: TextStyle( | ||||||
fontSize: 17, | ||||||
height: (20 / 17), | ||||||
// TODO(design) check if this is the right variable | ||||||
color: designVariables.labelMenuButton, | ||||||
).merge(weightVariableTextStyle(context, wght: 600)), | ||||||
maxLines: 1, | ||||||
overflow: TextOverflow.ellipsis, | ||||||
title(zulipLocalizations)))), | ||||||
Expanded( | ||||||
child: Padding( | ||||||
Comment on lines
+289
to
+290
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: let's preserve the original formatting, to de-indent the following lines |
||||||
padding: const EdgeInsets.symmetric(vertical: 4), | ||||||
child: RichText( | ||||||
maxLines: 1, | ||||||
overflow: TextOverflow.ellipsis, | ||||||
text: TextSpan( | ||||||
children: [ | ||||||
TextSpan( | ||||||
text: title(zulipLocalizations), | ||||||
style: TextStyle( | ||||||
fontSize: 17, | ||||||
height: 20 / 17, | ||||||
// TODO(design) check if this is the right variable | ||||||
color: designVariables.labelMenuButton) | ||||||
.merge(weightVariableTextStyle(context, wght: 600))), | ||||||
buildTrailing(context) ?? const TextSpan(), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we can skip the
Suggested change
where |
||||||
])))), | ||||||
const SizedBox(width: 12), | ||||||
if (hasMention) const _IconMarker(icon: ZulipIcons.at_sign), | ||||||
Padding(padding: const EdgeInsetsDirectional.only(end: 16), | ||||||
|
@@ -436,9 +443,11 @@ mixin _LongPressable on _HeaderItem { | |||||
|
||||||
class _StreamHeaderItem extends _HeaderItem with _LongPressable { | ||||||
final Subscription subscription; | ||||||
final bool isArchived; | ||||||
|
||||||
const _StreamHeaderItem({ | ||||||
required this.subscription, | ||||||
required this.isArchived, | ||||||
required super.collapsed, | ||||||
required super.pageState, | ||||||
required super.count, | ||||||
|
@@ -449,6 +458,23 @@ class _StreamHeaderItem extends _HeaderItem with _LongPressable { | |||||
@override String title(ZulipLocalizations zulipLocalizations) => | ||||||
subscription.name; | ||||||
@override IconData get icon => iconDataForStream(subscription); | ||||||
@override InlineSpan? buildTrailing(BuildContext context) { | ||||||
if (!isArchived) return null; | ||||||
|
||||||
final designVariables = DesignVariables.of(context); | ||||||
final zulipLocalizations = ZulipLocalizations.of(context); | ||||||
|
||||||
return WidgetSpan( | ||||||
child: Padding( | ||||||
padding: const EdgeInsetsDirectional.only(start: 4), | ||||||
child: Text( | ||||||
zulipLocalizations.channelArchivedLabel, | ||||||
style: TextStyle( | ||||||
fontSize: 17, | ||||||
height: 20 / 17, | ||||||
color: designVariables.labelMessageHeaderArchived, | ||||||
fontStyle: FontStyle.italic)))); | ||||||
} | ||||||
@override Color collapsedIconColor(context) => | ||||||
colorSwatchFor(context, subscription).iconOnPlainBackground; | ||||||
@override Color uncollapsedIconColor(context) => | ||||||
|
@@ -495,6 +521,7 @@ class _StreamSection extends StatelessWidget { | |||||
collapsed: collapsed, | ||||||
pageState: pageState, | ||||||
sectionContext: context, | ||||||
isArchived: subscription.isArchived, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this should match the constructor order of parameters There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, since the data comes from |
||||||
); | ||||||
return StickyHeaderItem( | ||||||
header: header, | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -313,6 +313,7 @@ class MessageListAppBarTitle extends StatelessWidget { | |
ZulipStream? stream, | ||
}) { | ||
final zulipLocalizations = ZulipLocalizations.of(context); | ||
final designVariables = DesignVariables.of(context); | ||
// A null [Icon.icon] makes a blank space. | ||
final icon = stream != null ? iconDataForStream(stream) : null; | ||
return Row( | ||
|
@@ -326,6 +327,17 @@ class MessageListAppBarTitle extends StatelessWidget { | |
const SizedBox(width: 4), | ||
Flexible(child: Text( | ||
stream?.name ?? zulipLocalizations.unknownChannelName)), | ||
if (stream?.isArchived ?? false) | ||
// TODO(#1285): Avoid concatenating translated strings | ||
Padding( | ||
padding: EdgeInsetsDirectional.fromSTEB(4, 4, 0, 4), | ||
child: Text( | ||
zulipLocalizations.channelArchivedLabel, | ||
style: TextStyle( | ||
fontSize: 18, | ||
// TODO(design): check if this is the right variable | ||
color: designVariables.labelMessageHeaderArchived, | ||
fontStyle: FontStyle.italic))), | ||
]); | ||
} | ||
|
||
|
@@ -1102,6 +1114,18 @@ class StreamMessageRecipientHeader extends StatelessWidget { | |
style: recipientHeaderTextStyle(context), | ||
overflow: TextOverflow.ellipsis), | ||
), | ||
if (stream?.isArchived ?? false) | ||
// TODO(#1285): Avoid concatenating translated strings | ||
Padding( | ||
padding: const EdgeInsetsDirectional.fromSTEB(4, 4, 0, 4), | ||
child: Text( | ||
zulipLocalizations.channelArchivedLabel, | ||
style: recipientHeaderTextStyle(context, | ||
// TODO(design): check if this is the right variable | ||
color: designVariables.labelMessageHeaderArchived, | ||
fontStyle: FontStyle.italic), | ||
overflow: TextOverflow.ellipsis, | ||
maxLines: 1)), | ||
Comment on lines
+1127
to
+1128
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
Padding( | ||
// Figma has 5px horizontal padding around an 8px wide icon. | ||
// Icon is 16px wide here so horizontal padding is 1px. | ||
|
@@ -1220,9 +1244,13 @@ class DmRecipientHeader extends StatelessWidget { | |
} | ||
} | ||
|
||
TextStyle recipientHeaderTextStyle(BuildContext context, {FontStyle? fontStyle}) { | ||
TextStyle recipientHeaderTextStyle( | ||
BuildContext context, { | ||
Color? color, | ||
FontStyle? fontStyle, | ||
}) { | ||
return TextStyle( | ||
color: DesignVariables.of(context).title, | ||
color: color ?? DesignVariables.of(context).title, | ||
fontSize: 16, | ||
letterSpacing: proportionalLetterSpacing(context, 0.02, baseFontSize: 16), | ||
height: (18 / 16), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,7 @@ class _SubscriptionListPageBodyState extends State<SubscriptionListPageBody> wit | |
final List<Subscription> pinned = []; | ||
final List<Subscription> unpinned = []; | ||
for (final subscription in store.subscriptions.values) { | ||
if (subscription.isArchived) continue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make sure that |
||
if (subscription.pinToTop) { | ||
pinned.add(subscription); | ||
} else { | ||
|
@@ -188,6 +189,8 @@ class _SubscriptionList extends StatelessWidget { | |
|
||
@override | ||
Widget build(BuildContext context) { | ||
assert(subscriptions.every((subscription) => !subscription.isArchived)); | ||
|
||
return SliverList.builder( | ||
itemCount: subscriptions.length, | ||
itemBuilder: (BuildContext context, int index) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this optional. On older servers, this field will not be sent.
A clean way to do this might be defaulting this to
false
, since its value can be indicated from the fact that no archived channels were returned from this endpoint before, with a// TODO(server-10) remove default
comment.