Skip to content

Conversation

chrisbobbe
Copy link
Contributor

Fixes: #5101
Fixes: #5171

@chrisbobbe chrisbobbe requested a review from gnprice December 21, 2021 00:15
@chrisbobbe
Copy link
Contributor Author

Please also see #5175 for some further improvements to toast texts, and more.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! Looks great; small comments below.

@@ -587,3 +624,53 @@ export const showStreamActionSheet = ({
}),
);
};

export const showPmActionSheet = ({
Copy link
Member

Choose a reason for hiding this comment

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

Really more like "PM conversation action sheet", right? For an individual PM, there's the message action sheet.

Copy link
Member

Choose a reason for hiding this comment

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

(Similarly for the function constructing the buttons.)

this.setMessageInputValue(
this.state.message.replace(
placeholder,
`[${_('Uploading {fileName} failed.', { fileName })}]()`,
`[${_('Failed to upload file: {fileName}.', { fileName })}]()`,
Copy link
Member

Choose a reason for hiding this comment

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

Let's also drop the . at the end. That works when the filename is in the middle of the sentence, but here it risks looking like part of the filename; and in general I think we don't put periods at the end of these short messages.

|}>;

export default function GroupDetailsScreen(props: Props): Node {
/**
* Details about a PM conversation, identified by PM key recipients.
Copy link
Member

Choose a reason for hiding this comment

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

The second half of this is redundant with the type. Best to leave it out -- that way it doesn't become wrong if we change the type in the future (renaming it, or changing to something like PmNarrow, or something else.)

@chrisbobbe
Copy link
Contributor Author

Thanks! Revision pushed.

Thanks to Greg for aligning the type of the `recipients` route param
with this new name, in the recent commit 06a4abc.

Suggested-by: Greg Price <greg@zulip.com>
Instead of a toast with the recipients' names.

When we start targeting Android 12, which we hope to do soon, toasts
will be limited to two lines [1]. It was never a great design to use
a toast here in the first place, since in theory you can have a
group PM conversation with arbitrarily many members, and there's
only so big a toast can get before it starts to look weird or cut
things off in any implementation. With just two lines of space, the
toasts will be even less helpful.

Better to give an action sheet, like we do on long-pressing topic
headers. The action sheet gives a button to take you to the PM
recipients screen, where you can see all the details of the
recipients.

[1] https://developer.android.com/about/versions/12/behavior-changes-12#toast-redesign

Fixes-part-of: zulip#5171
The "failed" part is really important, and we can't have it get cut
off [1] just because a file name is very long.

[1] https://developer.android.com/about/versions/12/behavior-changes-12#toast-redesign

Fixes: zulip#5171
…id 12

See the change described at
  https://developer.android.com/about/versions/12/behavior-changes-12#exported .

A value of "true" means that the component accepts input from other
apps.

- The launcher is another app, so we need "true" there so that Zulip
  can be launched.

- We need "true" for the share-to-Zulip activity; the content to be
  shared originates from other apps.
This change will be required in order to upload new releases to the
Play Store, effective probably 2022-11-01. That isn't very soon, but
we cut it kind of close last time, and we don't want to this time.

The change causes Android 12 and later to apply to our app a number
of behavior changes introduced in that version, documented here:
  https://developer.android.com/about/versions/12/behavior-changes-12

Most of the work for this was zulip#5145, "android notif: Replace our
`NotificationIntentService` with directly opening MainActivity".
Earlier in this series of commits, we also fixed zulip#5171 ("Handle
stricter length limit on Android toasts when targeting Android 12")
and handled the "Safer component exporting" change described at
  https://developer.android.com/about/versions/12/behavior-changes-12#exported .

There were a few other things that stood out in that "Behavior
Changes" article, but none that seem to require any specific action
from us; see
  zulip#5101 (comment) .

Fixes: zulip#5101
@gnprice gnprice force-pushed the pr-target-android-12 branch from 4bb1a85 to 8c2440c Compare December 22, 2021 00:56
@gnprice
Copy link
Member

gnprice commented Dec 22, 2021

Thanks! Looks good -- merging.

@gnprice gnprice merged commit 8c2440c into zulip:main Dec 22, 2021
@chrisbobbe
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle stricter length limit on Android toasts when targeting Android 12. Target Android 12 (set targetSdkVersion to 31), by 2022-11 deadline
2 participants