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 a regressions of changing TabGroup's title after it was created. #10778

Merged
merged 6 commits into from Mar 25, 2019

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Mar 14, 2019

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

Description:
Fixes a regression introduced in 8.0.0 for TabGroup where if you change a TabGroup's title property after it was drawn does not work anymore.

Test case:
app.js

var window1 = Titanium.UI.createWindow({
    title:'Tab 1'
});
 
var window2 = Titanium.UI.createWindow({
    title:'Tab 2'
});
 
var window3 = Titanium.UI.createWindow({
    title:'Tab 3'
});
 
var tab1 = Ti.UI.createTab({
    title:'Tab 1',
window:window1
});
 
var tab2 = Ti.UI.createTab({
    title:'Tab 2',
    window:window2
});
 
var tab3 = Ti.UI.createTab({
    title:'Tab 3',
    window:window3
});
 
tab1.addEventListener('selected', function() {
    tabGroup.title = 'Tab 1';
});
 
tab2.addEventListener('selected', function() {
    tabGroup.title = 'Tab 2';
});
 
tab3.addEventListener('selected', function() {
    tabGroup.title = 'Tab 3';
});
 
var tabGroup = Titanium.UI.createTabGroup({
    tabs:[tab1, tab2, tab3],
    swipeable:true,
    exitOnClose:false,
    style:Titanium.UI.Android.TABS_STYLE_BOTTOM_NAVIGATION,
    shiftMode:false
});
 
tabGroup.open();

@build
Copy link
Contributor

build commented Mar 14, 2019

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

💾 Here's the generated SDK zipfile.

📖 ❌ 2 tests have failed There are 2 tests failing and 462 skipped out of 3633 total tests.

Tests:

Classname Name Time Error
android.emulator.Titanium.UI.NavigationWindow basic open/close navigation 47.279 timeout of 10000ms exceeded
android.emulator.Titanium.UI.WebView .zoomLevel 1.728 expected 4 to equal 1

Generated by 🚫 dangerJS against 3c95f3b

{
// If the native view is drawn get the title value from the SupportActionBar
if (view != null) {
return ((AppCompatActivity) getActivity()).getSupportActionBar().getTitle().toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

We need a null check on:

  • getActivity() when fetching "title" after a TabGroup has been closed.
  • getSupportActionBar() when using the "Theme.AppCompat.NoTitleBar" theme.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can getTitle() return null if you use a Toolbar as an action bar without the title set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This turned out to be something bigger I have missed. Using a Toolbar as an ActionBar relies on adding it the usual way to a Window (through the window.add()), but this method is not available for the TabGroup. I am going to make setting the supportToolbar the only requirement for it to work for TabGroup. So far that raised a few interesting questions, but I will try to answer them all with the next update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good point. Yeah, there is no good way to add a Toolbar to a TabGroup since we don't allow TabGroup.add() to add any views.

I suppose we don't "need" to add toolbar support to tab groups. Sticking to the action bar might be fine. And we have TabbedBar support now which (in combination with ScrollableView) allows app devs to construct whatever elaborate tabbing interface they want now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will leave using a Toolbar as an ActionBar with TabGroup as a separate task. It turned out to be a bit more complicated than I expected. With the current way we support this for Window through Alloy and the TabGroup's implementation itself I prefer for it to be addressed together.

@@ -386,6 +391,11 @@ public TabProxy getSelectedTab()
return ((TabGroupProxy) getProxy()).getTabList().get(this.tabGroupViewPager.getCurrentItem());
}

public void updateTitle(String title)
{
this.actionBar.setTitle(title);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need a null check in case we're using the "Theme.AppCompat.NoTitleBar" theme.

# Conflicts:
#	tests/Resources/ti.ui.tabgroup.addontest.js
@ypbnv
Copy link
Contributor Author

ypbnv commented Mar 21, 2019

@jquick-axway I have updated the PR (and the backport as well) with the null guards.

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 March 22, 2019 15:37
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: When a Window has its own title in a tabGroup the title now appears. Tested with the test case above.

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 242664d into tidev:master Mar 25, 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