1177855 - Firefox Account profile images/avatars UI polish

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Vivek Balakrishnan[:vivek]]

This is bug to track the following improvements
1) Load the avatar in a neat rounded image
2) Cleanup profile fetch logic 
3) Add profile fetch to a background services may be to sync services

Comment 1

[Vivek Balakrishnan[:vivek]]

Attachment: part1.patch

Used picasso to download and show the avatar as preference icon.

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8626683 [details] [diff] [review]
part1.patch

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

Nifty!  This will be tricky to upstream to android-sync, because of Picasso, but I think we should go ahead and deal with that later.

Just some nits and small changes, please.

::: mobile/android/base/fxa/activities/FxAccountStatusFragment.java
@@ +654,5 @@
>        handler.removeCallbacks(profileFetchRunnable);
>      }
>  
> +    if (!profileJson.containsKey(FxAccountConstants.KEY_PROFILE_JSON_AVATAR)
> +            && !profileJson.containsKey(FxAccountConstants.KEY_PROFILE_JSON_USERNAME)) {

nit: && on first line, so the ! aligns.

@@ +675,5 @@
> +    }
> +
> +    // Icon update from java is not supported prior to API 11, skip the avatar image fetch and update for older device.
> +    if (!AppConstants.Versions.feature11Plus || TextUtils.isEmpty(avatarURI)) {
> +      Logger.info(LOG_TAG, "Skipping profile image fetch.");

Let's improve our lives later: separate checks, descriptive log messages.

@@ +679,5 @@
> +      Logger.info(LOG_TAG, "Skipping profile image fetch.");
> +      return;
> +    }
> +
> +    Picasso.with(getActivity())

Move .with onto a new line so all the '.'s align.

@@ +805,5 @@
>  
>    /**
> +   * A Target listener to update the profile avatar image.
> +   */
> +  private class ProfileAvatarTarget implements Target {

This is generally useful.  Extract it to a helper class (new file, named like PicassoPreferenceIconTarget) that takes all the parameters (fallback, corner radius, etc).

@@ +815,5 @@
> +    }
> +
> +    @Override
> +    public void onBitmapLoaded(Bitmap bitmap, Picasso.LoadedFrom from) {
> +      // Icon update from java is not supported prior to API 11

nit: capitalize Java, full sentence (trailing period).

::: mobile/android/base/resources/values/fxaccount_dimens.xml
@@ +26,5 @@
>  
>    <integer name="preference_fragment_scrollbarStyle">0x02000000</integer> <!-- outsideOverlay -->
> +
> +  <!-- Profile avatar image height. -->
> +    <dimen name="fxaccount_profile_target_height">48dp</dimen>

nit: indentation is looking funky here.

Why is this not called fxaccount_profile_avatar_image_height?  What does "target" mean?

Comment 3

[Vivek Balakrishnan[:vivek]]

Attachment: part1.patch

review nits addressed

Comment 4

[Vivek Balakrishnan[:vivek]]

Attachment: part2.patch

* Added profile fetch to SyncAdapter
* Added check for cache staleness

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1177855 - Partial review comments.

Bug 1177855 - Partial review comments.

* First, the new target needed to be generalized (no reference to
  profile) and was not handling corner radius correctly.  Please test
  that the new approach is correct!

* Second, we shouldn't be re-creating the Picasso Target frequently.
  Cache it and re-use it.

* Third, an AndroidFxAccount is transient, so it's not a good place to
  cache.  Drop the cached JSON entirely and just synchronize
  (statically, across instances) reading and writing to the
  AccountManager.

* Fourth, use FxAccountUtils.pii rather than Logger.pii.

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1177855: Fetch and show avatar image as preference icon -r=nalexander.

Bug 1177855: Fetch and show avatar image as preference icon -r=nalexander.

Comment 7

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1177855: Fetch and show avatar image as preference icon -r=nalexander.

Bug 1177855: Fetch and show avatar image as preference icon -r=nalexander.

Comment 8

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1177855 : Miscellaneous improvements related to profile avatar caching -r=nalexander.

Bug 1177855 : Miscellaneous improvements related to profile avatar caching -r=nalexander.

Comment 9

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1177855 : Miscellaneous improvements related to profile avatar caching -r=nalexander.

Bug 1177855 : Miscellaneous improvements related to profile avatar caching -r=nalexander.

Comment 10

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1177855 - simplified avatar fetch logic r?nalexander.

Bug 1177855 - simplified avatar fetch logic r?nalexander.

* Generalized PicassoPreferenceIcon implementation

* Cleaned up profile caching and simplified the complex profile fetch rate limiter logic

* profile always de-serialized from account manager rather from a cache.

* UI always updated asynchronously through broadcast after sync. A defaul icon is used as
  fallback during the on going sync.

Comment 11

[Nick Alexander :nalexander [he/him]]

url:        https://hg.mozilla.org/integration/fx-team/rev/1aa1e439591173468690fe2868574a033a7d223d
changeset:  1aa1e439591173468690fe2868574a033a7d223d
user:       vivek <[e-mail]>
date:       Tue Jun 30 21:09:44 2015 -0700
description:
Bug 1177855: Fetch and show avatar image as preference icon. r=nalexander

The profile JSON is stored in the Account bundle.  There's no need to
bump the bundle version, since missing (i.e., null) profile JSON is
legal.

This introduces and uses a general-purpose PicassoPreferenceIcon
Picasso Target that, on API 11+ devices, dynamically loads a
preference icon.

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8627933 [details]
MozReview Request: Bug 1177855: Fetch and show avatar image as preference icon -r=nalexander.

https://reviewboard.mozilla.org/r/12341/#review10821

Ship It!

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8627935 [details]
MozReview Request: Bug 1177855 : Miscellaneous improvements related to profile avatar caching -r=nalexander.

https://reviewboard.mozilla.org/r/12343/#review10823

Ship It!

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8627937 [details]
MozReview Request: Bug 1177855 - simplified avatar fetch logic r?nalexander.

https://reviewboard.mozilla.org/r/12345/#review10825

Ship It!

Comment 15

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/12341/#review10827

Ship It!

Comment 16

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/12343/#review10829

Ship It!

Comment 17

[Nick Alexander :nalexander [he/him]]

mozreview has complete lost it, so I can't clear the remaining flags.  In any case, I reviewed this and landed with some further simplifications.  vivek: good work!  On to the next!

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1aa1e4395911