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

fix(android): amend hasActivityTransitions() for transitions #10832

Merged
merged 6 commits into from Apr 15, 2019

Conversation

garymathews
Copy link
Contributor

  • Fix hasActivityTransitions() to allow transitions when a sharedElement has not been specified
TEST CASE
var win = Ti.UI.createWindow({
        backgroundColor: 'blue',
        layout: 'vertical'
    }),
    lbl = Ti.UI.createLabel({
        left: 5,
        text: 'TEST LABEL',
        transitionName: 'lbl'
    }),
    btn = Ti.UI.createButton({
        title: 'OPEN'
    });

btn.addEventListener('click', function () {
    var win2 = Ti.UI.createWindow({
            backgroundColor: 'red',
            layout: 'vertical',
            activityEnterTransition: Titanium.UI.Android.TRANSITION_SLIDE_RIGHT
        }),
        lbl2 = Ti.UI.createLabel({
            right: 5,
            text: 'TEST LABEL',
            transitionName: 'lbl'
        }),
        btn2 = Ti.UI.createButton({
            title: 'CLOSE'
        });

    btn2.addEventListener('click', function () {
        win2.close();
    });

    win2.add([lbl2, btn2]);
    // win2.addSharedElement(lbl, 'lbl'); // this should not need uncommenting to see a Window transition

    win2.open();
});

win.add([lbl, btn]);
win.open();

JIRA Ticket

@build
Copy link
Contributor

build commented Apr 8, 2019

Fails
🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 ❌ 1 tests have failed There are 1 tests failing and 462 skipped out of 3647 total tests.

Tests:

Classname Name Time Error
android.emulator.Titanium.Media.AudioPlayer #start, #stop 2.797 timeout of 2000ms exceeded

Generated by 🚫 dangerJS against 4ecedfa

@garymathews garymathews force-pushed the TIMOB-25678 branch 2 times, most recently from 0f48283 to b426ebf Compare April 8, 2019 21:17
{
return false;
super.onResume();
loadScript();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jquick-axway Any thoughts on moving loadScript() over to onResume()?

The reason for this is due to handleOpen(options) occurring before the RootActivity has resumed. Which prevents the transition from happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm open to the idea. It should reduce app startup stutter as well. Let's test it out.

I think the only thing we lose is Activity.onCreate callback support for the root activity, but that's not useful. You're also less likely to receive newintent event from the root activity, but everyone should be reading the launch intent on app startup anyways and not depend on this behavior. We might have to worry about assumptions made after the windowCreated() call in the TiBaseActivity.onCreate(). We might be okay, but that's the part we need to safe-guard.

@jquick-axway
Copy link
Contributor

jquick-axway commented Apr 9, 2019

@garymathews , since you've changed the code to call loadScript() via onResume(), we'll need a flag to indicate that the script is loaded so that we won't load the script a 2nd time after pause/resume.

For example:

  1. Use the below code as your "app.js".
  2. Launch the app.
  3. Tap on the home button.
  4. Resume the app.
  5. Notice that "ti.main.js" gets executed again in the log.
// This will only be logged once since "app.js" was already required-in.
// But the "ti.main.js" script will be re-executed, which is not what we want.
Ti.API.info("@@@ app.js was loaded.");

So, we'll probably want to do something like this...

private boolean wasScriptLoaded;

void protected loadScript()
{
	if (this.wasScriptLoaded) {
		return;
	}

	try {
		String fullUrl = resolveUrl(this.url);
		KrollRuntime.getInstance().runModule(KrollAssetHelper.readAsset(fullUrl), fullUrl, activityProxy);
		this.wasScriptLoaded = true;
	} finally {
		Log.d(TAG, "Signal JS loaded", Log.DEBUG_MODE);
	}
}

Copy link
Contributor

@jquick-axway jquick-axway 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

@ssjsamir ssjsamir self-requested a review April 11, 2019 10:31
Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: Application no longer hangs with the test case above and when using Yeti, transition element seem fine as well when a sharedElement has not been specified.

Test Environment

Google pixel xl 7.1.1 sim
APPC CLI: 7.0.10
Operating System Name: Mac OS Mojave
Operating System Version: 10.14.2
Node.js Version: 8.9.1
Xcode 10.1

@sgtcoolguy sgtcoolguy merged commit 14ad127 into tidev:master Apr 15, 2019
hansemannn added a commit to hansemannn/titanium_mobile that referenced this pull request May 20, 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