-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
@garymathews Can you update the docs? They currently indicate to be iOS only. Also it seems like the test fails on iOS. |
2ecffe7
to
fe53a62
Compare
@hansemannn Looks like it wasn't implemented for iOS either |
iphone/Classes/TiUITabGroup.m
Outdated
@@ -636,6 +636,10 @@ - (void)open:(id)args | |||
|
|||
- (void)close:(id)args | |||
{ | |||
if ([self.proxy _hasListeners:@"close"]) { | |||
[self.proxy fireEvent:@"close" withObject:event]; | |||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CR: Pass
There was a problem hiding this 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.
FR Passed. For android, the tabgroup Studio Ver: 5.2.0.201804230823 |
@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. |
@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? |
@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(); |
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. |
Generated by 🚫 dangerJS |
Seems to pass now 👏 |
close
event forTitanium.UI.TabGroup
TEST CASE
JIRA Ticket