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: add focused/closed properties to UI widgets #11433

Merged
merged 25 commits into from Jun 22, 2020

Conversation

sgtcoolguy
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-27711

Description:
This adds:

  • Ti.UI.Window.closed and Ti.UI.Window.focused
  • Ti.UI.TextField.focused
  • Ti.UI.TextArea.focused
  • Ti.UI.SearchBar.focused

Additionally, this tweaks the behavior on iOS to mimic Android. Specifically Ti.UI.Window's open and close methods used to block until the underlying open/close operation finished. Now they basically schedule that to happen async. The end result is that on both platforms we now get this ordering:

  • Ti.UI.Window#open() returns
  • Ti.UI.Window open event fires
  • Ti.UI.Window#close() returns
  • Ti.UI.Window close event fires

I also made an underlying fix to our legacy iOS proxies (via the JavaScriptCore C API, not the "new" bindings via Obj-C API). When looking up properties to be exposed to JS, it handles properties with modified getter method names, i.e.:

@property (readonly, getter=isFocused) BOOL focused;

Before it would ignore that the getter is actually named isFocused and would always assume it matches the property name (focused). I had to do this specifically because there was already an internal API focused: method on the base class for Ti.UI.TextField and TextArea.

@build
Copy link
Contributor

build commented Jan 14, 2020

Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 6632 tests are passing.
(There are 700 skipped tests not included in that total)

Generated by 🚫 dangerJS against 2abe173

@sgtcoolguy sgtcoolguy marked this pull request as ready for review January 14, 2020 19:48
@@ -518,6 +518,8 @@ protected void handleClose(KrollDict options)
if (activity != null && !activity.isFinishing() && !activity.isDestroyed()) {
activity.finish();
}

// TODO: Does this ever fire the close event?!
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the close event is only fired from closeFromActivity()

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the current behavior. The "close" event should only be fired once the activity window has been closed.

On Android, we cannot create/destroy activity windows directly. We can only request the OS to do it. The Android OS will not let us create/destroy (ie: open/close a window) while the app is in the background. And the Android OS can destroy an activity window (hence close it) for various reasons such as via back button, intents have particular flags, when Android's "Don't keep activities" is enabled, etc.

The "Don't keep activities" flag is the most interesting one to watch out for. When set, the parent activity window will be temporarily destroyed when displaying a child activity or homing-out, but will be re-created/opened when navigating back.

Honestly, I don't think we should muck with the open/close behavior too much. Perhaps what we really want is a "closing" event (because that's what is being requested) and keep the current "close" event (which I wish was called "closed" past-tense instead).

@jquick-axway
Copy link
Contributor

jquick-axway commented Jan 14, 2020

We need to test the following scenario...

var window = Ti.UI.createWindow();
window.open();
window.close();

On Android, since open/closing of an activity window is async, we have to store the state so that we know to close the window once the activity window has been created by the Android OS. We do this within the Activity.onCreate() here...
https://github.com/appcelerator/titanium_mobile/blob/821ca0645406893c863ecbcfc8f5d3b22333d19e/android/titanium/src/java/org/appcelerator/titanium/TiBaseActivity.java#L573-L584

We'll need to do the same test for iOS. (Or perhaps you've done it already?)

@jquick-axway
Copy link
Contributor

jquick-axway commented Jan 14, 2020

Since a window is opened/closed async, it really has 4 states.

  • opening
  • opened
  • closing
  • closed

Although I don't know if we want to introduce that complexity. Having a single boolean property is convenient, but you end up making an assumption about the other 3 states.

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

iOS changes looks good.

@sgtcoolguy
Copy link
Contributor Author

@jquick-axway It's true that because open and close are async, we do technically have the states of opening and closing. I thought that may be perhaps getting too far in the weeds to consider as properties to expose.

My goal is really to help developers avoid making duplicate close calls (or as in our tests, to know the window is already closed so as to not have to call close and then assume if nothing happens after a while that it was already closed before our call!)

As to your comment around testing:

var window = Ti.UI.createWindow();
window.open();
window.close();

What specifically do you mean we need to test here? That the properties update in between? That it just works as expected? (which I assume is that it opens and then closes right away?)

@jquick-axway
Copy link
Contributor

var window = Ti.UI.createWindow();
window.open();
window.close();

@sgtcoolguy , the above is supposed to close the window before it opens. We used to have an Android bug where the close() method was ignored since the window wasn't open yet (since it's async) and it would continue to open anyways. We should make sure iOS won't have the same issue as well.

@sgtcoolguy
Copy link
Contributor Author

@jquick-axway So I finally got around to testing this exact scenario and even made a dumb unit test to do it - and it passes on Android and fails on iOS. In fact, it even logs this:

[INFO]  Window is opening. Ignoring this close call

So I suspect that this is long-standing behavior on iOS. But if we want to match Android, it probably should be fixed. I'll take a look.

@sgtcoolguy sgtcoolguy merged commit a697b77 into tidev:master Jun 22, 2020
@sgtcoolguy sgtcoolguy deleted the TIMOB-27711 branch June 22, 2020 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants