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-26314] Android: Add versionCode to tiapp.xml #10264

Merged
merged 1 commit into from Sep 26, 2018

Conversation

m1ga
Copy link
Contributor

@m1ga m1ga commented Aug 16, 2018

@tidev tidev deleted a comment from build Aug 17, 2018
@hansemannn hansemannn added this to the 7.5.0 milestone Aug 17, 2018
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.

LGTM

@build
Copy link
Contributor

build commented Aug 17, 2018

Messages
📖

🎉 Another contribution from our awesome community member, m1ga! Thanks again for helping us make Titanium SDK better. 👍

📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@cb1kenobi
Copy link
Contributor

cb1kenobi commented Aug 17, 2018

Why do we need this? We already compute this when we write the AndroidManifest.xml: https://github.com/appcelerator/titanium_mobile/blob/82748c2cedaac99f9386a551c8b59b09322eb615/android/cli/commands/_build.js#L3721

@m1ga
Copy link
Contributor Author

m1ga commented Aug 17, 2018

@cb1kenobi oh ok. Didn't see that part cause the other "variables" had placeholders. I'll remove that part but the versionCode is still nice to have in the default tiapp.xml

@m1ga m1ga changed the title [AC-5840] Android: Add versionCode to tiapp.xml and use <version> in AndroidManifest [AC-5840] Android: Add versionCode to tiapp.xml Aug 17, 2018
@cb1kenobi
Copy link
Contributor

Sure, but we already have support for that via a custom android manifest section in the tiapp.xml:

<?xml version="1.0" encoding="UTF-8"?>
<ti:app xmlns:ti="http://ti.appcelerator.org">
    <android xmlns:android="http://schemas.android.com/apk/res/android">
        <manifest android:versionCode="2"/>
    </android>
</ti:app>

@m1ga
Copy link
Contributor Author

m1ga commented Aug 17, 2018

yes, but it is not exposed in the default (template) tiapp.xml. You have to look up the parameter name if you don't know it. In this PR I just add it since it is a very default value you always need to change in every project

Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

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

This is a welcome addition! Thanks for the contribution! APPROVED!!!

@hansemannn hansemannn changed the title [AC-5840] Android: Add versionCode to tiapp.xml [TIMOB-26314] Android: Add versionCode to tiapp.xml Aug 18, 2018
@lokeshchdhry
Copy link
Contributor

FR Passed.

tiapp.xml is now by default populated with android:versionCode="1".

Studio Ver: 5.1.1.201809051655
SDK Ver: 7.5.0 local build
OS Ver: 10.13.5
Xcode Ver: Xcode 9.4.1
Appc NPM: 4.2.13
Appc CLI: 7.0.6
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.2
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Nexus 6P (Android 8.1.0)

@garymathews
Copy link
Contributor

Humm... adding versionCode seems to cause the tests to consistently fail?

@cb1kenobi
Copy link
Contributor

I don't get why the tests are failing. This PR is just adding a versionCode to the project templates.

What's more confusing is Jenkins says that this PR's "changes" are in TCPProxy.java, not in the project template. Jenkins has some wires crossed.

@sgtcoolguy
Copy link
Contributor

The last build failed because of the test that opens a Ti.Map in a TabGroup. That also failed on another PR. Seems to be an unrelated issue entirely, but looks to be an Android regression that has started recently. cc @garymathews

@garymathews garymathews merged commit 8e7a610 into tidev:master Sep 26, 2018
@m1ga m1ga deleted the tiappxml branch August 7, 2020 07:57
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

8 participants