Some time may pass while we're at this await, and the user might make some edits in the input box. It looks like if they add or remove text before the start of what we inserted, the effect will be that we replace the wrong range of text, because beg and end no longer point to where it is. That'll likely leave the markup somewhat garbled, and it may also overwrite what they've typed.

I think with the native APIs it would be possible to attach something like a "span" to the relevant chunk of text, so that we can refer back to it even after the user has made edits that change its offset. But it looks like RN doesn't expose that functionality.

An alternative solution would be to look for the placeholder string we inserted -- i.e. [Uploading ${fileName}...]() -- and just replace that at the (first) place we find it. Hmm, and if we don't find it (because e.g. the user edited in the middle of it), then I guess insert it at the end. That seems like it'd be pretty robust.

yeah this seems much better, I am doing the find and replace implementation you suggested, I also checked out what RN provides just in case and they don't expose the "span" functionality just as you mentioned, I believe these were the right documentation to look at: https://reactnative.dev/docs/textinput

Yep, I think that's right!

I think the overall sequence of what this code does, if there are several files to upload/attach, is going to be:

• We insert the first placeholder, and start uploading.
• Eventually the upload finishes. We edit that placeholder, insert the next one, and start uploading.
• Etc., until we edit the final placeholder.

And I guess that agrees with what I see in your helpful screencaps.

That seems like not quite the ideal sequence -- having the placeholders come in one by one may feel kind of glitchy.

I think a better sequence would be:

• We insert all the placeholders immediately. We start uploading the first file.
• Eventually the upload finishes. We edit that placeholder, and start uploading the next file.
• Etc., until we edit the final placeholder.

(I think this code already makes exactly the right choice in that it only attempts one upload at a time. Uploads are probably often big files, and we may often be on a slow network connection -- no sense hammering it with a lot of uploads at once.)

So now I am doing it as you suggested (with a simple addition on failure):

• inserting all placeholders immediately.
• Search placeholders one by one and replace them with linkText.
• If any search fails, append the text at the very end of the message.
• If upload itself fails I am now replacing the placeholder with [Uploading ${fileName} failed.]().

The last point is important to note since it differs from the web app and the previous implementation. I took this decision based on the fact that replacing it with nothing won't be an ideal choice, since it would be very easy to miss out that some uploads have failed (even with a toast message). a permanent replacement with [Uploading ${fileName} failed.](). would be able to indicate a user (or the recipients) that there was more to send but it failed.

That reasoning makes sense to me!

If the webapp's behavior is that when the upload fails, you just have the [Uploading ${fileName}...]() placeholder stick around indefinitely, then I think this [Uploading ${fileName} failed.]() behavior would be an improvement there, too. Perhaps mention it in #frontend on chat.zulip.org?

We'll want the string being translated to be a constant string, with a placeholder in it for the filename. See docs/howto/translations.md, and search for "interpolating".

We'll also want to add the string to static/translations/messages_en.json; from there after the change is merged we'll sync it over to Transifex so that contributors there can translate it. See the next few paragraphs in the same doc.

Just a concern regarding this, docs/howto/translations.md says -

Translators will translate the constant string 'Hello, {name}', including the placeholder.

does that mean we also converts the filename variable to another language and if so shouldn't that remain preserved since it indicates the name of the file in user's device?

Ah, good question -- that explanation was ambiguous. Does this answer the question?
be39f56

We'll also want this to be translated. See previous comment for link to our docs on how.

same concern as above

this counts as a string concatenation which is denied in /docs/howto/translations.md

Never try to concatenate translated strings together, or do other string manipulation on them.

is this reasonable code? I kept it here since it only involves concatenation with newlines, which shouldn't create a problem according to me.

Thanks for asking. Yeah, this is fine as far as that's concerned -- what that's really meant to be about is attempting to put together a sentence with translated text, because the structure of the sentence will be different in different languages. I guess I should try to clarify that bit in the doc.

4bb2a4a

Thanks for asking. Yeah, this is fine as far as that's concerned -- what that's really meant to be about is attempting to put together a sentence with translated text, because the structure of the sentence will be different in different languages. I guess I should try to clarify that bit in the doc.

Instead of computing this a second time, how about having an array placeholders, so that here we can just say placeholders[i]?

Then we can still concatenate them for the insertMessageTextAtCursorPosition call, say with placeholders.join('\n\n').

The main advantage of computing them just once is that when we're doing it twice, it makes me worry a bit about whether they'll definitely come out the same both times. I'm pretty sure they always do with the code in this PR, but it's the sort of thing where it'd be very easy for them for a future change to the code to accidentally get them out of sync -- so, better to write the code so that structurally it makes sure they're the same.

made array of placeholders as well as fileNames. I also accidentally got the "replacement fileName" (attachment ${i+1}) out of sync in this very commit 😅 (you can see it in the code highlighted here) so this makes it a lot more relevant.
btw I fixed that as well and added a corresponding translation string for it.

Ha, indeed! And I didn't notice that either 😁 So that's a nice live demonstration of the point.

Let's make this try/catch be tightly scoped around just the await api.uploadFile line. That way the error handling in the catch block gets to be focused on a very specific way things can fail, which is the API call.

The fundamental difference between an exception from the API call and an exception from any of the rest of this code is that an exception from any of the rest would represent a bug in our code. But with an API call it's part of its interface that it might throw an exception for reasons that aren't a bug in anything, just the way the network is at that moment. Generally when throwing an exception is a normal part of the interface of something, we want to handle it explicitly right there, but for exceptions that would represent a bug we want to let them propagate up to very general handlers at the outer layers of the app.

One way in particular that having this larger try block would be bad, if a situation ever arises where it makes a difference, is that it would mean we had a bug somewhere in the code that's trying to replace the placeholder with the link text -- and we'd be suppressing the error here, so that we didn't get a report of it in Sentry to help us find and fix it.

I see, I was wondering why this specific code pattern --

try {
  mayThrowError()
} catch (e) {
  // handle error
  return;
}
// handle success

-- was present at some places, instead of just handling success inside the try block after mayThrowError. Infact I changed this in e6bcb0e (in #4514) thinking it was not necessary, which you also addressed in #4514 (comment). This makes sense.

Yep, exactly.

I've also now gone and expanded this to an entry in our style guide: d99ad3a

That reasoning makes sense to me!

If the webapp's behavior is that when the upload fails, you just have the [Uploading ${fileName}...]() placeholder stick around indefinitely, then I think this [Uploading ${fileName} failed.]() behavior would be an improvement there, too. Perhaps mention it in #frontend on chat.zulip.org?

Oh, in the commit message:

ComposeMenu: Refactor `IconFile` to `IconAttach`.

let's use a different verb instead of "refactor" -- like "Replace IconFile with IconAttach."

(Or perhaps better yet: "Use IconAttach instead of IconFile." I like having the summary line say first what the new code is doing, in preference to what the old code did -- usually that's the more important thing to know when going back through the history.)

The reason is that "refactor" to me means a change that's mainly about how the code is, rather than how it behaves to the user -- the paradigmatic case is a change that gives the code some very different structure, while having no visible effect at all on how the code behaves for the user. Whereas the point of this change is 100% about what the user sees.

Sure.

Oh, but a different translation-related remark:

Let's have the translated string here be just Uploading {fileName}..., and interpolate the result inside the [${…}]() markup syntax.

That way what the translators have to deal with is simpler -- no extra markup on the outside, which we'd be counting on them to keep verbatim and we don't have a great way to make that clear to them.

And on the other hand that's perfectly fine from the perspective of the question in the https://github.com/zulip/zulip-mobile/pull/4590/files#r622602182 thread, because the extra stuff we're adding is purely syntax from the markup language, not anything that's part of English or the user's natural language.

(Conversely, we do want to keep the ... inside the translated string, because that's punctuation and it might be expressed differently in another language.)

Sure, did these changes.

The new return in that catch block above causes me to notice a pitfall here: if anything in the function above this point exits the function early, this numUploading decrement won't run and we'll keep thinking there's an ongoing upload forever (until the user navigates out of the narrow so that this component gets discarded.)

In particular that will happen if the new return fires, but also in the previous revision it could happen if for some reason something inside that catch block (like the this.setMessageInputValue call, for example) were to throw an exception. (At the end of the branch, there's no longer the return but there's even more code in the function, so there are more opportunities for something to throw an exception.)

The way to fix this, in a way where it's easy to tell by looking at the code that it's definitely taken care of, is to put this decrement in a finally block. So the function might look like this:

  insertAttachment = async (attachments: DocumentPickerResponse[]) => {
    this.setState(({ numUploading }) => ({
      numUploading: numUploading + 1,
    }));
    try {
      // …
    } finally {
      this.setState(({ numUploading }) => ({
        numUploading: numUploading - 1,
      }));
    }
  };

It's a bit annoying because it causes the whole rest of the function to be indented -- really the ideal pattern here would be RAII, like in C++ or Rust. But this is probably the best approximation to that that we have in JS.