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

Add action indicator when resending a message. #6347

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@greenjoe
Contributor

greenjoe commented Mar 9, 2017

First time contributor checklist

Contributor checklist

  • Nexus 5x, Android 7.1.1
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax
  • I have made the choice whether I want the BitHub reward or not by omitting or adding the word FREEBIE in the commit message of my first commit

Description

When sending of message fails, user can go into details of that message and click RESEND button to retry. As issue #6307 mentions, after clicking that button nothing really indicates on the screen that message is being resent in the background.

This pull request hides the RESEND button when resending asynchronous task is started, as well as hides the "error_description" TextView and replaces it with the "action_description" TextView which says "Resending...". That should provide a nice indicator that clicking the RESEND button actually had an effect.

Add action indicator when resending a message.
When message is being resent, "RESEND" button
now disappears. "Resending..." text is displayed instead
of an error message.
Fixes #6307
@mammuth

This comment has been minimized.

Contributor

mammuth commented Mar 9, 2017

Cool! 👍
What happens if the sending fails another time? Becomes the resend-button visible again and the indicator (actionDescription) is removed?

@greenjoe

This comment has been minimized.

Contributor

greenjoe commented Mar 9, 2017

@mammuth Thanks, I've tested the case when sending fails multiple times as well as the case when it fails and then succeeds and it seems to work fine.

There is no explicit code to make the resend-button visible again and hide the actionDescription, but I believe that every time resending is finished the whole MessageRecipientListItem view is created again from scratch. Behavior that is now present in the master branch, which is to just make RESEND button disabled, also probably relies on the fact that the view will be created again (there is no code to re-enable the button).

I'm not an android developer by trade so I'm not 100% sure that my understanding is correct, but it looks like

  • MessageDetailsActivity creates instance of MessageDetailsLoader that loads message details
  • MessageDetailsActivity.onLoadFinished is called every time the resending task finishes and executes MessageRecipientAsyncTask
  • That task calls MessageDetailsActivity.updateRecipients which creates a new adapter recipientsList.setAdapter(new MessageDetailsRecipientAdapter(...)); and the adapter in turn creates new instances of MessageRecipientListItem class.

Please correct me if I'm wrong :-)

@@ -880,6 +880,7 @@
<!-- message_recipients_list_item -->
<string name="message_recipients_list_item__verify">VERIFY</string>
<string name="message_recipients_list_item__resend">RESEND</string>
<string name="message_recipients_list_item__resending">Resending&#8230;</string>

This comment has been minimized.

@McLoo

McLoo Mar 12, 2017

Contributor

IRRC we moved to ... instad of using &#8230; (just 3 string left)

This comment has been minimized.

@greenjoe

greenjoe Mar 12, 2017

Contributor

Well Android Studio suggested using the entity instead of three dots and I checked if the entity is used anywhere in the file - it was, so I used it too. Now I see that there are indeed more usages of three dots than of the entity. I can change it if it is important, but I believe it may be better to just do find and replace in one commit and change all the instances instead.

@moxie0

This comment has been minimized.

Member

moxie0 commented Mar 14, 2017

Did you try changing the button label and seeing how that looks instead of adding a new view that you swap?

@greenjoe

This comment has been minimized.

Contributor

greenjoe commented Mar 15, 2017

@moxie0
I believe that it looks better when using textview, especially when translation of "Resending" is long. I also think that there's no point displaying a button when it cannot be clicked and I believe it's nice if that red text disappears when you already took an action.

The screenshots from both options are below so you can judge it yourself (I'm sorry they are "reddish", I forgot to turn Twilight off).
The changes for the "simpler" version are here greenjoe@12b19bf and I can create a pull request with them if you like it more.

Textview (as in this pull request):
en-2a
Button label:
en-2b

And when translations are longer (used polish strings, I was too lazy to change language of the whole app)

Textview:
pl-2a
Button label:
pl-2b

@moxie0 moxie0 closed this in deb9664 Mar 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment