Closed Bug 791475 Opened 7 years ago Closed 6 years ago

Android updater notification should provide more information like progress

Categories

(Firefox for Android :: General, enhancement)

ARM
Android
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: aryx, Assigned: mtp.boon)

References

Details

(Whiteboard: [Updater] [mentor=snorp][lang=java])

Attachments

(2 files, 1 obsolete file)

The Android service-based updater implemented in bug 786380 should at least show the progress so the user can see when it will be done/he can restart for getting latest version. Time remaining and download size are two other things which should be evaluated.
Whiteboard: [Updater]
Snorp, do you think this would be a good mentor bug?
Flags: needinfo?(snorp)
(In reply to :Margaret Leibovic from comment #2)
> Snorp, do you think this would be a good mentor bug?

Definitely!
Flags: needinfo?(snorp)
Whiteboard: [Updater] → [Updater] [mentor=snorp][lang=java]
Duplicate of this bug: 902479
Hey, I'd like to have a try at this please.
Assignee: nobody → mtp.boon
Status: NEW → ASSIGNED
Attached patch 791475.patch (obsolete) — Splinter Review
How's this looking at the moment?

Originally there was talk of time remaining and filesize but with the current "Touch here to apply patch when it's downloaded" message there's not really room for this.

I seem to remember snorp talking about improving the flow of this as a future bug and making it so you don't need to do this. This would then free up some space for a filesize / time remaining. Thoughts?
Attachment #799044 - Flags: feedback?(snorp)
Comment on attachment 799044 [details] [diff] [review]
791475.patch

Review of attachment 799044 [details] [diff] [review]:
-----------------------------------------------------------------

Looking really good!

::: mobile/android/base/UpdateService.java
@@ +72,5 @@
>      private SharedPreferences mPrefs;
>  
>      private NotificationManager mNotificationManager;
>      private ConnectivityManager mConnectivityManager;
> +    private Builder mBuilder; 

Whitespace

@@ +119,5 @@
>              registerForUpdates(false);
>          } else if (UpdateServiceHelper.ACTION_CHECK_FOR_UPDATE.equals(intent.getAction())) {
>              startUpdate(intent.getIntExtra(UpdateServiceHelper.EXTRA_UPDATE_FLAGS_NAME, 0));
> +        	// Use this instead for forcing a download from about:fennec
> +        	// startUpdate(UpdateServiceHelper.FLAG_FORCE_DOWNLOAD | UpdateServiceHelper.FLAG_REINSTALL);

Fix indentation

@@ +380,1 @@
>          

Whitespace

@@ +382,5 @@
> +        mBuilder.setContentTitle(getResources().getString(R.string.updater_downloading_title))
> +    	    .setContentText(mApplyImmediately ? "" : getResources().getString(R.string.updater_downloading_select))
> +    	    .setSmallIcon(android.R.drawable.stat_sys_download)
> +    	    .setContentIntent(contentIntent);
> +    	

Whitespace

@@ +383,5 @@
> +    	    .setContentText(mApplyImmediately ? "" : getResources().getString(R.string.updater_downloading_select))
> +    	    .setSmallIcon(android.R.drawable.stat_sys_download)
> +    	    .setContentIntent(contentIntent);
> +    	
> +        mBuilder.setProgress(100, 0, false);

It might actually be a good idea to show indeterminate progress here until we actually start the download?

@@ +404,5 @@
>      }
>  
>      private File downloadUpdatePackage(UpdateInfo info, boolean overwriteExisting) {
>          File downloadFile = new File(Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS), new File(info.url.getFile()).getName());
> +        

Whitespace :)
Attachment #799044 - Flags: feedback?(snorp) → feedback+
Attached patch 791475v2.patchSplinter Review
Made changes and also checked that android.support.v4.app.NotificationCompat is OK to use.
Attachment #799044 - Attachment is obsolete: true
Attachment #805407 - Flags: review?(snorp)
Comment on attachment 805407 [details] [diff] [review]
791475v2.patch

Review of attachment 805407 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, this is looking good!

Can you attach a screenshot of what this looks like in action? We should get design to approve that before putting it in, I think.
Attachment #805407 - Flags: review?(snorp) → review+
Attached image screenshot_791475.png
Sure, think this should do.

I'm assuming there's someone from design that I should have put in the "need more info from" section for this but couldn't find anyone to ask, sorry!
Looks great! Ian Barlow is our drsign guy, I cced him. I'm sure he'll chime in.
Guys, this is really sweet. As a user it's a super great improvement over the actual state of the indicator. GOOD WORK!
This looks great, thanks for the patch! The only thing I could think to change about it would be displaying the percentage complete as well as the progress meter. That's probably only a small tweak on top of what you have though, so we could file a followup on that.
(In reply to Michael Boon from comment #10)
> Created attachment 807492 [details]
> screenshot_791475.png
> 
> Sure, think this should do.
> 
> I'm assuming there's someone from design that I should have put in the "need
> more info from" section for this but couldn't find anyone to ask, sorry!

Nailed it. Thanks Michael :)
https://hg.mozilla.org/integration/fx-team/rev/1dde3e9a04c4
Keywords: checkin-needed
Whiteboard: [Updater] [mentor=snorp][lang=java] → [Updater] [mentor=snorp][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1dde3e9a04c4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Updater] [mentor=snorp][lang=java][fixed-in-fx-team] → [Updater] [mentor=snorp][lang=java]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.