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): Fix setting an active tab for TabGroup before opening. #10740

Merged
merged 9 commits into from Mar 8, 2019

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Mar 1, 2019

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

Description:
Fixes setting an active tab for TabGroup before it is opened. Fixes backwards compatibility for
events - one unnecessary "focus" is currently triggered on TabGroup opening. Removes unused code. Clean up imports. Adds new unit tests. Updates docs.

Note: I have tested with both supported styles - default and bottom navigation. What I wonder is what is the best way to include the bottom navigation style in unit tests - add all the tests with it ( that would work, but for any new style possibly added later it may become too much work) or we use the style property randomly chosen for each test. With the latter we still may miss some edge cases.

I will try to figure out the failing of the "blur" unit tests since this PR changes event firing a little bit.

Test case:
app.js

var tabGroup = Ti.UI.createTabGroup();
 
var win1 = Titanium.UI.createWindow({
    title: 'Tab 1',
    backgroundColor: '#fff'
});
var tab1 = Titanium.UI.createTab({
title: 'Tab 1',
    window: win1
});
 
var label1 = Titanium.UI.createLabel({
    color: '#999',
    text: 'I am Window 1',
    textAlign: 'center',
    width: 'auto'
});
 
win1.add(label1);
var win2 = Titanium.UI.createWindow({
    title: 'Tab 2',
    backgroundColor: '#fff'
});
var tab2 = Titanium.UI.createTab({
    title: 'Tab 2',
    window: win2
});
 
var label2 = Titanium.UI.createLabel({
    color: '#999',
    text: 'I am Window 2',
    textAlign: 'center',
    width: 'auto'
});
 
win2.add(label2);
tabGroup.addTab(tab1);
tabGroup.addTab(tab2);
tabGroup.activeTab = 1;
Ti.API.info("### activeTab: " + tabGroup.activeTab);
tabGroup.open();

Fixes setting an active tab for TabGroup before it is opened. Fixes backwards compatiblity for
events. Removes unused code. Clean up imports. Adds new unit tests. Updates docs. Fixes TIMOB-26843
@ypbnv ypbnv added this to the 8.1.0 milestone Mar 1, 2019
@ypbnv ypbnv requested a review from jquick-axway March 1, 2019 10:30
@build build requested review from a team March 1, 2019 11:03
@build
Copy link
Contributor

build commented Mar 1, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 344 tests are passing.

Generated by 🚫 dangerJS against e31cfa3

@@ -445,14 +445,10 @@ protected void handlePostOpen()

TabProxy activeTab = handleGetActiveTab();
if (activeTab != null) {
selectedTab = activeTab;
selectedTab = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This class' onTabSelected() method is missing a null check on the selectedTab member variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to this onTabSelected() method?
https://github.com/appcelerator/titanium_mobile/blob/master/android/modules/ui/src/java/ti/modules/titanium/ui/TabGroupProxy.java#L529
If that is the case, it is intended for the selectedTab to be null in order to skip the sending of the focus/blur events when a tab is set as an active one before the opening of the TabGroup. But I may have not got what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh you're right. Never mind.

Copy link
Contributor

@sgtcoolguy sgtcoolguy Mar 6, 2019

Choose a reason for hiding this comment

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

Note that the unit test around blur events is now failing, and is likely related to the changes in this area...

https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/master/Resources/ti.ui.tabgroup.test.js#L232-L247

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some digging I found that the "blur" unit test started failing because this PR fixed firing a pair of focus/blur events that are not supposed to be fired. That problem was introduced at the same time when the unit test was added (Both by me) and this is the reason it didn't fail every single time in other PRs. With my latest update this should be cleared up.

Copy link
Contributor

Choose a reason for hiding this comment

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

The unit test is passing now.

.getTabList()
.get(currentlySelectedIndex)
.fireEvent(TiC.EVENT_UNSELECTED, null, false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check if getProxy() returns null in the selectTabItemInController() method and the onMenuItemClick() method, which can happen when the proxy has been released (ie: it's JavaScript object was garbage collected).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@jquick-axway
Copy link
Contributor

Looks good @ypbnv. I'm just asking for a few more null checks just in case. Thanks.

@keerthi1032
Copy link
Contributor

FR Passed. Active tab works fine .some tests are failed in Jenkins.waiting for that to get resolved. also we need Backport PR FOR 8.0.
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
Titanium SDK
SDK Version = 8.1.0.v20190301023310
Device =samsung s5 android 6
Emulator =google pixel 3 xL android 8

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

@ypbnv
Copy link
Contributor Author

ypbnv commented Mar 7, 2019

@jquick-axway Would you be able to take a look at my latest commit - I moved the nullifying of a Tab's parent - from when the views are released to when the Tab proxy itself is released. That is the reason the blur event was failing. Its source is the Tab and bubbles up to the TabGroup, but the focus change happens after the views of the TabGroup are released. And at the views release the parent was set to null, so in the end the event had nowhere to propagate to.

@sgtcoolguy sgtcoolguy merged commit af9f728 into tidev:master Mar 8, 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

7 participants