Refactor the VariantEmojiManager #165

rubengees · 2017-07-01T16:11:23Z

This should avoids leaks and improve the architecture. The behaviour is not changed. Would fix #164.

Code changes to achieve this

New interface VariantEmoji similar to RecentEmoji. This is exposed through the builder and can be implemented by users for saving somewhere else than the SharedPreferences. VariantEmojiManager is the default implementation of that, similar to the RecentEmojiManager.
VariantEmojiManager is not a singleton anymore. This leads to passing the object around anywhere, but thats better than a singleton.
The EmojiImageView now gets updated on click, instead of implicitely by a listener. This is better as it leads to lower coupling.
Various renamings and smaller clean ups. I thought this might be a good idea; We already have breaking changes, so now is the time to clean up 😁.
- The EmojiOnClickListener has been extended to also have the EmojiImageView as a parameter. I needed that for updating the view properly, but it's also consistent with Android's listeners, which also have the View as parameter.

codecov · 2017-07-01T16:28:51Z

Codecov Report

Merging #165 into master will decrease coverage by 0.84%.
The diff coverage is 38.67%.

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   30.01%   29.16%   -0.85%     
==========================================
  Files          21       21              
  Lines         753      744       -9     
  Branches       82       82              
==========================================
- Hits          226      217       -9     
- Misses        511      513       +2     
+ Partials       16       14       -2

vanniktech

looks good on first glance, I'll also test this locally later

vanniktech · 2017-07-01T19:20:08Z

app/src/main/java/com/vanniktech/emoji/sample/MainActivity.java

-        .setOnEmojiClickedListener(new OnEmojiClickedListener() {
-          @Override public void onEmojiClicked(@NonNull final Emoji emoji) {
+        .setOnEmojiClickListener(new OnEmojiClickListener() {
+          @Override


nit: can you move it onto the same line as below?

vanniktech · 2017-07-01T19:20:17Z

app/src/main/java/com/vanniktech/emoji/sample/MainDialog.java

-            .setOnEmojiClickedListener(new OnEmojiClickedListener() {
-              @Override public void onEmojiClicked(@NonNull final Emoji emoji) {
+            .setOnEmojiClickListener(new OnEmojiClickListener() {
+              @Override


vanniktech · 2017-07-01T19:21:23Z

emoji/src/main/java/com/vanniktech/emoji/EmojiImageView.java

-  }
-
-  @Override protected void onDetachedFromWindow() {
+  @Override


nit: same line as below

vanniktech · 2017-07-01T19:23:28Z

emoji/src/main/java/com/vanniktech/emoji/EmojiArrayAdapter.java

 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;

 import static com.vanniktech.emoji.Utils.checkNotNull;

 final class EmojiArrayAdapter extends ArrayAdapter<Emoji> {
-  @Nullable private final OnEmojiClickedListener listener;
-  @Nullable private final OnEmojiLongClickedListener longListener;
+  @Nullable private final VariantEmoji variantManager;


Should be @nonnull right since you create it initially in the Popup

No, the EmojiArrayAdapter of the recent tab gets null passed in for the variantManager, because we do not want to show what the variantManager returns, but that what the recentManager returns.

Yes, this is a bit hacky, we could also have a dedicated EmojiArrayAdapter for that, but I thought that would be a bit of overkill.

vanniktech · 2017-07-01T19:25:33Z

emoji/src/main/java/com/vanniktech/emoji/EmojiVariantPopup.java

    dismiss();

+    this.rootImageView = clickedImage;


nit: remove this.

vanniktech · 2017-07-01T19:25:37Z

emoji/src/main/java/com/vanniktech/emoji/EmojiVariantPopup.java

@@ -57,6 +60,8 @@ void show(@NonNull final View clickedImage, @NonNull final Emoji emoji) {
  }

  void dismiss() {
+    this.rootImageView = null;


nit: remove this.

vanniktech

Formatting looks good now although now we don't have the optimization anymore (of setting the image resource directly after changing the skin tone) which yields to the lag again. (Wasn't there previously). I'd suggest to keep that behavior. Other than that everything looks really good 👍

rubengees · 2017-07-01T19:58:03Z

Already fixed, working on some checkstyle issues then I'll push that change also.

rubengees · 2017-07-01T20:05:55Z

Done 👍

vanniktech

One question about visibility. Also can you make the class final?

vanniktech · 2017-07-01T20:10:09Z

emoji/src/main/java/com/vanniktech/emoji/EmojiImageView.java

+   *
+   * @param emoji The new emoji variant to show.
+   */
+  void updateEmoji(@NonNull final Emoji emoji) {


should this be public? otherwise you can't access it when you want to do your custom implementation

vanniktech · 2017-07-01T20:10:12Z

emoji/src/main/java/com/vanniktech/emoji/EmojiImageView.java

@@ -88,7 +88,7 @@ public EmojiImageView(final Context context, final AttributeSet attrs) {
    }
  }

-  public void setEmoji(@NonNull final Emoji emoji) {
+  void setEmoji(@NonNull final Emoji emoji) {


should this be public? otherwise you can't access it when you want to do your custom implementation

This is not supposed to be overwritten, the class itself is only public, because of the package structure (it's used in some listeners, which reside in a different package). For that reason it has the @RestrictTo(LIBRARY) annotation.

rubengees · 2017-07-01T20:22:01Z

EmojiImageView is final or do you mean another class?

vanniktech

Okay all good. I somehow mixed something up.

Thanks Ruben :)

rubengees added 3 commits July 1, 2017 17:58

Refactor the VariantEmojiManager to avoid leaks and improve architecture

c535f53

Formatting for imports

d8ff9e8

Some clean ups

2447c10

vanniktech requested changes Jul 1, 2017

View reviewed changes

rubengees added 2 commits July 1, 2017 21:35

Merge branch 'master' into refactor-variant-manager

ee1d852

More formatting

8654e01

vanniktech requested changes Jul 1, 2017

View reviewed changes

Eliminate lag when updating the displayed variant in the EmojiImageView

74e3c4a

vanniktech reviewed Jul 1, 2017

View reviewed changes

vanniktech approved these changes Jul 1, 2017

View reviewed changes

vanniktech merged commit e15e896 into vanniktech:master Jul 1, 2017

rubengees deleted the refactor-variant-manager branch July 1, 2017 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the VariantEmojiManager #165

Refactor the VariantEmojiManager #165

rubengees commented Jul 1, 2017 •

edited

Loading

codecov bot commented Jul 1, 2017 •

edited

Loading

vanniktech left a comment

vanniktech Jul 1, 2017

vanniktech Jul 1, 2017

vanniktech Jul 1, 2017

vanniktech Jul 1, 2017

rubengees Jul 1, 2017

vanniktech Jul 1, 2017

vanniktech Jul 1, 2017

vanniktech left a comment

rubengees commented Jul 1, 2017

rubengees commented Jul 1, 2017

vanniktech left a comment

vanniktech Jul 1, 2017

vanniktech Jul 1, 2017

rubengees Jul 1, 2017

rubengees commented Jul 1, 2017

vanniktech left a comment

Refactor the VariantEmojiManager #165

Refactor the VariantEmojiManager #165

Conversation

rubengees commented Jul 1, 2017 • edited Loading

Code changes to achieve this

codecov bot commented Jul 1, 2017 • edited Loading

Codecov Report

vanniktech 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanniktech left a comment

Choose a reason for hiding this comment

rubengees commented Jul 1, 2017

rubengees commented Jul 1, 2017

vanniktech 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

rubengees commented Jul 1, 2017

vanniktech left a comment

Choose a reason for hiding this comment

rubengees commented Jul 1, 2017 •

edited

Loading

codecov bot commented Jul 1, 2017 •

edited

Loading