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 media upload progress. Closes #412 #426

Merged
merged 1 commit into from Oct 29, 2017

Conversation

Projects
None yet
3 participants
@charlag
Collaborator

charlag commented Oct 28, 2017

There's an upload progress shown on the media now.
Had some fun with merging with myself, I hope everything is okay (also couldn't help myself but formatted ComposeActivity).
photo_2017-10-29_00-06-23

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Oct 29, 2017

Member

Really nice, this is a feature a wanted for a long time too.
Some remarks:

  • please remove the logging of the progress value, it totally spams logcat
  • wouldn't a progress bar (circular or other) look better than a number?
  • the percent number is really small, if you stay with numbers please make them bigger
  • when the screen is rotated while the file is uploaded, the upload seems to restart? (maybe we should suppress recreation of the activity on config change android:configChanges="orientation|screenSize|keyboardHidden" in manifest?)
  • add the license information on top of your new files please

(while playing with the uploads, I think I might have found what the problem with #174 is - we need to increase the socket timeout)

Member

connyduck commented Oct 29, 2017

Really nice, this is a feature a wanted for a long time too.
Some remarks:

  • please remove the logging of the progress value, it totally spams logcat
  • wouldn't a progress bar (circular or other) look better than a number?
  • the percent number is really small, if you stay with numbers please make them bigger
  • when the screen is rotated while the file is uploaded, the upload seems to restart? (maybe we should suppress recreation of the activity on config change android:configChanges="orientation|screenSize|keyboardHidden" in manifest?)
  • add the license information on top of your new files please

(while playing with the uploads, I think I might have found what the problem with #174 is - we need to increase the socket timeout)

@moggers87

This comment has been minimized.

Show comment
Hide comment
@moggers87

moggers87 Oct 29, 2017

If you do stick with text rather than a progress bar/graphic, what are you doing to do about contrast? It looks fine in the screenshot, but I can't imagine it works too well with a nearly white image.

moggers87 commented Oct 29, 2017

If you do stick with text rather than a progress bar/graphic, what are you doing to do about contrast? It looks fine in the screenshot, but I can't imagine it works too well with a nearly white image.

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Oct 29, 2017

Collaborator

@moggers87 I've implemented exactly the same thing at work. The image is dimmed (colorFilter).
Anyway, I'm experimenting with drawing something like a circular progress, but I struggle with drawing non-full circle. @connyduck did you ever do it? I'm thinking about saveLayer (which is expensive) or about vector drawable with trimEnd

Collaborator

charlag commented Oct 29, 2017

@moggers87 I've implemented exactly the same thing at work. The image is dimmed (colorFilter).
Anyway, I'm experimenting with drawing something like a circular progress, but I struggle with drawing non-full circle. @connyduck did you ever do it? I'm thinking about saveLayer (which is expensive) or about vector drawable with trimEnd

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Oct 29, 2017

Member

@charlag We could take a library like this https://github.com/dinuscxj/CircleProgressBar
(or you can check out how they do it)

Member

connyduck commented Oct 29, 2017

@charlag We could take a library like this https://github.com/dinuscxj/CircleProgressBar
(or you can check out how they do it)

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Oct 29, 2017

Collaborator

@connyduck thanks for the lead. They're basically drawing full circle and then another non-full one on top of it. I cannot do it because I have to save the image below.
https://github.com/dinuscxj/CircleProgressBar/blob/master/circleprogressbar/src/main/java/com/dinuscxj/progressbar/CircleProgressBar.java#L313

Collaborator

charlag commented Oct 29, 2017

@connyduck thanks for the lead. They're basically drawing full circle and then another non-full one on top of it. I cannot do it because I have to save the image below.
https://github.com/dinuscxj/CircleProgressBar/blob/master/circleprogressbar/src/main/java/com/dinuscxj/progressbar/CircleProgressBar.java#L313

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Oct 29, 2017

Member

Ah yes I see the problem. What about overlaying a determinate ProgressBar? It won't be circular, but is that a problem?

Member

connyduck commented Oct 29, 2017

Ah yes I see the problem. What about overlaying a determinate ProgressBar? It won't be circular, but is that a problem?

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Oct 29, 2017

Collaborator

@connyduck oh, it would complicate things in other places and will be less efficient. If nothing will work I will resort to separate ProgressBar but I would like to stay with one view per image if possible.

Collaborator

charlag commented Oct 29, 2017

@connyduck oh, it would complicate things in other places and will be less efficient. If nothing will work I will resort to separate ProgressBar but I would like to stay with one view per image if possible.

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Oct 29, 2017

Member

Good look is worth an addidtional view imho, but you are the one implementing it, so you decide.
If you can't find an easy solution let us go with the percent for now

Member

connyduck commented Oct 29, 2017

Good look is worth an addidtional view imho, but you are the one implementing it, so you decide.
If you can't find an easy solution let us go with the percent for now

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Oct 29, 2017

Collaborator

So, I've updated things:

  • HttpLoggingInterceptor level is set to BASIC, upload progress logging is removed
  • I've implemented kind of a circular progress bar. Canvas#saveLayer() is not the cheapest operation but I couln't find another way (and it's still cheaper than another view). Also we can customize it in any way we want.
  • Please, please, don't do an android:configChanges. It's very tricky to handle it correctly. I say we either adopt some library for component, which survives (which also brings separation of concerns and testability) - like ViewModel from Support LIbrary or Mosby or whatever. The point is, Activities and Fragments are huge now. I'd go with ViewModel (and Rx but LiveData will do). In case we won't use anything we can use FragmentActivity#onRetainCustomNonConfigurationInstance() or retain a special fragment
  • Thanks for telling about license - I've completely overlooked it.
Collaborator

charlag commented Oct 29, 2017

So, I've updated things:

  • HttpLoggingInterceptor level is set to BASIC, upload progress logging is removed
  • I've implemented kind of a circular progress bar. Canvas#saveLayer() is not the cheapest operation but I couln't find another way (and it's still cheaper than another view). Also we can customize it in any way we want.
  • Please, please, don't do an android:configChanges. It's very tricky to handle it correctly. I say we either adopt some library for component, which survives (which also brings separation of concerns and testability) - like ViewModel from Support LIbrary or Mosby or whatever. The point is, Activities and Fragments are huge now. I'd go with ViewModel (and Rx but LiveData will do). In case we won't use anything we can use FragmentActivity#onRetainCustomNonConfigurationInstance() or retain a special fragment
  • Thanks for telling about license - I've completely overlooked it.
@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Oct 29, 2017

Member

Wow, great work.
Can you make the circle a little bit smaller (about 50%) and the line a tick (I think 1dp is enough) bigger so it resembles the native ProgressBar more?
Also I think circlePaint.setTextSize is not needed since you don't draw any text?

Member

connyduck commented Oct 29, 2017

Wow, great work.
Can you make the circle a little bit smaller (about 50%) and the line a tick (I think 1dp is enough) bigger so it resembles the native ProgressBar more?
Also I think circlePaint.setTextSize is not needed since you don't draw any text?

@charlag

This comment has been minimized.

Show comment
Hide comment
@charlag

charlag Oct 29, 2017

Collaborator

What about this? photo_2017-10-29_17-36-43

Collaborator

charlag commented Oct 29, 2017

What about this? photo_2017-10-29_17-36-43

@connyduck

This comment has been minimized.

Show comment
Hide comment
@connyduck

connyduck Oct 29, 2017

Member

👌

Member

connyduck commented Oct 29, 2017

👌

@connyduck connyduck merged commit 15e3757 into tuskyapp:master Oct 29, 2017

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