Add "Copy to clipboard" button to message action sheet #213

chrisbobbe · 2023-07-05T23:54:11Z

This leaves some TODOs in the tests, for testing code that calls into a Flutter plugin. Greg is working on a good way to do that; see #204 (comment).

Empirically, the relevant calls here return Futures that never resolve, which will be fine until we want to stub out the native-code functionality.

Fixes: #132

gnprice

Thanks!

Small comments below. Let's also try adding tests of the copyWithPopup calls, atop those first few commits of #204 (which I think we're likely to merge soon.)

gnprice · 2023-07-06T00:55:02Z

lib/widgets/action_sheet.dart

+    if (rawContent == null) {
+      return;
+    }
+
+    if (!messageListContext.mounted) return;


nit: the non-parallelism looks a bit odd:

Suggested change

if (rawContent == null) {

return;

}

if (!messageListContext.mounted) return;

if (rawContent == null) return;

if (!messageListContext.mounted) return;

gnprice · 2023-07-06T00:55:52Z

lib/widgets/action_sheet.dart

+
+  @override get icon => Icons.copy;
+
+  @override get label => 'Copy to clipboard';


This should be a bit clearer about what it's going to do, I think — there are several things one might imagine being copied.

Perhaps "Copy message text"?

chrisbobbe · 2023-07-19T19:09:10Z

Thanks for the review! Revision pushed, with:

tests of copyWithPopup
clipboard checks in the message-action-sheet tests for "Copy message text"

gnprice

Cool! Glad to have tests for this. Various small comments below.

gnprice · 2023-07-20T00:08:12Z

lib/model/binding.dart

+  /// Provides device and operating system information,
+  /// via package:device_info_plus.
+  ///
+  /// This wraps [DeviceInfoPlugin.deviceInfo].


Suggested change

/// This wraps [DeviceInfoPlugin.deviceInfo].

/// This wraps [device_info_plus.DeviceInfoPlugin.deviceInfo].

gnprice · 2023-07-20T00:20:38Z

lib/model/binding.dart

+  /// The user-visible SDK version of the framework.
+  ///
+  /// Possible values are defined in: https://developer.android.com/reference/android/os/Build.VERSION_CODES.html


Suggested change

/// The user-visible SDK version of the framework.

///

/// Possible values are defined in: https://developer.android.com/reference/android/os/Build.VERSION_CODES.html

```suggestion

/// The Android SDK version.

///

/// Possible values are defined in: https://developer.android.com/reference/android/os/Build.VERSION_CODES.html

I wouldn't call it "user-visible" — one never sees this number as an Android user, only as an Android developer.

(I guess the Android docs on the related field https://developer.android.com/reference/android/os/Build.VERSION#SDK calls it that. Looks like that text dates at least to the dawn of Android's public Git history in 2008. It's possible it made sense then, but it hasn't in a long time.)

Ah yeah, this was copied from the plugin. Will remove that.

gnprice · 2023-07-20T00:21:20Z

lib/model/binding.dart

+  /// The current operating system version.
+  /// https://developer.apple.com/documentation/uikit/uidevice/1620043-systemversion


Suggested change

/// The current operating system version.

/// https://developer.apple.com/documentation/uikit/uidevice/1620043-systemversion

/// The current operating system version.

///

/// See: https://developer.apple.com/documentation/uikit/uidevice/1620043-systemversion

gnprice · 2023-07-20T00:26:12Z

lib/model/binding.dart

+
+  @override
+  Future<BaseDeviceInfo> deviceInfo() async {
+    return switch (await device_info_plus.DeviceInfoPlugin().deviceInfo) {


Doesn't super matter in this instance, but in general I like making awaits conspicuous — they're a key part of the control flow, after all — by putting them at the head of either a whole statement or an initializer:

Suggested change

return switch (await device_info_plus.DeviceInfoPlugin().deviceInfo) {

final deviceInfo = await device_info_plus.DeviceInfoPlugin().deviceInfo;

return switch (deviceInfo) {

gnprice · 2023-07-20T00:29:45Z

test/model/binding.dart

 }
+
+final _defaultDeviceInfoResult = IosDeviceInfo(systemVersion: '16.0');


Let's make this a static on the class — that way it can be grouped right next to the rest of the device-info stuff, even while this binding continues to grow more features.

Also let's make it an Android version, because that's what Flutter elsewhere defaults to in tests:
https://api.flutter.dev/flutter/foundation/defaultTargetPlatform.html

gnprice · 2023-07-20T00:38:05Z

test/model/binding.dart

+  /// This returns a list of the arguments to all calls made
+  /// to `ZulipBinding.instance.deviceInfo()` since the last call to


/// This returns a list of the arguments to all calls made

List<()>

😆

I think I'm actually inclined to leave out this "take" method, and the log it consumes, entirely, though. Because the underlying method doesn't (or anyway isn't meant to) have any side effects, I don't think it's ever going to really be a caller's or a test's business whether the implementation of some feature calls this method, or in particular when or how many times it does so. What is a caller's or test's business is whether the implementation successfully does the appropriate things on different platforms; and the test tests that by having several variants of itself that manipulate deviceInfoResult differently.

If we later wrap some method that does have side effects (so a test inspecting a call log is appropriate) but takes no arguments, though, I'd be fine with this type for the corresponding "take" method. I'm not sure the formal parallelism with takeLaunchUrlCalls (and future methods like it) is ultimately helpful, but maybe it will be.

Haha, makes sense.

gnprice · 2023-07-20T00:47:06Z

test/test_clipboard.dart

+
+  dynamic clipboardData;
+
+  Future<Object?> handleMethodCall(MethodCall methodCall) async {


This is based on some piece of test code in the Flutter tree, right? Worth a brief comment pointing to there — the code is pretty gnarly-looking with all those dynamics, and that's helpful context for understanding it.

gnprice · 2023-07-20T00:47:52Z

test/test_clipboard.dart

+      case 'Clipboard.setData':
+        clipboardData = methodCall.arguments;
+      default:
+        if (methodCall.method.startsWith('Clipboard')) {


Suggested change

if (methodCall.method.startsWith('Clipboard')) {

if (methodCall.method.startsWith('Clipboard.')) {

unless you intend to match things like "ClipboardPlus.getFancyData" too.

gnprice · 2023-07-20T00:57:31Z

test/widgets/clipboard_test.dart

+void main() {
+  setUp(() async {
+    TestZulipBinding.ensureInitialized();
+    TestWidgetsFlutterBinding.ensureInitialized();


Suggested change

void main() {

setUp(() async {

TestZulipBinding.ensureInitialized();

TestWidgetsFlutterBinding.ensureInitialized();

void main() {

TestZulipBinding.ensureInitialized();

TestWidgetsFlutterBinding.ensureInitialized();

setUp(() async {

That's the usual idiom for these, because they only have effect once.

gnprice · 2023-07-20T01:07:13Z

test/widgets/clipboard_test.dart

+    TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.setMockMethodCallHandler(
+      SystemChannels.platform,
+      MockClipboard().handleMethodCall,


This one is good to do in the per-test setup, though, because it resets the clipboard's data.

chrisbobbe · 2023-07-20T19:45:11Z

Thanks for the review! Revision pushed.

These will also be helpful for testing the upcoming "Copy to clipboard" button, which also requires fetching raw Markdown from the server.

Fixes: zulip#132

gnprice · 2023-08-01T00:18:34Z

Thanks for the revision! Sorry for the delayed review. All looks good; merging.

chrisbobbe added the a-msglist The message-list screen, except what's label:a-content label Jul 5, 2023

chrisbobbe requested a review from gnprice July 5, 2023 23:54

gnprice reviewed Jul 6, 2023

View reviewed changes

chrisbobbe force-pushed the pr-copy-to-clipboard branch from d28aaa8 to a383afe Compare July 19, 2023 19:07

chrisbobbe force-pushed the pr-copy-to-clipboard branch from a383afe to 020fb80 Compare July 19, 2023 19:09

gnprice reviewed Jul 20, 2023

View reviewed changes

chrisbobbe force-pushed the pr-copy-to-clipboard branch from 020fb80 to fe719c2 Compare July 20, 2023 19:45

chrisbobbe added 7 commits July 31, 2023 17:14

binding: Wrap device_info_plus's DeviceInfoPlugin().deviceInfo getter

ec352c8

clipboard tests: Add, using new TestZulipBinding.deviceInfo()

df9367c

clipboard [nfc]: Tighten up unmounted-return

bce1274

clipboard [nfc]: Tighten up shouldShowSnackbar

fe84c60

action-sheet tests [nfc]: Move some helpers out a level

821e9d0

These will also be helpful for testing the upcoming "Copy to clipboard" button, which also requires fetching raw Markdown from the server.

action sheet: Pull out fetchRawContentWithFeedback helper

e1ca6a1

action_sheet: Add "Copy message text" button

9a06553

Fixes: zulip#132

gnprice force-pushed the pr-copy-to-clipboard branch from fe719c2 to 9a06553 Compare August 1, 2023 00:18

gnprice merged commit 9a06553 into zulip:main Aug 1, 2023

chrisbobbe deleted the pr-copy-to-clipboard branch August 1, 2023 00:19

gnprice mentioned this pull request Aug 1, 2023

test: Fix new action-sheet tests #244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Copy to clipboard" button to message action sheet #213

Add "Copy to clipboard" button to message action sheet #213

chrisbobbe commented Jul 5, 2023

gnprice left a comment

gnprice Jul 6, 2023

gnprice Jul 6, 2023

chrisbobbe commented Jul 19, 2023

gnprice left a comment

gnprice Jul 20, 2023

gnprice Jul 20, 2023

chrisbobbe Jul 20, 2023

gnprice Jul 20, 2023

gnprice Jul 20, 2023

gnprice Jul 20, 2023

gnprice Jul 20, 2023

chrisbobbe Jul 20, 2023

gnprice Jul 20, 2023

gnprice Jul 20, 2023

gnprice Jul 20, 2023

gnprice Jul 20, 2023

chrisbobbe commented Jul 20, 2023

gnprice commented Aug 1, 2023


		@override get icon => Icons.copy;

		@override get label => 'Copy to clipboard';

	/// This wraps [DeviceInfoPlugin.deviceInfo].
	/// This wraps [device_info_plus.DeviceInfoPlugin.deviceInfo].

		/// The current operating system version.
		/// https://developer.apple.com/documentation/uikit/uidevice/1620043-systemversion

	return switch (await device_info_plus.DeviceInfoPlugin().deviceInfo) {
	final deviceInfo = await device_info_plus.DeviceInfoPlugin().deviceInfo;
	return switch (deviceInfo) {

		}

		final _defaultDeviceInfoResult = IosDeviceInfo(systemVersion: '16.0');

		/// This returns a list of the arguments to all calls made
		/// to `ZulipBinding.instance.deviceInfo()` since the last call to


		dynamic clipboardData;

		Future<Object?> handleMethodCall(MethodCall methodCall) async {

	if (methodCall.method.startsWith('Clipboard')) {
	if (methodCall.method.startsWith('Clipboard.')) {

Add "Copy to clipboard" button to message action sheet #213

Add "Copy to clipboard" button to message action sheet #213

Conversation

chrisbobbe commented Jul 5, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 19, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 20, 2023

gnprice commented Aug 1, 2023