-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixed #4836 & #4840 Added the functionality of cancel upload and also solved the small bug of pausing upload #4843
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4843 +/- ##
============================================
+ Coverage 50.14% 50.57% +0.43%
- Complexity 2207 2225 +18
============================================
Files 338 338
Lines 15475 15546 +71
Branches 1348 1359 +11
============================================
+ Hits 7760 7863 +103
+ Misses 7141 7100 -41
- Partials 574 583 +9
Continue to review full report at Codecov.
|
@nicolas-raoul, please review this.... |
Hey @nicolas-raoul, is this working ? Or i have to change something ? |
I tested it yesterday and thought it was not working. Yesterday I thought that Pause was not working on the first tap, so I was always tapping it several times. Since pausing sometimes takes several seconds, how about showing a modal "Pausing..." popup so that people like me do not tap the button twice? Thanks a lot! |
Ok, will push a changes |
@nicolas-raoul, I tried to display the progress dialog but still the user have to wait for few seconds cause the progress bar still doing it's process even after the pausing, should i fix that progress bar in this PR ? |
Can you close the "Pausing..." popup just when the "Pause" button disappears? |
I think we should put a delay of 2 second, so that before closing pausing dialog, what do you think ? |
I don't think introducing delays is a good idea, as it might be too short or too long depending on the bandwidth and other factors. |
@nicolas-raoul, added the pausing pop up as you said.. but why this progress bar is showing it's progress for few seconds after the pausing it, i think it may be cause of the uploadWorker ? what do you think ? |
Yes, possibly. Making it stop immediately might be a solution, but may be more complex than just monitoring the worker. |
So, what changes required in this ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please showing a modal "Pausing..." popup that closes exactly when the "Pause" button disappears.
Thanks!
@nicolas-raoul, I think doing that, user will not be able to see the pop up |
@nicolas-raoul, done requested changes |
Strangely even with your new commit I am not seeing the popup. |
Yes, that's what i am trying to say, I think this heppen cause of the, setup of the upload Worker, cause the pause button just changing the state of the button, the actual pause is done by uploadWorker, which takes some time, and also before uploadWorker complete it's task, the pauseUpload completing it's task and returning which eventually calls the closing popup, In short, currently we are displaying the popup for just setting up the global variable and state, which don't require much time |
So, what do you think about this ? |
Hey @nicolas-raoul, finally it's working now, You can check test now, it's working better than ever Thanks... |
@@ -54,13 +56,18 @@ | |||
private final CompositeDisposable compositeDisposable = new CompositeDisposable(); | |||
private final MediaClient mediaClient; | |||
private boolean isWikipediaButtonDisplayed; | |||
private ProgressDialog pausingPopUp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Progress Dialog is deprecated, I suggest using the progress bar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I have to create a custom dialog with the progress bar, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try, I will tell in some time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make the progress dialog using alert dialog but in that case i also need to create another xml file, is it ok ?
Hey @madhurgupta10, successfully replaced progress Dialog with the progress Bar and I build it using the alert dialog. Can you please review now, Thanks. |
Hey @nicolas-raoul, can you please review this |
@nicolas-raoul, is anything needs to change ? |
@@ -135,7 +136,7 @@ public AppLanguageLookUpTable getLanguageLookUpTable() { | |||
ContributionDao contributionDao; | |||
|
|||
/** | |||
* In memory list of contributios whose uploads ahve been paused by the user | |||
* In memory list of contribution whose uploads have been paused by the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for the future: Thanks for fixing these! Please do not hesitate to make such changes in a separate pull request, so that this pull request does not contain unrelated changes :-)
By the way:
In memory -> In-memory
contribution -> contributions
app/src/main/java/fr/free/nrw/commons/contributions/ContributionViewHolder.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
/** | ||
* Update pause/resume button to show resume state | ||
*/ | ||
private void setResume() { | ||
pauseResumeButton.setImageResource(R.drawable.play_icon); | ||
pauseResumeButton.setTag(R.string.resume); | ||
pauseResumeButton.setTag("resume"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed?
ViewUtil.showShortToast(getContext(), R.string.cancelling_upload); | ||
contributionsListPresenter.deleteUpload(contribution); | ||
}, () -> { | ||
///do nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Do nothing.
@@ -389,7 +388,17 @@ public void retryUpload(final Contribution contribution) { | |||
|
|||
@Override | |||
public void deleteUpload(final Contribution contribution) { | |||
contributionsListPresenter.deleteUpload(contribution); | |||
DialogUtil.showAlertDialog(getActivity(), | |||
"Cancel Upload", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind reminding me what this string is used for, and why is does not need localization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this string is the title of dialog util, And I forgot to do localization
|
||
ContributionViewHolder(final View parent, final Callback callback, | ||
final MediaClient mediaClient) { | ||
super(parent); | ||
this.mediaClient = mediaClient; | ||
ButterKnife.bind(this, parent); | ||
this.callback = callback; | ||
|
||
// setting the pausingPopUp dialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Set a dialog indicating that the upload is being paused. This is needed because pausing an upload might take a dozen seconds.
Added the changes |
Hey @madhurgupta10, I am blocked to upload the picture, so how can I work on the bugs of the upload feature |
I think you can make an appeal to unblock your account, @nicolas-raoul could you please share more information on this? |
@arinmodi Are you blocked on beta or prod? For both you can appeal indeed. |
Hey @nicolas-raoul, is anything else requires ? |
@nicolas-raoul, changed a bit, please review this.... |
@nicolas-raoul, how can i appeal for unblock my account in beta version |
What is your username? "Anonymous me" does not seem to be blocked on beta: https://commons.wikimedia.beta.wmflabs.org/wiki/Special:Log?type=block&user=&page=User%3AAnonymous+me&wpdate=&tagfilter=&subtype= |
my user name Arin Modi |
Apparently you uploaded a meme. In the future, please only upload pictures you have taken yourself (even pictures of a spoon) :-) Thanks! I will try to get you unblocked. |
ok, I just uploaded by mistake |
Hey @nicolas-raoul, any updates on this PR ? |
Hey @nicolas-raoul, please review this |
Added the functionality of cancel upload and also solved the small bug of pausing upload
Fixes #4836 & #4840
What changes did you make and why?
I made the cancel button visible after the pause and also added confirmation dialog
For second issue to solve I change the id with string in setTag function of pauseResume button.
Tests performed
Tested build variant on ASUS_X00TD with API level 28