messageActionSheet: Add option to copy link of message to clipboard by chrisbobbe · Pull Request #3865 · zulip/zulip-mobile

chrisbobbe · 2020-02-03T19:49:40Z

STATUS EDIT: This has gone stale; I'd be happy for a new PR to be made (particularly with #4146 in mind, and perhaps #3757 will be done by the time we pick this up again), drawing from the work done here if that's helpful.

Continuation of #3412, responding to the changes requested in #3412 (comment).

These commits do introduce user-facing strings; should I get set up with Transifex or is that something you do, @gnprice?

gnprice · 2020-02-21T23:29:27Z

Adding a cross-reference: I think this is meant to fix #2623.

gnprice · 2020-02-21T23:32:15Z

These commits do introduce user-facing strings; should I get set up with Transifex or is that something you do, @gnprice?

The essential thing in the PR branch is just that it adds to messages_en.json; that's the data we then feed to Transifex. I can sync to Transifex and back, but also we should get you set up with it :-)

gnprice

Thanks @chrisbobbe for picking this up! Some small comments below.

One UI comment: I think we can remove the "Share" option. It seems like a lot to have three of these:
"""
Copy message content
Copy message link
Share
"""
and the action sheet is pretty overwhelming already.

gnprice · 2020-02-21T23:39:25Z


+/**
+ * Produce the operand of a `stream` operator from a message.
+ * See encode_stream_id in `static/js/util` in the web app.


hash_util, I think

gnprice · 2020-02-21T23:40:22Z

+ * */
+const unparseStreamOperand = (message: Message) => {
+  const name = (message.display_recipient || 'unknown').replace(' ', '-');
+  return `${encodeHashComponent(message.stream_id.toString())}-${encodeHashComponent(name)}`;


Could be made slightly closer to the webapp code, and slightly shorter, by making one encodeHashComponent call after the ${...}-${...} step.

gnprice · 2020-02-21T23:41:46Z

+ * See encode_stream_id in `static/js/util` in the web app.
+ * */
+const unparseStreamOperand = (message: Message) => {
+  const name = (message.display_recipient || 'unknown').replace(' ', '-');


My first reaction to this unknown bit was that that seems like a sign we should raise an error instead. But that's what the webapp does! In this case I think aligning with the webapp code wins, particularly as this is logic we'd ideally like to share in the future.

gnprice · 2020-02-21T23:48:34Z

+    message.type === 'stream'
+      ? `stream/${unparseStreamOperand(message)}/topic/${unparseTopicOperand(message)}`
+      : `pm-with/${unparsePmOperand(message)}`;
+  return encodeURI(


Why encodeURI? It looks like that differs from the webapp -- and it seems like it'd cause a second round of %-encoding.

... I guess that's probably a no-op, because of our dot-encoding? Cleaner to leave it off, though.

gnprice · 2020-02-21T23:52:03Z

+  });
+  test('properly encodes streams with tricky characters', () => {


nit: blank lines between tests (here and below)

gnprice · 2020-02-22T00:03:12Z

+  Clipboard.setString(getLinkToMessage(auth.realm, message));
+  showToast(_('Copied link to message'));
+};
+copyMessageLink.title = 'Copy message link';


I looked at this and thought "gosh, that sounds ambiguous -- what about when the message contains a link? won't it sound like this means to copy that link?"

Then I found there'd been discussion of exactly that on the previous PR: #3412 (comment) 🙂 and I guess my conclusion was:

I think it's still a little ambiguous what "message link" means [...] Is it "Copy a link to the message", or "Copy the link in the message"? But it's good enough I'll be happy to merge it, and we can wordsmith further later.

So that seems reasonable -- I'll stand by that. If you have some ideas for that further wordsmithing, though, I'm interested!

Considering that the success string just above is "Copied link to message", I'd say present-tensing that as "Copy link to message" is probably for the best.

gnprice · 2020-02-22T00:05:04Z

+  "Copy message content": "Copy message content",
+  "Copied message content": "Copied message content",
+  "Copy message link": "Copy message link",
+  "Copied link to message": "Copied link to message",


I think there's a few more new/edited strings, like "Failed to copy message content to clipboard".

Good catch, thanks

gnprice · 2020-02-22T00:08:38Z

  const rawMessage = isAnOutboxMessage(message) /* $FlowFixMe: then really type Outbox */
    ? message.markdownContent
    : (await api.getRawMessageContent(auth, message.id)).raw_content;


In the last commit when getRawMessageContent gets introduced, this spot should switch to it. Basically that new function is the result of factoring out this statement.

rk-for-zulip

A few trivialities:

rk-for-zulip · 2020-02-24T21:07:35Z

+      subject: '.%%2E/#\ud83d\udc4b',
+    });
+    expect(getLinkToMessage(realm, message)).toEqual(
+      `${realm}/#narrow/stream/123-general/topic/.2E.25.252E.2F.23.F0.9F.91.8B/near/${message.id}`,


You may also want to add a non-ASCII Unicode character in the BMP (i.e., one not represented by a pair of surrogates in UTF-16).

rk-for-zulip · 2020-02-24T21:14:02Z

+  Clipboard.setString(getLinkToMessage(auth.realm, message));
+  showToast(_('Copied link to message'));
+};
+copyMessageLink.title = 'Copy message link';


Considering that the success string just above is "Copied link to message", I'd say present-tensing that as "Copy link to message" is probably for the best.

rk-for-zulip · 2020-02-24T21:34:35Z

 const isAnOutboxMessage = (message: Message | Outbox): boolean => message.isOutbox;

+const getRawMessageContent = async (auth: Auth, message: Message | Outbox) =>
+  isAnOutboxMessage(message) /* $FlowFixMe: then really type Outbox */


This should be fixable simply by testing message.isOutbox inline, rather than calling isAnOutboxMessage.

chrisbobbe · 2020-02-24T21:49:31Z

One UI comment: I think we can remove the "Share" option. It seems like a lot to have three of these:

I tend to agree, but just for completeness, here's a consolidated list of issues related to the "Share" option (and thus that would be affected by its removal), and @jainkuniya's comment in support of keeping it:

#3796
#3793
#3577
#2122
#2623 (comment)

Let me know how you'd like to proceed.

Add getLinkToMessage, on the model of by_conversation_and_time_uri in static/js/hash_util.js in zulip/zulip, to prepare for the child, which adds an option in the message action sheet to share a link to the message.

Use `getLinkToMessage`, introduced in the parent, to enable an option, called 'Copy message link', from the message action sheet to copy a link to the message to the clipboard. Preserve the option to copy the message's content, but rename that to 'copy message content.' Fixes: zulip#2623.

…essage. This may not be kept around forever because copying a link and copying a message's contents are both supported. But now there's a way to do both in the same action, from the Share menu.

Make `getRawMessageContent` as a proxy for getting the raw (markdown) message content, whether from an API request, in the case of a Message, or directly from the object, in case of an Outbox.

chrisbobbe · 2020-02-28T00:10:30Z

I've made the other changes; just my question in #3865 (comment) remains, I think.

rk-for-zulip · 2020-02-29T00:58:22Z

Probably what we need is some sort of hierarchical menu:

┎━━━━━━━━━━━━━━┓      ┌──────────────┐
┃ Copy...    ▶ ┃      │ Copy Link    │
┡━━━━━━━━━━━━━━┩      ├──────────────┤
│ Edit Message │      │ Copy Message │
├──────────────┤   ⇒  └──────────────┘
│ Share        │
├──────────────┤
│     ....     │
└──────────────┘

... and while I'm sure there's an easy-ish way to do that in React Native, I'm not familiar with it.

chrisbobbe mentioned this pull request Feb 3, 2020

messageActionSheet: Add option to copy link of message to clipboard #3412

Closed

chrisbobbe force-pushed the copy-message-link branch from f36cade to 43ad7da Compare February 5, 2020 00:29

chrisbobbe linked an issue Feb 6, 2020 that may be closed by this pull request

Replace "Share" with "Copy link to message" #2623

Open

chrisbobbe mentioned this pull request Feb 7, 2020

Update implementation for subject_links -> topic_links transition #3884

Open

gnprice reviewed Feb 22, 2020

View reviewed changes

gnprice changed the title ~~[WIP] messageActionSheet: Add option to copy link of message to clipboard~~ messageActionSheet: Add option to copy link of message to clipboard Feb 22, 2020

rk-for-zulip reviewed Feb 24, 2020

View reviewed changes

ishammahajan added 4 commits February 27, 2020 15:53

internalLinks: Add getLinkToMessage function.

eb69ee3

Add getLinkToMessage, on the model of by_conversation_and_time_uri in static/js/hash_util.js in zulip/zulip, to prepare for the child, which adds an option in the message action sheet to share a link to the message.

messageActionSheet: Make Share -> Copy copy a link along with the m…

983f142

…essage. This may not be kept around forever because copying a link and copying a message's contents are both supported. But now there's a way to do both in the same action, from the Share menu.

messageActionSheet: Change logic for getting raw message content.

52cfb9c

Make `getRawMessageContent` as a proxy for getting the raw (markdown) message content, whether from an API request, in the case of a Message, or directly from the object, in case of an Outbox.

chrisbobbe force-pushed the copy-message-link branch from 43ad7da to 52cfb9c Compare February 28, 2020 00:09

chrisbobbe marked this pull request as draft April 22, 2021 19:18

		});
		test('properly encodes streams with tricky characters', () => {

Uh oh!

Uh oh!

Conversation

chrisbobbe commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Feb 21, 2020

Uh oh!

gnprice commented Feb 21, 2020

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rk-for-zulip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Feb 28, 2020

Uh oh!

rk-for-zulip commented Feb 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrisbobbe commented Feb 3, 2020 •

edited

Loading

chrisbobbe commented Feb 24, 2020 •

edited

Loading