Fix Emoji keyboard in certain cases #329

mario · 2019-01-24T01:55:34Z

This fixes an issue brought to my attention in #328 and a few other issues I've noticed with the Popup view.

rubengees

I tested this in a real application and it doesn't work quite right in landscape. The popup is now way to large, as you can see in the screenshot. Note that I do not have set adjustResize.

Apart from that I added some questions, since I can't really follow the logic anymore...

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

mario · 2019-01-25T21:16:59Z

I'll investigate the sizing when NOT using adjustResize.

mario · 2019-01-25T21:39:00Z

Can you let me know the combination you use for activity + edittext (imeOptions) so I can try to debug as I can't really reproduce? (the emoji panel is the same size as keyboard for me)

mario · 2019-01-25T22:16:44Z

@rubengees when you get the time to answer, I'll take a look. Got another PR in the mean time :)

rubengees · 2019-01-25T22:23:44Z

On my Activity I have no special properties (so no adjustResize), the EmojiEditText is like this (simplified):

<com.vanniktech.emoji.EmojiEditText
    android:layout_width="match_parent"
    android:layout_height="wrap_content"
    android:inputType="textMultiLine|textCapSentences|textAutoCorrect"
    android:maxLines="5"
    android:scrollbars="vertical" />

mario · 2019-01-26T21:37:07Z

I now came up with a significantly better solution and have rebased the PR against master. Please test and reply accordingly - thanks!

mario · 2019-01-26T22:29:31Z

After this is merged, I'd appreciate a release as well so I can remove the commit from my build.gradle ;)

This, after all, should solve a lot of layout issues people faced in the past.

mario · 2019-01-27T00:15:21Z

After further testing, I can confirm (and be happy about it!) that the bug @rubengees reported is gone, so I have you folks will have the same results.

vanniktech · 2019-01-27T00:19:00Z

I'll test it out on Monday. Are there any further changes you want to see for 0.6.0?

mario · 2019-01-27T00:26:23Z

Ideally we'd at least have the ignored emojis fixed for 0.6, but yea...not something I can do much about :)

Do you folks have a slack channel/telegram group/signal group/whatever where we can talk more real-time btw?

vanniktech · 2019-01-27T09:46:25Z

Nah. Just GitHub. Maybe Ruben can help with the Emojis. Also what should we do about EmojiOne?

mario · 2019-01-27T10:04:53Z

I'd drop it if that's what the folks prefer.

vanniktech · 2019-01-27T10:15:31Z

Can we then also update the Emoji ordering @rubengees? I feel like when cutting a new release we should have all of the emojis in place.

rubengees · 2019-01-27T11:47:50Z

Regarding this PR - still does not work in landscape for me. The popup is not shown at all...

Maybe Ruben can help with the [ignored] Emojis.

I'll try to look into that 🙂

Can we then also update the Emoji ordering?

You mean the order in which the emojis are shown in the picker? If yes, I can't think of a way. The generator simply uses the ordering of our datasource.

mario · 2019-01-27T12:06:47Z

@rubengees can you talk the Sample app shipped in the repo (and this PR, in particular) to exhibit the behaviour you mentioned so I can properly debug? I've tried a few combinations and they all work as expected for me :(

rubengees · 2019-01-27T14:35:49Z

@mario Hmm it seems to have been an issue with my app, at least partially. I hide and show the input based on data that is loaded and also instantiated the popup only when it was first used (using Kotlin's lazy) and that seemed to have put the logic into an awkward state.

It works mostly now. But I have noticed this:

The first screenshot shows the landscape view without the picker opened. The second with it opened. As you can see, the "done" button is not accessible for the user anymore. I think the popup should only have the height of the keyboard in landscape also.

mario · 2019-01-27T15:08:03Z

So the suggestion is to instantiate the popup as soon as possible due to global layout listener in general. Let me investigate your issue now :)

mario · 2019-01-27T15:14:17Z

Nop, can't reproduce - can you send me a sample layout file? I just remove all the adjustResize and set the flagNoExtractUI and it works as expected :(

rubengees · 2019-01-27T15:19:07Z

I am currently on mobile only, but will try to put something together and investigate more thoroughly asap. My observations above were only quick findings.

mario · 2019-01-27T16:01:33Z

Thanks!

…

On Sun, 27 Jan 2019, 16:19 Ruben Gees ***@***.*** wrote: I am currently on mobile only, but will try to put something together and investigate more thoroughly asap. My observations above were only quick findings. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#329 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAWsq5l8o6EujVrzoL6pdbgsci7K5mKks5vHcNrgaJpZM4aP-x1> .

mario · 2019-01-31T00:58:25Z

Sorry to bother, but any progress on getting me a way to reproduce or a fix @rubengees?

rubengees · 2019-01-31T07:16:48Z

Sorry, not yet. I'm currently busy with work, but have not forgotten about it!

rubengees · 2019-02-03T13:19:28Z

I have investigated further and the problems with the popup not showing were all linked to problems with my app, sorry for the confusion... Maybe we should add a disclaimer to the README, that the popup needs to be instantiated as early as possible, to avoid others to make the same mistake?

Apart from that, only the problem with the popup being above the "done" button is left for me. I can reproduce that without any modifications to the sample or the library. It happens on my real phone (OnePlus 5T, Android 9.0 stock) and also on an default Android Studio emulator (Pixel 2, Android 9.0).

vanniktech · 2019-02-03T13:27:08Z

Maybe we should add a disclaimer to the README, that the popup needs to be instantiated as early as possible, to avoid others to make the same mistake?

Feel free to create a PR with that.

mario · 2019-02-12T22:50:12Z

There were two separate leaks that I've hopefully fixed correctly - and hopefully the build turns green this time.

Signed-off-by: Mario Danic <[email protected]>

mario · 2019-02-13T15:33:18Z

Tadadadadada .... it's green! :D

Signed-off-by: Mario Danic <[email protected]>

mario · 2019-02-13T16:05:07Z

Flow (Assuming we just started)

Click the emoji toggle button
Check if the EditText we're using has a noExtractUI flag and we're in a landscape orientation
If we are, store the original IME options, set the flag on the editText and restart input so the changes would get picked up by the keyboard right away

We do this because the global layout doesn't get into effect with fullscreen EditText since the keyboard is in another process and in a different layout.

We set the receiver to know what happens when we try to show the keyboard, and toggle the keyboard. If a keyboard is already shown, it'll show the Emoji popup.

Now that we basically have a regular keyboard and views, we can measure keyboard height using two alternatives:
hidden method to get keyboard height
magic calculations as a fallback in case reflection fails (even though the fallback also uses reflection to a degree)
on dismiss, we basically have to do three things:
- clear the emoji receiver so we don't leak
- check if we changed the original imeOptions of the editText and if so, restore them
- cancel autofillManager so we don't leak EmojiSpans

Let me know if I failed to explain something properly and I'll happily do it.

rubengees · 2019-02-14T18:53:39Z

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

+      if (SDK_INT >= O) {
+        AutofillManager autofillManager = context.getSystemService(AutofillManager.class);
+        if (autofillManager != null) {
+          autofillManager.cancel();


Great! I sometimes saw this leak in my app, but always thought that was an Android SDK one.

:) It's almost like magic! :)

I'm pretty sure this is the only available open source lib these days that handles keyboard absolutely correctly btw, which is a great accomplishment - thanks to both @rubengees and @vanniktech for pushing when things didn't work as they were supposed to!

vanniktech

Feel free to merge once @rubengees approves. The changes can also be done in a follow up.

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

vanniktech · 2019-02-14T19:36:00Z

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

+      final Method visibleHeightMethod = inputMethodManagerClass.getDeclaredMethod("getInputMethodWindowVisibleHeight");
+      visibleHeightMethod.setAccessible(true);
+      return (int) visibleHeightMethod.invoke(imm);
+    } catch (NoSuchMethodException exception) {


nit: can be grouped together

No they can't, tried that and the IDE complained, because this gets compiled to catch an exception only available on Java 8 if I recall correctly. Catching generic Exception should also not be done since it hides RuntimeExceptions on which we want to crash.

vanniktech · 2019-02-14T19:36:07Z

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

+      }
+    } catch (NoSuchFieldException noSuchFieldException) {
+      Log.w(TAG, noSuchFieldException.getLocalizedMessage());
+    } catch (IllegalAccessException illegalAccessException) {


Co-Authored-By: mario <[email protected]>

mario · 2019-02-14T20:00:21Z

@rubengees can we merge? After that, @vanniktech can make a release.

Signed-off-by: Mario Danic <[email protected]>

rubengees · 2019-02-14T20:38:52Z

I'll make a last test soon, then I'll approve, thanks for the explanation!

rubengees

Works great! No leaks and can't find any issues anymore, great job @mario!

vanniktech · 2019-02-14T21:42:11Z

Thanks @mario really appreciate the effort you put into this! Feel free to merge!

mario · 2019-02-14T21:53:42Z

Please make a release now, thanks! :)

vanniktech · 2019-02-14T21:55:11Z

Will do first thing in the morning tomorrow

mario requested review from vanniktech and rubengees January 24, 2019 01:55

mario force-pushed the fix-328 branch 2 times, most recently from 54eb9e0 to fad8606 Compare January 24, 2019 01:58

rubengees requested changes Jan 25, 2019

View reviewed changes

emoji/src/main/java/com/vanniktech/emoji/EmojiPopup.java Outdated Show resolved Hide resolved

emoji/src/main/java/com/vanniktech/emoji/EmojiPopup.java Outdated Show resolved Hide resolved

mario force-pushed the fix-328 branch from fad8606 to 6ad9ce9 Compare January 26, 2019 21:36

mario mentioned this pull request Jan 28, 2019

Show duplicate emojis in the input but not in the picker #336

Merged

mario force-pushed the fix-328 branch 2 times, most recently from fb61127 to ac1d9ed Compare February 12, 2019 22:49

Fix memory leaks

0afa88a

Signed-off-by: Mario Danic <[email protected]>

mario force-pushed the fix-328 branch from ac1d9ed to 0afa88a Compare February 12, 2019 22:51

Update for codestyle issues

1db9ac8

Signed-off-by: Mario Danic <[email protected]>

mario added 2 commits February 13, 2019 16:41

Change ERR from public to package private

dc7c46f

Signed-off-by: Mario Danic <[email protected]>

Change min height to 50dp

746caa4

Signed-off-by: Mario Danic <[email protected]>

mario modified the milestones: 0.7.0, 0.6.0 Feb 13, 2019

rubengees reviewed Feb 14, 2019

View reviewed changes

vanniktech previously approved these changes Feb 14, 2019

View reviewed changes

vanniktech changed the title ~~Fix #328~~ Fix Emoji keyboard in certain cases Feb 14, 2019

Update emoji/src/main/java/com/vanniktech/emoji/EmojiResultReceiver.java

d7bd99a

Co-Authored-By: mario <[email protected]>

mario dismissed vanniktech’s stale review via d7bd99a February 14, 2019 19:59

Update emoji/src/main/java/com/vanniktech/emoji/EmojiResultReceiver.java

28aca9a

Co-Authored-By: mario <[email protected]>

Import nullable

68846ef

Signed-off-by: Mario Danic <[email protected]>

vanniktech approved these changes Feb 14, 2019

View reviewed changes

rubengees approved these changes Feb 14, 2019

View reviewed changes

mario merged commit 70c58cf into master Feb 14, 2019

mario deleted the fix-328 branch February 14, 2019 21:53

mario mentioned this pull request Feb 14, 2019

Other UI elements can't be selected when emoji popup is visible #340

Closed

Fix Emoji keyboard in certain cases #329

Fix Emoji keyboard in certain cases #329

Conversation

mario commented Jan 24, 2019

rubengees left a comment

Choose a reason for hiding this comment

mario commented Jan 25, 2019

mario commented Jan 25, 2019 • edited Loading

mario commented Jan 25, 2019

rubengees commented Jan 25, 2019

mario commented Jan 26, 2019

mario commented Jan 26, 2019

mario commented Jan 27, 2019

vanniktech commented Jan 27, 2019

mario commented Jan 27, 2019

vanniktech commented Jan 27, 2019

mario commented Jan 27, 2019

vanniktech commented Jan 27, 2019

rubengees commented Jan 27, 2019

mario commented Jan 27, 2019

rubengees commented Jan 27, 2019

mario commented Jan 27, 2019

mario commented Jan 27, 2019

rubengees commented Jan 27, 2019

mario commented Jan 27, 2019 via email

mario commented Jan 31, 2019

rubengees commented Jan 31, 2019

rubengees commented Feb 3, 2019 • edited Loading

vanniktech commented Feb 3, 2019

mario commented Feb 12, 2019

mario commented Feb 13, 2019

mario commented Feb 13, 2019

rubengees Feb 14, 2019

Choose a reason for hiding this comment

mario Feb 14, 2019

Choose a reason for hiding this comment

mario Feb 14, 2019

Choose a reason for hiding this comment

vanniktech left a comment

Choose a reason for hiding this comment

vanniktech Feb 14, 2019

Choose a reason for hiding this comment

rubengees Feb 14, 2019

Choose a reason for hiding this comment

vanniktech Feb 14, 2019

Choose a reason for hiding this comment

mario commented Feb 14, 2019

rubengees commented Feb 14, 2019

rubengees left a comment

Choose a reason for hiding this comment

vanniktech commented Feb 14, 2019

mario commented Feb 14, 2019

vanniktech commented Feb 14, 2019

mario commented Jan 25, 2019 •

edited

Loading

rubengees commented Feb 3, 2019 •

edited

Loading