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): Added Ti.App "close" event support #10637

Merged
merged 8 commits into from Sep 10, 2019

Conversation

jquick-axway
Copy link
Contributor

@jquick-axway jquick-axway commented Jan 19, 2019

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

Summary:

  • Fires "close" event just before JS runtime terminates.
    • We document that this must be the last event. So, this is the perfect place to do it.
    • Had to modify V8Object and V8Function to allow V8 calls while in the "released" state.
  • KrollRuntime was refactored a bit:
    • Removed RELAUNCH state. No longer needed since 8.0.0's intent refactoring and now that Services/BroadcastReceivers won't keep JS runtime alive anymore.
    • Removed CountDownLatch. No longer needed since JS runtime can only be ran from UI thread.
    • Modified V8Object and V8Function to allow V8 calls while KrollRuntime is in the RELEASED state, which happens when an OnDisposingListener is being invoked.
  • Moved cancelTimers() call from KrollRuntime to TiApplication.
    • Doing this via listener is a better solution. No longer tightly coupled.

Note:
I've noticed that our TiApplication.softRestart() does not follow the rules and calls the V8Runtime's dispose/init methods directly. This means our OnDisposingListeners won't get invoked. The Ti.App._restart() handling still works with my PR (I've tested it), but we may want to look into this in the future.

Close Event Test:

  1. Build and run the below code on Android.
  2. Press the "Back" button.
  3. Verify the following got logged: ### App 'close' event was fired.
  4. Relaunch the app.
  5. Repeat steps 2 and 3.
Ti.App.addEventListener("close", function() {
	Ti.API.info("### App 'close' event was fired.");
});
Ti.UI.createWindow().open();

LiveView Test:
This test verifies that our runtime restart changes work okay.

  1. Build and run kitchensink-v2 via LiveView on Android.
  2. Verify that the app launches okay.
  3. Edit a JS file and click save.
  4. Verify that the app re-launches okay.

- Removed KrollRuntime state RELAUNCHED. No longer relevant.
- Removed KrollRuntime CountDownLatch. No longer needed since Kroll thread is not supported.
@keerthi1032
Copy link
Contributor

FR passed. Event fired when back button clicked and kitchen sink works fine with live view. build failed in jenkin. Waiting for CR .

Operating System
Name = Mac OS X
Version = 10.13.6
Architecture = 64bit
Node.js
Node.js Version = 8.12.0
npm Version = 6.4.1
Titanium CLI
CLI Version = 5.1.1
Studio Version=
CLI =7.0.10 master 10
Titanium SDK
SDK Version = local 8.1.sdk
Device =Samsung s5 android 6, Oneplus 5t android 9
Emulator =Pixel 3xl android 8

@sgtcoolguy sgtcoolguy modified the milestones: 8.1.0, 8.2.0 Jun 3, 2019
@build
Copy link
Contributor

build commented Jul 8, 2019

Warnings
⚠️

Commit 8fc1dbbee99749dd8903a6902104db32f252d151 has a message "[TIMOB-26542] Android: Added Ti.App "close" event support

  • Removed KrollRuntime state RELAUNCHED. No longer relevant.
  • Removed KrollRuntime CountDownLatch. No longer needed since Kroll thread is not supported." giving 2 errors:
  • subject may not be empty
  • type may not be empty
Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 4336 tests are passing.
(There are 471 tests skipped)

📖

🚨 This PR has one or more commits with warnings/errors for commit messages not matching our configuration. You may want to squash merge this PR and edit the message to match our conventions, or ask the original developer to modify their history.

Generated by 🚫 dangerJS against 5c2f3fe

@jquick-axway
Copy link
Contributor Author

@garymathews, I'm going to push this PR out from 8.1.0 to 8.3.0 since it makes high risk changes to our kroll runtime.

@jquick-axway jquick-axway modified the milestones: 8.2.0, 8.3.0 Jul 8, 2019
@tidev tidev deleted a comment from build Sep 9, 2019
@sgtcoolguy
Copy link
Contributor

FR passed, but still needs a CR cc @garymathews @ypbnv

Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

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

CR: PASS

@garymathews garymathews merged commit 44a5968 into tidev:master Sep 10, 2019
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