Skip to content
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

feat(android): parity for keyboardframechanged and keyboardVisible #13726

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented Jan 21, 2023

Add Android parity for keyboardframechanged event and Ti.App.keyboardVisible to Ti.App:

var win = Ti.UI.createWindow();
var tf = Ti.UI.createTextField({width: 100});
var tf2 = Ti.UI.createTextField({width: 100, top: 0});
win.add([tf,tf2]);

Ti.App.addEventListener("keyboardframechanged", function(e){
	alert("keyboard visible: " + Ti.App.keyboardVisible);
	if (Ti.App.keyboardVisible) {
		alert("Size: " + e.keyboardFrame.width + " x " + e.keyboardFrame.height);
	}
});
win.addEventListener("open", function(e){
	alert("keyboard visible: " + Ti.App.keyboardVisible);
})
win.open();

@m1ga
Copy link
Contributor Author

m1ga commented Jan 21, 2023

simplescreenrecorder-2023-01-21_13.43.59.mp4

@hansemannn
Copy link
Collaborator

@m1ga
Copy link
Contributor Author

m1ga commented Jan 23, 2023

I can trigger that event and use the keyboardVisible property 👍
Not all fields will be filled in the event but at least it will be there for Android and you can check the keyboard status!

@hansemannn
Copy link
Collaborator

Yep, great parity already!

@m1ga
Copy link
Contributor Author

m1ga commented Jan 23, 2023

It's not in the Ti.App. namespace

  • Ti.App.keyboardVisible
  • and Ti.App.addEventListener("keyboardframechanged")

@m1ga m1ga changed the title feat(android): add keyboard show/hide event feat(android): parity for keyboardframechanged and keyboardVisible Jan 23, 2023
@m1ga
Copy link
Contributor Author

m1ga commented Jan 24, 2023

Not sure about some of the errors in the test suite. Don't think they are related to this PR.
e.g. I see

Expected to throw exception\\' undefined undefined' to be 'Unable to convert null'"

in error.test.js Native exception surfaced. It's using Ti.Geolocation.accuracy = null; to check for 'Unable to convert null'.
It says its returning undefined but when I just run that line in a test app I see
Screenshot_20230124-193207

so the error is Unable to convert null 🤷

I've tested my app, your app, kitchensink and hyperloop examples and they look fine 👍

@hansemannn
Copy link
Collaborator

Still, the onKeyboardChanged callback seems to be always called now, causing more work on the main thread, even when not using the feature. This should be prevented prior merging this PR

@m1ga
Copy link
Contributor Author

m1ga commented Jan 24, 2023

It shouldn't fire the event because it will go this route:
Screenshot_20230124_201152
it's not calling fireEvent in onKeyboardChanged, it's calling a method in TiApplication, from there it will call fireEvent and check hasListener

Where do you see the main thread activity? I could add the event back to the window so the check is in the same file but then it wouldn't be parity

@hansemannn
Copy link
Collaborator

I meant the Java callback invocation and construct of the kroll dict, sorry for the confusion

@m1ga
Copy link
Contributor Author

m1ga commented Jan 24, 2023

exposed the hasListener and added it to the if checks:

fec7cc7#diff-6f50b13a3f7b68de5485b38155383cdbd1ec15887ad7fd84573b826d79154790R772

@hansemannn
Copy link
Collaborator

Thanks for all the adjustments! Sorry for being so picky..but I think it's worth to double-check the calls before it's getting an issue in production. We should be ready to go here..if someone else can verify the fix, we can merge.

Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-review approved, waiting for functional testing.

@hansemannn
Copy link
Collaborator

@m1ga Any external testing done? Maybe the users that requested this can help. This can always be the way to fast-lane changes.

@m1ga
Copy link
Contributor Author

m1ga commented Feb 24, 2023

Since this is a Android 11+ fix it wasn't useful for the user (Android 8.1 device) so I don't know the current status. I don't have any use for it so I didn't implement it in a client app at the moment

apidoc/Titanium/App/App.yml Outdated Show resolved Hide resolved
apidoc/Titanium/App/App.yml Outdated Show resolved Hide resolved
@m1ga
Copy link
Contributor Author

m1ga commented Sep 14, 2024

Tested on Android 14 (Pixel 9) and Android 15 (Emulator) again, working fine. Event is fired and keyboardVisible value is correct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants