Show avatar with username initials. #372

philipbrito · 2017-08-01T13:12:43Z

@RocketChat/android

Closes #369

Since it is a helper, we don't need to instantiate the RocketChatUserAvatar to each use. This commit adds methods to get a drawable with username initials.

rafaelks · 2017-08-01T13:16:07Z

app/src/main/java/chat/rocket/android/helper/RocketChatUserAvatar.kt

+
+        return TextDrawable.builder()
+                .beginConfig()
+                .useFont(Typeface.SANS_SERIF)


Will it use the default one from the OS?

It will be rendered as expected on Application, since the Typeface was added by the Android platform since API level 1.

rafaelks · 2017-08-01T13:17:54Z

app/src/main/java/chat/rocket/android/helper/RocketChatUserAvatar.kt

+            0xFFF44336.toInt(), 0xFFE91E63.toInt(), 0xFF9C27B0.toInt(), 0xFF673AB7.toInt(), 0xFF3F51B5.toInt(),
+            0xFF2196F3.toInt(), 0xFF03A9F4.toInt(), 0xFF00BCD4.toInt(), 0xFF009688.toInt(), 0xFF4CAF50.toInt(),
+            0xFF8BC34A.toInt(), 0xFFCDDC39.toInt(), 0xFFFFC107.toInt(), 0xFFFF9800.toInt(), 0xFFFF5722.toInt(),
+            0xFF795548.toInt(), 0xFF9E9E9E.toInt(), 0xFF607D8B.toInt())


What do you think about creating unit tests for this class? You could test a few example names with a few different usernames. On iOS I tested a bunch of different cases: https://github.com/RocketChat/Rocket.Chat.iOS/blob/develop/Rocket.ChatTests/Views/AvatarViewSpec.swift.

Of course! I'll do unit tests! Thank you guy for putting those thoughts.

luciofm · 2017-08-02T12:09:45Z

app/src/main/java/chat/rocket/android/helper/OkHttpHelper.java

+        }
+      }
+    });
+    return contentType;


This most likely will be null, since you are doing an Asynchronous request.

To get the contentType we need to do a network call - blocking operation, so we need to do an async task, right? There are the following possibilities:

Get the contentType using the HttpURLConnection with the AsyncTask.

Get the contentType using the HttpURLConnection with RxJava.

Get the contentType using the OkHttp which is currently used on RC app - OkHttp is a built-in response cache, a better cookie model, a better headers model and a better call model.

So, how we should handle this requirement?

We should not do this, if we really need to get the Content-Type, we will need to implement our own Image loading library, like Fresco or Picasso, to download, cache and display it...

Doing a lot of HEADs here will make a lot of useless requests on fling, we are not caching the result...

Ideally this should be handled on the Image library, on Fresco, or switch to some that already supports SVG.

luciofm · 2017-08-02T12:13:18Z

app/src/main/java/chat/rocket/android/helper/RocketChatUserAvatar.kt

+     * @see getUri
+     */
+    fun getImageFormat(uri: String): String {
+        return OkHttpHelper.getContentType(uri)


We should have this information from the backend, each time we show an Avatar doing a HEAD (network operation) operation and a GET seems very expensive.

Yes, I pay attention w/ it, so to get the image format the requested method is HEAD and not GET. Take a look.

https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html:

The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response. The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request. This method can be used for obtaining metainformation about the entity implied by the request without transferring the entity-body itself. This method is often used for testing hypertext links for validity, accessibility, and recent modification.

luciofm · 2017-08-02T12:15:50Z

app/src/main/java/chat/rocket/android/helper/OkHttpHelper.java

  private static OkHttpClient httpClientForUploadFile;
  private static OkHttpClient httpClientForDownloadFile;
  private static OkHttpClient httpClientForWS;
+  private static String contentType;


This will not work as expected...

First time you call getContentType() it will probably return null, since it is a async request.
Next time you call it, you will probably get the contentType from the previous request, which may be right or not.

On a list flinging, there will be dozens of requests overwriting contentType

First time you call getContentType() it will probably return null, since it is a async request.

See the comment below.

On a list flinging, there will be dozens of requests overwriting contentType

I agree w/ you. There is a way to avoid it, but it involves involves a disabled support. See this comment.

luciofm · 2017-08-02T12:19:17Z

app/src/main/java/chat/rocket/android/helper/RocketChatUserAvatar.kt

+     * @param username The username.
+     * @return A string with username initials.
+     */
+    fun getUsernameInitials(username: String): String {


The function doesn't conform with the documentation, it handles only separation by "." and not by spaces

@luciofm What did you mean?

Yes, the javadoc (for kotlin 😄) says one thing and the function does another..

luciofm · 2017-08-02T12:20:06Z

app/src/main/java/chat/rocket/android/layouthelper/chatroom/AbstractMessageViewHolder.java

@@ -14,6 +14,7 @@
 public abstract class AbstractMessageViewHolder extends ModelViewHolder<PairedMessage> {
  protected final RocketChatAvatar avatar;
  protected final ImageView userNotFoundAvatarImageView;
+  protected final ImageView imageViewSvgUserAvatar;


Do we need to use a separate ImageView? This should probably be handled on RocketChatAvatar.

We need, because RocketChatAvatar is a class which returns a SimpleDraweeView. There is a setImageDrawable() method on SimpleDraweeView but it is deprecated (trying to use it you'll get the messed avatars). Also, from Fresco:

Don’t set images directly on a DraweeView
Currently DraweeView is a subclass of Android’s ImageView. This has various methods to set an image (such as setImageBitmap, setImageDrawable)
If you set an image directly, you will completely lose your DraweeHierarchy, and will not get any results from the image pipeline.

My suggestion is to work with Fresco, and implement support for SVG directly on it...

I mean, adding SVG support for Fresco

luciofm · 2017-08-02T12:21:43Z

app/src/main/java/chat/rocket/android/renderer/MessageRenderer.kt

                userNotFoundAvatarImageView.visibility = View.GONE
-                rocketChatAvatarWidget.visibility = View.VISIBLE
+                val userAvatarUri = RocketChatUserAvatar.getUri(hostname, username)
+                val userAvatarImageContentType = OkHttpHelper.getContentType(userAvatarUri)


As commented before, this will not work consistently.

rafaelks · 2017-08-02T14:22:38Z

@filipedelimabrito Why don't you implement it the way iOS does?

Here's the structure of the AvatarView, basically:

-> UIView (the avatar view itself, root)
     -> ViewInitials (view with the initials, manually created, always present)
     -> ImageView (imageView with avatar, after loaded and cached)

This way, while the image is loading the imageView is transparent and the ViewInitials is the one that's being present to the user. If ImageView is loaded/cached, it will show the ImageView instead of ViewInitials.

philipbrito · 2017-08-02T14:54:30Z

That's I said before to you @rafaelks. We can set a placeholder image w/ username initials and if the image from backend is loaded successful then we show it. Note that the images w/ username initials are loaded from the backend, but it isn't build correctly - until now (see RocketChat/Rocket.Chat#7572). As the chatroom background is white we can't see the username initials which are white too (the background isn't load because the SVG is build in a wrong way).

We are supporting the SVG rendering on RC app (I added it on some PR times ago). We simply can disable the SVG support to set the avatar w/ username initials and Fresco will do the rest (loading images that are not in SVG format, and setting the placeholder w/ username initials when the images are in SVG format - or, when the loading from server fails).

See this comment also, can be useful.

…mabrito/Rocket.Chat.Android into avatar-with-username-initials

rafaelks · 2017-08-03T00:22:51Z

rocket-chat-android-widgets/src/main/java/chat/rocket/android/widget/RocketChatWidgets.java

@@ -26,7 +31,7 @@ public static void initialize(Context context, OkHttpClient okHttpClient) {
    ImagePipelineConfig imagePipelineConfig = OkHttpImagePipelineConfigFactory
        .newBuilder(context, okHttpClient)
        .setRequestListeners(listeners)
-        .setImageDecoderConfig(CustomImageFormatConfigurator.createImageDecoderConfig())
+//        .setImageDecoderConfig(CustomImageFormatConfigurator.createImageDecoderConfig())


Can we remove this commented code?

If we need to plan to support SVG image in the future I think that we could leave this commented code. If not, we can remove it (and the CustomImageFormatConfigurator and SvgDecoder class too).

We have github... We can always look the code history to recover this...

rafaelks · 2017-08-03T00:23:10Z

...dgets/src/main/java/chat/rocket/android/widget/message/autocomplete/user/UserViewHolder.java

@@ -16,6 +20,12 @@
  private final RocketChatAvatar avatar;
  private final ImageView status;

+  private static final int[] COLORS = new int[] {


Why are you repeating the colors here?

Yes, for the same reason bellow.

:(

rafaelks · 2017-08-03T00:23:38Z

...dgets/src/main/java/chat/rocket/android/widget/message/autocomplete/user/UserViewHolder.java

+        .buildRoundRect(getUsernameInitials(username), getUserAvatarBackgroundColor(username), round);
+  }
+
+  private String getUsernameInitials(String username) {


This method seems to be duplicated...?

Yes :(
I just put it as before (w/ some the correct modifications). This class belongs to another module, there is the possibility to add the :app dependency to this module to avoid that duplication, but I really don't know if this is the correct (in the terms of design, architecture...). So I just did like before.

I actually think the widgets should live on the app module... unless they are meant to be reusable, them they should live on another repository...

rafaelks

Just small change in your method docs.

rafaelks · 2017-08-03T11:47:23Z

rocket-chat-android-widgets/src/main/java/chat/rocket/android/widget/helper/UserAvatarHelper.kt

+    }
+
+    /**
+     * Returns a string with the username initials. For example: username John Doe returns JD initials.


I think you should change this docs to something that will work. In your method John Doe will not become JD.

luciofm · 2017-08-03T12:55:05Z

rocket-chat-android-widgets/src/test/kotlin/UserAvatarHelperTest.kt

@@ -0,0 +1,22 @@
+import chat.rocket.android.widget.helper.UserAvatarHelper


Tests should live in the same package (and directory structure) as the class you are testing...

luciofm · 2017-08-03T12:55:46Z

rocket-chat-android-widgets/src/main/res/layout/message_avatar.xml

Format it with 4 spaces indentation, please.

rafaelks · 2017-08-03T14:18:38Z

rocket-chat-android-widgets/src/main/java/chat/rocket/android/widget/helper/UserAvatarHelper.kt

@@ -58,7 +58,7 @@ object UserAvatarHelper {
    }

    /**
-     * Returns a string with the username initials. For example: username John Doe returns JD initials.
+     * Returns a string with the username initials. For example: username John.Doe returns JD initials.


rafaelks · 2017-08-03T14:18:52Z

LGTM, what about you @luciofm?

luciofm · 2017-08-03T19:31:40Z

app/src/main/java/chat/rocket/android/helper/OkHttpHelper.java

@@ -48,4 +63,28 @@ public static OkHttpClient getClientForWebSocket() {
    }
    return httpClientForWS;
  }
+
+  public static String getContentType(String uri) {


Can you remove this method that is not used anymore.

philipbrito added 6 commits August 1, 2017 09:56

Add getContentType(...) and getDefaultHttpClient() method.

4d84c1e

Update RocketChatUserAvatar.

4ec4fff

Since it is a helper, we don't need to instantiate the RocketChatUserAvatar to each use. This commit adds methods to get a drawable with username initials.

Add an imageView to be used with avatars in svg format.

f9af22f

Update UserRenderer.kt

ae82341

Update MessageRenderer.kt

d794b5b

Update RocketChatAvatar.java

3b24532

rafaelks reviewed Aug 1, 2017

View reviewed changes

philipbrito added the 🏗️ work in progress label Aug 1, 2017

rafaelks and others added 6 commits August 1, 2017 10:45

Merge branch 'develop' into avatar-with-username-initials

70edac3

Update Kotlin version

a891e28

Add tests dependencies and Kotlin tests directories

ed0f72d

Update RocketChatUserAvatar.kt

c54a414

Add RocketChatUserAvatarTest.kt

cfc8d79

Update method name declaration.

281ac8a

luciofm suggested changes Aug 2, 2017

View reviewed changes

rafaelks and others added 8 commits August 2, 2017 16:12

Merge branch 'develop' into avatar-with-username-initials

bc12df1

Update RocketChatUserAvatar.kt

4f636c2

Delete image_svg_user_avatar/imageViewSvgUserAvatar

f35ab14

Add placeholder image.

96f503e

Update message_avatar.xml

52a0d83

Disable SVG support.

827ada1

Delete blank line.

60074da

Merge branch 'avatar-with-username-initials' of github.com:filipedeli…

1efd2a1

…mabrito/Rocket.Chat.Android into avatar-with-username-initials

rafaelks reviewed Aug 3, 2017

View reviewed changes

Delete RocketChatUserAvatar and RocketChatUserAvatarTest class.

8b18a0f

philipbrito added 7 commits August 2, 2017 23:12

Update build.gradle

3756ee1

Add UserAvatarHelper and UserAvatarHelperTest class.

1cdd442

Delete classes.

5efbd8e

Update RocketChatWidgets.java

4f3302b

Update FrescoHelper class.

3e43f04

Update UserViewHolder class.

8471796

Update classes.

5390166

philipbrito removed the 🏗️ work in progress label Aug 3, 2017

rafaelks suggested changes Aug 3, 2017

View reviewed changes

luciofm reviewed Aug 3, 2017

View reviewed changes

philipbrito added 3 commits August 3, 2017 11:01

Update UserAvatarHelper.kt

c9975c1

Update message_avatar.xml

d07d6f9

Update package.

7a81eb1

rafaelks reviewed Aug 3, 2017

View reviewed changes

rafaelks approved these changes Aug 3, 2017

View reviewed changes

RocketChat deleted a comment from rafaelks Aug 3, 2017

luciofm reviewed Aug 3, 2017

View reviewed changes

Delete unused methods.

9b37d89

luciofm approved these changes Aug 3, 2017

View reviewed changes

luciofm merged commit 8ac51b5 into RocketChat:develop Aug 3, 2017

		@@ -0,0 +1,22 @@
		import chat.rocket.android.widget.helper.UserAvatarHelper

Show avatar with username initials. #372

Show avatar with username initials. #372

Conversation

philipbrito commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelks commented Aug 2, 2017

philipbrito commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelks commented Aug 3, 2017

Choose a reason for hiding this comment