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-20332] Android: Closing a Ti.UI.TabGroup that contains a ti.map view crashes the app #9628

Merged
merged 10 commits into from Dec 18, 2017

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Nov 27, 2017

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

Description:
Getting a reference to the wrong FragmentManager when an instance of TIUIFragment is used in TabGroup.
Removed an unused import.

Test case:
Add the module in tiapp.xml
<module platform="android">ti.map</module>

app.js

// this sets the background color of the master UIView (when there are no windows/tab groups on it)
Titanium.UI.setBackgroundColor('#000');

// create tab group
var tabGroup = Titanium.UI.createTabGroup();

var win1 = Titanium.UI.createWindow({
    title:'Tab 1',
    backgroundColor:'#fff'
});

var tab1 = Titanium.UI.createTab({
    icon:'KS_nav_views.png',
    title:'Tab 1',
    window:win1
});

var Map = require('ti.map');

var mapview = Map.createView({top: 0, height: '80%'});

win1.add(mapview);

tabGroup.addTab(tab1);

tabGroup.open();

var button = Ti.UI.createButton({bottom: 10});

button.addEventListener('click', function() {
    tabGroup.close();
})

win1.add(button);

Expected result - application should close without any crashes.

@build
Copy link
Contributor

build commented Nov 27, 2017

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@sgtcoolguy
Copy link
Contributor

Can we please turn that manual test into an automated mocha test? I assume the only difference would be to instead of using a click listener to close the tab group, use something like setTimeout to close the tab after a short delay. The test would probably best go into tests/Resources/ti.ui.tabgroup.addontest.js

@ypbnv
Copy link
Contributor Author

ypbnv commented Nov 28, 2017

@sgtcoolguy Added the unit test. Two things that I think deserve a notice:

  • I added Google Maps API key in tiapp.xml when running the test locally.
  <android 
    xmlns:android="http://schemas.android.com/apk/res/android">
    <manifest>
        <meta-data android:name="com.google.android.geo.API_KEY" android:value="<API_KEY>"/>
      </application>
    </manifest>
  </android>
  • Even without delay sometimes loading the map took more than 2000 ms (the default allowed timeout for tests), thus I have increased it to 5 seconds and removed any time out from the code. We may want to adjust that for a better performance if that slows the tests too much.

Copy link
Contributor

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

@sgtcoolguy
Copy link
Contributor

@ypbnv @garymathews Do we have an API key we can use for the unit test app? Looks like without it the test crashes on Android and fails on iOS.

@ypbnv
Copy link
Contributor Author

ypbnv commented Nov 29, 2017

@sgtcoolguy If we do not have an API from a dedicated account for testing I could generate one and DM it for you to set it up.

@build build added the android label Dec 1, 2017
@sgtcoolguy sgtcoolguy added this to the 7.1.0 milestone Dec 1, 2017
@sgtcoolguy
Copy link
Contributor

Ok, this is now passing the new unit tests. Ready for QE validation. Once merged it should likely be cherry-picked to 7_0_X branch.

@lokeshchdhry
Copy link
Contributor

FR Passed.

Closing the tabgroup with mapview does not crash the app.

Studio Ver: 5.0.0.201712081732
SDK Ver: 7.1.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.11
Appc CLI: 7.0.0
Daemon Ver: 1.0.0
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.10
Node Ver: 8.9.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 5 --- Android 6.0.1
⇨ google Pixel --- Android 7.1.1

@lokeshchdhry lokeshchdhry merged commit 06bf460 into tidev:master Dec 18, 2017
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