-
-
Notifications
You must be signed in to change notification settings - Fork 292
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
Refactor the VariantEmojiManager #165
Refactor the VariantEmojiManager #165
Conversation
Codecov Report
@@ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good on first glance, I'll also test this locally later
.setOnEmojiClickedListener(new OnEmojiClickedListener() { | ||
@Override public void onEmojiClicked(@NonNull final Emoji emoji) { | ||
.setOnEmojiClickListener(new OnEmojiClickListener() { | ||
@Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you move it onto the same line as below?
.setOnEmojiClickedListener(new OnEmojiClickedListener() { | ||
@Override public void onEmojiClicked(@NonNull final Emoji emoji) { | ||
.setOnEmojiClickListener(new OnEmojiClickListener() { | ||
@Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
} | ||
|
||
@Override protected void onDetachedFromWindow() { | ||
@Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same line as below
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be @nonnull right since you create it initially in the Popup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dismiss(); | ||
|
||
this.rootImageView = clickedImage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove this.
@@ -57,6 +60,8 @@ void show(@NonNull final View clickedImage, @NonNull final Emoji emoji) { | |||
} | |||
|
|||
void dismiss() { | |||
this.rootImageView = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
Already fixed, working on some checkstyle issues then I'll push that change also. |
Done 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question about visibility. Also can you make the class final?
* | ||
* @param emoji The new emoji variant to show. | ||
*/ | ||
void updateEmoji(@NonNull final Emoji emoji) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be public? otherwise you can't access it when you want to do your custom implementation
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be public? otherwise you can't access it when you want to do your custom implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay all good. I somehow mixed something up.
Thanks Ruben :)
This should avoids leaks and improve the architecture. The behaviour is not changed. Would fix #164.
Code changes to achieve this
interface
VariantEmoji
similar toRecentEmoji
. This is exposed through thebuilder
and can be implemented by users for saving somewhere else than theSharedPreferences
.VariantEmojiManager
is the default implementation of that, similar to theRecentEmojiManager
.VariantEmojiManager
is not a singleton anymore. This leads to passing the object around anywhere, but thats better than a singleton.EmojiImageView
now gets updated on click, instead of implicitely by a listener. This is better as it leads to lower coupling.EmojiOnClickListener
has been extended to also have theEmojiImageView
as a parameter. I needed that for updating the view properly, but it's also consistent with Android's listeners, which also have theView
as parameter.