CB-14089: (android) Add Kotlin support #441

joshchandler · 2018-05-14T17:29:59Z

Platforms affected

Android

What does this PR do?

This adds support for the Kotlin programming language. Plugins can start being written in or converted to the language with this added.

What testing has been done on this change?

I haven't added any testing yet, but I created an example project by updating the ./bin/create command to use Activity.kt as the main activity instead of Activity.java. Running ./cordova/run compiles the app and the app runs without error.

Checklist

Reported an issue in the JIRA database
Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
Added automated test coverage as appropriate for this change.

joshchandler · 2018-05-14T17:39:58Z

bin/templates/project/Activity.kt

@@ -0,0 +1,38 @@
+/*


I'm not entirely sure if this file should make it in and it's currently not hook up to the create method. I had just created it to do some functional testing. They claim that Kotlin is 100% interchangeable with Java as long as the stdlibs are imported.. Since that's now a thing with this change we could actually use this in favor of Activity.java, if you all are open to it.

dpogue · 2018-05-22T04:14:29Z

These questions might be obvious to someone more familiar with gradle, but I have only limited experience with it. Does this cause additional libraries to be added unconditionally to non-Kotlin projects? Is there an increase in output binary size for existing projects?

joshchandler · 2018-06-06T19:27:27Z

@dpogue Yes this adds the Kotlin library to all projects in all instances. Initially I thought that it'd make sense to add it anyways since it's an official language for Android development..

I'd be open to only adding it if it's explicitly defined in config.xml or something similar to that.

…o kotlin

test/gradle/wrapper/gradle-wrapper.properties

erisu · 2018-09-06T05:01:31Z

bin/templates/project/app/build.gradle

@@ -30,13 +32,14 @@ buildscript {

    dependencies {
        classpath 'com.android.tools.build:gradle:3.1.0'
+        classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version"


Can we wrap all of the Kotlin code and make it conditional by a flag? This is regarding to the same concerns that @dpogue mentioned.

It might be possible that not everyone would use Kotlin.

For example:

if (cdvHelpers.getConfigPreference('EnableKotlin', 'false').toBoolean()) { classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version" }

And this conditional statement would be applied everywhere else relating to Kotlin.

Awesome, did not know this was available. Thanks

@joshchandler Sorry, the cdvHelpers is currently not available in the dependencies scope, as I mentioned in this example.

Wrapping the apply plugin should be OK. Example:

if (cdvHelpers.getConfigPreference('EnableKotlin', 'false').toBoolean()) { apply plugin: 'kotlin-android' }

There is a discussion on a PR that also modifies the the same build.gradle file for other features. It also had the same issue where cdvHelpers is not available in the dependency scope. Adding it is as easy as adding the apply from: '../CordovaLib/cordova.gradle' in the scope BUT I am waiting for any feedback about it.
You can see the comment here: #438 (comment)

Oh I see.. Ok well then I'll wait until feedback is received on that google services PR. I'm not familiar enough with gradle to confidently say that importing into the dependencies scope wouldn't cause any problems. cordova.gradle only consists of helper methods, so I don't see any reason why it would be an issue though.

If it doesn't cause any issues then this would just be another reason to pull it in here and the other dependencies scope further below this file.

bin/templates/project/Activity.kt

…to kotlin

codecov-io · 2018-09-11T14:06:09Z

Codecov Report

Merging #441 into master will increase coverage by 4.92%.
The diff coverage is 65.68%.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   61.64%   66.57%   +4.92%     
==========================================
  Files          16       18       +2     
  Lines        1945     1822     -123     
  Branches      363        0     -363     
==========================================
+ Hits         1199     1213      +14     
+ Misses        746      609     -137

Impacted Files	Coverage Δ
bin/templates/cordova/lib/check_reqs.js	`49.5% <10%> (+0.92%)`	⬆️
bin/templates/cordova/lib/Adb.js	`100% <100%> (ø)`	⬆️
bin/templates/cordova/lib/builders/builders.js	`100% <100%> (ø)`	⬆️
bin/templates/cordova/lib/AndroidManifest.js	`100% <100%> (ø)`	⬆️
bin/templates/cordova/lib/device.js	`100% <100%> (ø)`	⬆️
bin/lib/create.js	`92.72% <100%> (+38.03%)`	⬆️
...n/templates/cordova/lib/builders/ProjectBuilder.js	`69.64% <33.33%> (+24.51%)`	⬆️
bin/templates/cordova/lib/build.js	`26.77% <40%> (-1.69%)`	⬇️
bin/templates/cordova/Api.js	`53.48% <50%> (+9.87%)`	⬆️
bin/templates/cordova/lib/emulator.js	`89.62% <66.66%> (-0.3%)`	⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 478450f...53b3a9c. Read the comment docs.

bin/templates/project/app/build.gradle

brodycj · 2018-11-12T17:36:47Z

What is the status of this proposal?

If we merge PR #507 (Gradle updates) I think this one should be rebased.

dpogue · 2018-11-12T17:39:54Z

The status is that we're trying to find an option that doesn't unconditionally pull kotlin support libraries into all Cordova projects, although it's worth more investigation there to see whether that actually impacts the resulting APK filesize.

…to kotlin

joshchandler · 2019-05-07T16:19:51Z

I had some time to come back to this..

This now successfully ensures that the Kotlin binaries are not added if the config.xml preference EnableKotlin is false or does not exist. Building the example app in the project, without this preference, the compiled apk app-debug.apk is 1.8MB. When set to true, we are adding the binaries for Kotlin and app-debug.apk grows to a size of 2.6MB. So, this change will not affect android projects unless that preference is explicitly set to true.

Given that it adds 0.8MB to the apk, is that large enough to keep this preference set to false by default?

test/build.gradle

timbru31

I've added some suggestions - most of them are probably necessary now because we haven't finished this PR in (nearly) two years.

However, one additional request:
While Android Studio per default saves Kotlin files (.kt) in the src/main/java package, I prefer to separate them into a different folder (src/main/kotlin).
(See 3. of https://developer.android.com/studio/projects/add-kotlin)

Therefore, I'd like to discuss if we should add this code snippet, too:

android {
   sourceSets {
       main.java.srcDirs += 'src/main/kotlin'
   }
}

This would IMHO not hurt to include, but rather gives developers the opportunity to choose between this two styles.

timbru31 · 2020-01-09T17:29:56Z

bin/templates/project/app/build.gradle

@@ -291,6 +300,11 @@ android {

 dependencies {
    implementation fileTree(dir: 'libs', include: '*.jar')
+
+    if (cdvHelpers.getConfigPreference('EnableKotlin', 'false').toBoolean()) {
+        compile "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"


Why do we require the jdk7 stdlib here? This should be changed to kotlin-stdlib instead.

Suggested change

compile "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"

compile "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"

timbru31 · 2020-01-09T17:30:35Z

bin/templates/project/app/build.gradle


 buildscript {
+    ext.kotlin_version = '1.3.31'


Suggested change

ext.kotlin_version = '1.3.31'

ext.kotlin_version = '1.3.61'

timbru31 · 2020-01-09T17:30:55Z

test/app/build.gradle

@@ -51,4 +52,8 @@ dependencies {
    implementation 'com.android.support:appcompat-v7:26.1.0'
    testImplementation 'junit:junit:4.12'
    testImplementation 'org.json:json:20140107'
+    compile "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"


see above

Suggested change

compile "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"

compile "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"

timbru31 · 2020-01-09T17:32:26Z

test/app/build.gradle

+    compile "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
+}
+repositories {
+    mavenCentral()


See https://blog.bintray.com/2015/02/09/android-studio-migration-from-maven-central-to-jcenter/ why jcenter should be used

Suggested change

mavenCentral()

jcenter()

timbru31 · 2020-01-09T17:32:46Z

test/build.gradle

@@ -19,6 +19,7 @@
 // Top-level build file where you can add configuration options common to all sub-projects/modules.

 buildscript {
+    ext.kotlin_version = '1.3.31'


Suggested change

ext.kotlin_version = '1.3.31'

ext.kotlin_version = '1.3.61'

erisu · 2020-01-16T05:28:38Z

Hi @joshchandler, I noticed that there was no activity after the "request change" that was posted a week ago. I went ahead and created a second PR to quickly prepare and get ready to release Kotlin support.

The new PR uses this PR as its base. (New PR: #896)

The PR changes consist of:

Rebased with the latest master branch
Fixes the merge conflicts
Applies suggested changes
Additionally improves

I expect to merge my PR with in the next day or two which will automatically close this PR.

Since your commits were cherry-picked into my new PR, it will still retain your contributions.

Thank you for your contribution and sorry that it took this long to merge in your commit.

joshchandler · 2020-01-16T16:39:40Z

@erisu No problem! Yea, I had seen the feedback and was planning on addressing them soon. Thanks for picking this up!

joshchandler commented May 14, 2018

View reviewed changes

Add Kotlin support

68ce9a8

joshchandler force-pushed the kotlin branch from aba627b to 68ce9a8 Compare May 14, 2018 21:03

Merge branch 'master' of https://github.com/skuid/cordova-android int…

c704191

…o kotlin

erisu reviewed Sep 6, 2018

View reviewed changes

joshchandler added 2 commits September 6, 2018 16:52

Merge branch 'master' of https://github.com/apache/cordova-android in…

8f72c43

…to kotlin

Address comments

478450f

erisu mentioned this pull request Sep 26, 2018

Cordova 9 release plan apache/cordova#10

Closed

33 tasks

phyr0s reviewed Oct 27, 2018

View reviewed changes

bin/templates/project/app/build.gradle Outdated Show resolved Hide resolved

brodycj mentioned this pull request Nov 12, 2018

Gradle updates #507

Merged

3 tasks

dpogue mentioned this pull request Jan 2, 2019

Android App Bundle support #610

Closed

joshchandler added 4 commits May 7, 2019 10:55

Merge branch 'master' of https://github.com/apache/cordova-android in…

9291077

…to kotlin

Remove whitespace changes

67d6dff

Bump kotlin version

bbcbaff

Apply cdvHelper function in dependency scope

53b3a9c

dpogue added this to the 9.0.0 milestone May 8, 2019

timbru31 reviewed Sep 17, 2019

View reviewed changes

test/build.gradle Show resolved Hide resolved

timbru31 mentioned this pull request Jan 9, 2020

Refactoring: Native Kotlin support timbru31/cordova-plugin-lottie-splashscreen#150

Closed

timbru31 requested changes Jan 9, 2020

View reviewed changes

erisu mentioned this pull request Jan 16, 2020

feat: add kotlin support #896

Merged

3 tasks

erisu closed this in #896 Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CB-14089: (android) Add Kotlin support #441

CB-14089: (android) Add Kotlin support #441

joshchandler commented May 14, 2018 •

edited

Loading

joshchandler May 14, 2018

dpogue commented May 22, 2018

joshchandler commented Jun 6, 2018

erisu Sep 6, 2018

erisu Sep 6, 2018

joshchandler Sep 11, 2018

erisu Sep 11, 2018

joshchandler Sep 11, 2018

codecov-io commented Sep 11, 2018 •

edited

Loading

brodycj commented Nov 12, 2018

dpogue commented Nov 12, 2018

joshchandler commented May 7, 2019

timbru31 left a comment

timbru31 Jan 9, 2020

timbru31 Jan 9, 2020

timbru31 Jan 9, 2020

timbru31 Jan 9, 2020

timbru31 Jan 9, 2020

erisu commented Jan 16, 2020

joshchandler commented Jan 16, 2020

	compile "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
	compile "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"

CB-14089: (android) Add Kotlin support #441

CB-14089: (android) Add Kotlin support #441

Conversation

joshchandler commented May 14, 2018 • edited Loading

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

Choose a reason for hiding this comment

dpogue commented May 22, 2018

joshchandler commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 11, 2018 • edited Loading

Codecov Report

brodycj commented Nov 12, 2018

dpogue commented Nov 12, 2018

joshchandler commented May 7, 2019

timbru31 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erisu commented Jan 16, 2020

joshchandler commented Jan 16, 2020

joshchandler commented May 14, 2018 •

edited

Loading

codecov-io commented Sep 11, 2018 •

edited

Loading