-
-
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
Improve Popup logic #97
Conversation
final int fixedOffsetX = actualLocation.x > x ? x - differenceX : x + differenceX; | ||
final int fixedOffsetY = actualLocation.y > y ? y - differenceY : y + differenceY; | ||
|
||
popupWindow.update(fixedOffsetX, fixedOffsetY, DONT_UPDATE_FLAG, DONT_UPDATE_FLAG); |
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 might look weird, but is needed for dialogs. I have not found a way to calculate the offset properly, so we "fix" it here if it differs from the wanted location.
Codecov Report
@@ Coverage Diff @@
## master #97 +/- ##
=======================================
- Coverage 24.96% 23.96% -1%
=======================================
Files 19 19
Lines 629 655 +26
Branches 67 71 +4
=======================================
Hits 157 157
- Misses 459 485 +26
Partials 13 13 Continue to review full report at Codecov.
|
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.
Codewise it does look okay, although this does not work anymore in my simulator (Genymotion).
I assume that happens because of the hardware keyboard, Genymotion uses. |
Yup before it worked perfectly. Also I have the soft keyboard activated and it shows up. |
Okay, I'll investigate. On which API Level does it happen? |
Genymotion Google Nexus 5X 7.0.0 API 24 |
Fixed in bf2fc8b. This seemed to be a bug on APIs greater than 20. We now "fix" the location, exactly like the variant popup. Furthermore I made the |
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.
Sorry I haven't gotten around this any earlier. So far this looks good. I'll test it with a few other devices soon and then merge it if there are no issues.
@@ -62,6 +63,20 @@ static int screenHeight(@NonNull final Activity context) { | |||
return result; | |||
} | |||
|
|||
static Activity contextToActivity(@NonNull final Context context) { |
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.
method name: asActivity
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.
Thanks a lot for this. I tested it on 10+ devices and found no problems.
This would address some of the points in #96 and fix #26.
Contents of this Pull Request
The internal popup logic has been improved in several ways:
Activity
.EmojiPopup
had a wrong state concerningisKeyboardOpen
.onEmojiPopupShownListener
is only shown, after the popup is really shown. Previously it was called, when we started opening the keyboard, which could lead to a wrong state if it was not shown (If a hardware keyboard is present for example).Code changes to achive this
rootView
is calculated from theDecorView
.popupWindow.setFocusable(true)
is now called on the variant popup.popupWindow.setInputMethodMode(PopupWindow.INPUT_METHOD_NOT_NEEDED)
was the key to make that work without weird side effects.I have added a simple Dialog to the sample for showcasing and testing.
Things concerning backwards compability
emojiPopup.dismiss();
inonKeyboardClose
is not necessary anymore.Context
of therootView
to be anActivity
now. We need that as we want to access theWindow
for better height calculation.