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

[TIMOB-25656] Android: getOrCreateView() should always return a View #9721

Merged
merged 4 commits into from Jan 25, 2018

Conversation

garymathews
Copy link
Contributor

  • Allow getOrCreateView() to always return a View
  • If its activity has been destroyed before being created, it will be created with the parent of that activity (falling back to the current activity or root)

TEST CASE

var rootWindow = Ti.UI.createWindow(),
    button = Ti.UI.createButton({ title: 'TEST' });

button.addEventListener('click', function(e) {
	var childWindow = Ti.UI.createWindow();

	childWindow.addEventListener('open', function(e) {
		var imageView = Ti.UI.createImageView();

		childWindow.addEventListener('close', function(e) {
			console.log('tintColor: ' + imageView.tintColor);
		});

		childWindow.close();
	});

	childWindow.open();
});

rootWindow.add(button);
rootWindow.open();

JIRA Ticket

Copy link
Contributor

@ypbnv ypbnv 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

@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Jan 12, 2018

FR Passed. Will do another FR pass with suggestions below.

TiViewProxy.getOrCreateView does not return null.

Studio Ver: 5.0.0.201712081732
SDK Ver: 7.1.0 local build
OS Ver: 10.13.2
Xcode Ver: Xcode 9.2
Appc NPM: 4.2.11
Appc CLI: 7.0.1
Daemon Ver: 1.0.1
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.10
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 5 --- Android 6.0.1
⇨ google Pixel --- Android 7.1.1

@@ -557,6 +556,12 @@ protected TiUIView handleGetView()
}

Activity activity = getActivity();
if (activity.isDestroyed()) {
activity = activity.getParent();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hold on. I see a problem here. The parent activity can be in a destroyed state too.

Instead of falling-back to the parent activity, I think you should set the activity to null here and let the below code grab the top-most activity. Odds are the top-most activity is the one the proxy's view is going to be used on. Or alternatively, walk up the parent activity tree until you find a non-destroyed activity, such as the LauncherActivity.

The below code should exercise this issue...

var rootWindow = Ti.UI.createWindow();
var button = Ti.UI.createButton({ title: "Start Test" });
button.addEventListener("click", function(e) {
	var imageView;
	var childWindow1 = Ti.UI.createWindow();
	childWindow1.addEventListener("open", function(e) {
		var childWindow2 = Ti.UI.createWindow();
		childWindow2.addEventListener("open", function(e) {
			// Create the image view under the 2nd child window.
			imageView = Ti.UI.createImageView();
			childWindow2.close();
		});
		childWindow2.addEventListener("close", function(e) {
			childWindow1.close();
		});
		childWindow2.open();
	});
	childWindow1.addEventListener("close", function(e) {
		// Attempt to access after both child windows have been destroyed.
		imageView.tintColor;

		// With this PR's code, the proxy would theoretically create a view a 2nd
		// time here because above line switched to using destroyed parent activity.
		imageView.tintColor;
	});
	childWindow1.open();
});
rootWindow.add(button);
rootWindow.open();

@jquick-axway
Copy link
Contributor

@lokeshchdhry, we should also test using the customer's app.js code attached to JIRA ticket TIMOB-25656. That apps spawns multiple child windows before attempting to create the proxy's view.

@lokeshchdhry
Copy link
Contributor

Thanks @jquick-axway for the suggestion.

I am seeing some issues with the user code.

  1. On android 7.1.1 : No issues.

  2. On android 6.0.1 : Runtime error:

[ERROR] :  [Nexus 5] TiExceptionHandler: (main) [13606,13606] ----- Titanium Javascript Runtime Error -----
[ERROR] :  [Nexus 5] TiExceptionHandler: (main) [0,13606] - In /app.js:49,14
[ERROR] :  [Nexus 5] TiExceptionHandler: (main) [0,13606] - Message: Uncaught Attempt to invoke virtual method 'boolean android.app.Activity.isDestroyed()' on a null object reference
[ERROR] :  [Nexus 5] TiExceptionHandler: (main) [0,13606] - Source:   drawerView.setRightView(realRight);
[ERROR] :  [Nexus 5] V8Exception: Exception occurred at /app.js:49: Uncaught Attempt to invoke virtual method 'boolean android.app.Activity.isDestroyed()' on a null object reference
[ERROR] :  [Nexus 5] V8Exception: Attempt to invoke virtual method 'boolean android.app.Activity.isDestroyed()' on a null object reference
  1. On android 4.4.2 emulator : No issues.

The latest code above by @jquick-axway , does not throw any error on android 7.1.1, 6.0.1 & 4.4.2.

@garymathews garymathews force-pushed the TIMOB-25656 branch 2 times, most recently from 9267551 to 17d9b4a Compare January 17, 2018 18:34
@garymathews
Copy link
Contributor Author

@jquick-axway @lokeshchdhry Updated PR

@build
Copy link
Contributor

build commented Jan 17, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

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

@lokeshchdhry
Copy link
Contributor

FR Passed.

TiViewProxy.getOrCreateView does not return null & no runtime error is thrown on android 6.0.1.

Studio Ver: 5.0.0.201712081732
SDK Ver: 7.1.0 local build
OS Ver: 10.13.2
Xcode Ver: Xcode 9.2
Appc NPM: 4.2.11
Appc CLI: 7.0.1
Daemon Ver: 1.0.1
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.10
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 5 --- Android 6.0.1
⇨ google Pixel --- Android 7.1.1
Emulator: Android 8.0

@lokeshchdhry
Copy link
Contributor

@jquick-axway , Danger is not passing because its needs tests. Can you please provide some or if not possible to provide a test then remove needs tests & add no tests label.

@build build added the android label Jan 24, 2018
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