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-25685] Android: TabGroup 'close' event is never fired #9758

Merged
merged 9 commits into from May 23, 2018

Conversation

garymathews
Copy link
Contributor

  • Implement close event for Titanium.UI.TabGroup
TEST CASE
var win = Ti.UI.createWindow(),
	tabGroup = Ti.UI.createTabGroup();
	tab = Ti.UI.createTab({
		title: 'Tab',
		window: win
	});

tabGroup.addEventListener('close', function() {
	console.log('TabGroup.close()');
});

tabGroup.addTab(tab);
tabGroup.open();

JIRA Ticket

@garymathews garymathews added this to the 7.1.0 milestone Jan 19, 2018
@build build added the android label Jan 19, 2018
@hansemannn hansemannn requested a review from ypbnv January 20, 2018 10:05
@hansemannn
Copy link
Collaborator

@garymathews Can you update the docs? They currently indicate to be iOS only. Also it seems like the test fails on iOS.

@garymathews
Copy link
Contributor Author

@hansemannn Looks like it wasn't implemented for iOS either

@@ -636,6 +636,10 @@ - (void)open:(id)args

- (void)close:(id)args
{
if ([self.proxy _hasListeners:@"close"]) {
[self.proxy fireEvent:@"close" withObject:event];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be called event-driven (by delegates that actually execute the closing. Let me find a better place to put it.

EDIT: Either here or by implementing the windowDidClose selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this close method called by windowDidClose anyway?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is (by windowWillClose), but if you call it manually, the close event may fire before the window actually finishing closing. I'll prepare a test-case tomorrow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, so let's do this (as the above test-case does not actually close the tab group):

var win = Ti.UI.createWindow(),
	tabGroup = Ti.UI.createTabGroup();
	tab = Ti.UI.createTab({
		title: 'Tab',
		window: win
	});
  
win.addEventListener('open', function() {
  setTimeout(function() {
    tabGroup.close();
  }, 2000);
})

tabGroup.addEventListener('close', function() {
	console.log('TabGroup.close()');
});

tabGroup.addTab(tab);
tabGroup.open();

It will wait 2 seconds after the application finished launching and close the tab-group. First, windowWillClose is called to cleanup the navigation stack. Once done, it will will invoke it's super-class that will clean up all child views (TiViewProxy instances).

Then, finally, the TiWindowProxy, which the TiUITabGroupProxy inherits from cleans up (dismisses the actual controller, fire the child close events) and sends windowDidClose to TiUITabGroupProxy.

So after all, the close event in its current state would fire before the tabgroup actually finished closing, which is different from the behavior of other root view controllers (like window or split-window on iOS).

And as you stated above, because the class inherits from TiWindowProxy, is should fire the event already (but the link you referenced was linking to the wrong method).

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

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

Remove the iOS change as discussed above.

@lokeshchdhry
Copy link
Contributor

FR Passed.

For android, the tabgroup close event is fired successfully.

Studio Ver: 5.2.0.201804230823
SDK Ver: 7.2.0 local build
OS Ver: 10.13.4
Xcode Ver: Xcode 9.3
Appc NPM: 4.2.13
Appc CLI: 7.0.3
Daemon Ver: 1.1.1
Ti CLI Ver: 5.1.0
Alloy Ver: 1.12.0
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 1.8.0_131
Devices: ⇨ google Nexus 6P --- Android 8.0.0
⇨ google Nexus 5 --- Android 6.0.1
Emulator: ⇨ Android 4.1.2

@lokeshchdhry
Copy link
Contributor

@garymathews , Can you please look at the changes that need to be done on the IOS part. Also it would be great if you can look at the conflicts as well.
Thanks !!

@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
@sgtcoolguy
Copy link
Contributor

@hansemannn We'd like to get this PR in for 7.3.0, but it seems there's some discussion around the iOS changes. Can you please take a look and see if you can provide an alternate fix for iOS here?

@hansemannn
Copy link
Collaborator

@garymathews iOS looks fine (even without this change):

var win = Ti.UI.createWindow(),
	tabGroup = Ti.UI.createTabGroup();
	tab = Ti.UI.createTab({
		title: 'Tab',
		window: win
	});

tabGroup.addEventListener('close', function() {
	console.log('TabGroup.close()');
});

tabGroup.addEventListener('open', function() {
    setTimeout(function () {
        console.log('CLOSING NOW');
        tabGroup.close();
    }, 1000);
})

tabGroup.addTab(tab);
tabGroup.open();

@sgtcoolguy sgtcoolguy added the hold for parity ✋ This PR should not be merged until an equivalent PR is ready for the other platform(s) label May 23, 2018
@sgtcoolguy sgtcoolguy assigned sgtcoolguy and unassigned hansemannn May 23, 2018
@sgtcoolguy
Copy link
Contributor

I'm taking a look at this locally and will try to resolve the (possible) issue. Looks to me like the unit test just was written out-of-order, resulting in the iOS failure.

@build
Copy link
Contributor

build commented May 23, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn
Copy link
Collaborator

Seems to pass now 👏

@sgtcoolguy sgtcoolguy removed the hold for parity ✋ This PR should not be merged until an equivalent PR is ready for the other platform(s) label May 23, 2018
@sgtcoolguy sgtcoolguy merged commit 394ee46 into tidev:master May 23, 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

6 participants