Codebase Improvements

[serebit]

I have some general suggestions (that I'm willing to put into practice via PRs) for improving FlorisBoard's codebase. These include:

• Cleaning up existing dependencies (some deps are unnecessary, for example, such as stdlib-jre7)
• Using kotlinx-serialization instead of moshi (smaller runtime size, slightly faster, and no reliance on reflection)
• Using Kotlin Gradle build scripts rather than Groovy build scripts and cleaning up build logic
• Storing source files at source root rather than within single-nested subpackages (i.e. src/main/kotlin/App.kt rather than src/main/java/dev/patrickgold/florisboard/App.kt)

This list isn't exhaustive-- there are other things that could be done too. As these are rather major changes, I wanted to seek approval first.

[patrickgold]

Thanks for your suggestions for cleaning up the codebase! Here are my thoughts:

Cleaning up existing dependencies (some deps are unnecessary, for example, such as stdlib-jre7)

I totally agree with this. This is a nice little corpse from the days I was still using Java 1.7. If you find any other unused include dependencies, feel free to remove them!

Using kotlinx-serialization instead of moshi (smaller runtime size, slightly faster, and no reliance on reflection)

The reason I went with Moshi is that I wanted to have a JSON deserializer which has a good range of features and is relatively performant. My first attempt was to use Gson but I quickly realized that Gson is a resource and performance hog. So I went with Moshi because it seemed more easy to st up that using kotlinx-serialization and was way faster. In general though I don't have a good reason why we shouldn't use kotlinx-serialization instead of Moshi, as long as the end result is still the same. And if it is slightly faster, it is even better :)

Using Kotlin Gradle build scripts rather than Groovy build scripts and cleaning up build logic

I definitely agree with cleaning up the build logic, though does Kotlin Gradle have any advantage in terms of build time over Groovy? From what I've seen the syntax slightly changes (it both gets better and worse at the same time IMO) but the build time is relatively the same.

Storing source files at source root rather than within single-nested subpackages (i.e. src/main/kotlin/App.kt rather than src/main/java/dev/patrickgold/florisboard/App.kt)

This is the only suggestion I do not really agree with, as one can easily read the package name for a class based on all single-nested subpackages and I kinda like this approach. The only thing that would make sense to me is to rename it to src/main/kotlin/dev/patrickgold/florisboard/App.kt, as FlorisBoard is a 100% Kotlin Android app. This should only be changed though once I merge my suggestions-phase1 branch back into master to avoid a merge disaster :)

[serebit]

The reason I went with Moshi is that I wanted to have a JSON deserializer which has a good range of features and is relatively performant. My first attempt was to use Gson but I quickly realized that Gson is a resource and performance hog. So I went with Moshi because it seemed more easy to st up that using kotlinx-serialization and was way faster. In general though I don't have a good reason why we shouldn't use kotlinx-serialization instead of Moshi, as long as the end result is still the same. And if it is slightly faster, it is even better :)

Even the Gson maintainers don't like Gson! It's pretty awful. Moshi is a good runner-up, but it still does things mostly Java-style. kotlinx-serialization was pretty awkward to use pre-1.0, but the latest version is actually quite nice and reasonable to use so long as the code is architected for it. Plus, it has the benefit of using zero reflection, so the runtime is pretty light (about half the size of Moshi), and it's a bit faster in deserialization (though not by a whole lot).

I definitely agree with cleaning up the build logic, though does Kotlin Gradle have any advantage in terms of build time over Groovy? From what I've seen the syntax slightly changes (it both gets better and worse at the same time IMO) but the build time is relatively the same.

Initial from-scratch compilation with build.gradle.kts is generally slower than build.gradle, as Gradle has to cache a compiled version of the script (in my testing, the overall fresh build time goes from 27s to 32s for this project). Build time overall after the script compilation has been cached is generally the same. As for syntax, it was originally pretty obnoxious, but it's improved with time-- most commonly used libraries and plugins now have first-class support for the gradle.kts syntax. Here's what the current configurations would look like when converted to an idiomatic gradle.kts syntax:

build.gradle.kts

```kotlin plugins { base // adds clean task } subprojects { repositories { google() jcenter() } } ```

settings.gradle.kts

```kotlin rootProject.name = "FlorisBoard" include(":app") pluginManagement { repositories { gradlePluginPortal() google() } // allows the plugins syntax to be used with the android gradle plugin resolutionStrategy.eachPlugin { if (requested.id.id == "com.android.application") { useModule("com.android.tools.build:gradle:${requested.version}") } } } ```

app/build.gradle.kts

```kotlin plugins { id("com.android.application") version "4.1.1" kotlin("android") version "1.4.10" kotlin("android.extensions") version "1.4.10" } android { compileSdkVersion(29) buildToolsVersion("29.0.2") compileOptions { sourceCompatibility = JavaVersion.VERSION_1_8 targetCompatibility = JavaVersion.VERSION_1_8 } kotlinOptions { jvmTarget = JavaVersion.VERSION_1_8.toString() } defaultConfig { applicationId = "dev.patrickgold.florisboard" minSdkVersion(23) targetSdkVersion(29) versionCode(26) versionName("0.3.7") testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" } buildFeatures { viewBinding = true } buildTypes { named("debug").configure { applicationIdSuffix = ".debug" resValue("string", "floris_app_name", "FlorisBoard Debug") } named("release").configure { isMinifyEnabled = false proguardFiles.add(getDefaultProguardFile("proguard-android-optimize.txt")) resValue("string", "floris_app_name", "@string/app_name") } } testOptions { unitTests { isIncludeAndroidResources = true } } } dependencies { implementation("org.jetbrains.kotlin", "kotlin-stdlib-jdk7") implementation("org.jetbrains.kotlin", "kotlin-reflect") implementation("androidx.appcompat", "appcompat", "1.2.0") implementation("androidx.core", "core-ktx", "1.3.2") implementation("androidx.preference", "preference-ktx", "1.1.1") implementation("androidx.constraintlayout", "constraintlayout", "2.0.4") implementation("com.google.android", "flexbox", "2.0.1") implementation("com.squareup.moshi", "moshi-kotlin", "1.9.2") implementation("com.squareup.moshi", "moshi-adapters", "1.9.2") implementation("com.google.android.material", "material", "1.2.1") implementation("org.jetbrains.kotlinx", "kotlinx-coroutines-core", "1.3.7") implementation("org.jetbrains.kotlinx", "kotlinx-coroutines-android", "1.3.7") implementation("com.jaredrummler", "colorpicker", "1.1.0") implementation("com.jakewharton.timber", "timber", "4.7.1") implementation("com.michael-bull.kotlin-result", "kotlin-result", "1.1.9") implementation("com.nambimobile.widgets", "expandable-fab", "1.0.2") testImplementation("junit", "junit", "4.12") testImplementation("androidx.test", "core", "1.3.0") testImplementation("org.mockito", "mockito-core", "1.10.19") testImplementation("org.mockito", "mockito-inline", "2.13.0") testImplementation("org.robolectric", "robolectric", "4.4") androidTestImplementation("androidx.test.ext", "junit", "1.1.2") androidTestImplementation("androidx.test.espresso", "espresso-core", "3.3.0") } ```

In my opinion, the main benefit of the kts syntax is significantly better IDE support and a much more declarative method of writing build scripts. The initial compilation speed is also being addressed with the new Kotlin compiler and additional caching measures-- Gradle 6.8 introduces compilation avoidance based on the classpath ABI, for example.

This is the only suggestion I do not really agree with, as one can easily read the package name for a class based on all single-nested subpackages and I kinda like this approach. The only thing that would make sense to me is to rename it to src/main/kotlin/dev/patrickgold/florisboard/App.kt, as FlorisBoard is a 100% Kotlin Android app. This should only be changed though once I merge my suggestions-phase1 branch back into master to avoid a merge disaster :)

This is fair-- while the root convention is the one recommended by JetBrains, it also goes against the conventions that Java has held for a long time.

[patrickgold]

Thanks for your detailed response!

Even the Gson maintainers don't like Gson!

Well that says a lot 😄

Initial from-scratch compilation with build.gradle.kts is generally slower than build.gradle ...

Hmm yeah if the build time increases for the first time build but then stays the same, this is ok for me. The syntax of the grade.kts files you provided looks a lot better and more consistent from what I've seen when I searched in Google. It seems that in the last 6-8 months things definitely have improved. The only thing which I still don't like is this:

        applicationId = "dev.patrickgold.florisboard"
        minSdkVersion(23)

Why is the applicationId a property and the minSdkVersion a function, thus requiring two different syntaxes which in the Groovy system shared the same syntax... But this is pretty much nitpicking by me and I think that we should migrate to the Kotlin Gradle system as I also think that it is the future.

In my opinion, the main benefit of the kts syntax is significantly better IDE support

Granted this is one thing I missed a lot from Groovy. Another reason to switch to Kotlin Gradle then!

[Surendrajat]

I also have few suggestions related to this:

• Adding an .editorconfig for a code/formatting consistency across editors and platforms.
• It'd be nice to add some sort of code-formatter or formatting rules for non-Kotlin files such as JSON files. While trying to update a keyboard layout, the manual formatting feels a bit fragile and might end up producing unnecessary diffs. I'm not sure if something like Prettier exists for Java/Kotlin/Gradle projects though.

[serebit]

Adding an .editorconfig for a code/formatting consistency across editors and platforms.

Great idea, I'll do that too.

It'd be nice to add some sort of code-formatter or formatting rules for non-Kotlin files such as JSON files. While trying to update a keyboard layout, the manual formatting feels a bit fragile and might end up producing unnecessary diffs. I'm not sure if something like Prettier exists for Java/Kotlin/Gradle projects though.

This is trickier, but I'll see if I can find anything. .editorconfig might actually be able to help with this.

[serebit]

I also had some additional thoughts:

• Replace kotlin-result with the builtin kotlin.Result class. Requires a compiler arg to allow returning for now, that being -Xallow-result-return-type, but imo the fewer dependencies the better if functionality already exists in the language
• Add extremely light plugin to fetch new dep versions-- this plugin works perfectly, and can fetch updated versions for all dependencies and plugins in the repository. It also has a negligible impact on the Gradle configuration time-- somewhere in the millisecond range
• Update dependencies, plugins, and Gradle wrapper to latest versions (exempting the Android SDK version)
• Remove unused/redundant files (app/proguard-rules.pro, which contains no rules; and app/.gitignore, which contains one redundant entry that's already covered by /.gitignore)

[patrickgold]

Adding an .editorconfig for a code/formatting consistency across editors and platforms.

That's definitely a great idea which I totally support.

Replace kotlin-result with the builtin kotlin.Result class. Requires a compiler arg to allow returning for now, that being -Xallow-result-return-type, but imo the fewer dependencies the better if functionality already exists in the language

There were several reasons why I avoided the kotlin.Result class, namely being that you can only return an object inherited from Throwable on failure, whereas normally you can return anything, regardless if the result resembles a success or failure. Also the currently used kotlin-result library has a much more closer syntax to the behavior in Rust (where I first learnt to work with results) and I like the Ok(...) and Err(...) approach a bit more.

Remove unused/redundant files (app/proguard-rules.pro, which contains no rules; and app/.gitignore, which contains one redundant entry that's already covered by /.gitignore)

They are redundant now because there are currently no app-specific proguard files for the app module, only project-wide. Though imo it doesn't hurt to just leave it in there, if we need to adjust the proguard rules later on for the app module. The .gitignore file was auto-generated by Android Studio and never touched since then.

[serebit]

There were several reasons why I avoided the kotlin.Result class, namely being that you can only return an object inherited from Throwable on failure, whereas normally you can return anything, regardless if the result resembles a success or failure. Also the currently used kotlin-result library has a much more closer syntax to the behavior in Rust (where I first learnt to work with results) and I like the Ok(...) and Err(...) approach a bit more.

I'd still advocate for using the kotlin.Result class anyway; to me, the reduction of dependency count is worth the minor change in syntax, considering how little the Result class is used in FlorisBoard's codebase as is. That said, if I do submit a pull request for swapping kotlin-result for kotlin.Result, it'll be separate from the main PR so you have the ability to reject it without rejecting all other changes along with it.