Skip to content

Commit

Permalink
refactor: Convert from Gson to Moshi (#428)
Browse files Browse the repository at this point in the history
Moshi is faster to decode JSON at runtime, is actively maintained, has a
smaller memory and method footprint, and a slightly smaller APK size.
Moshi also correctly creates default constructor arguments instead of
leaving them null, which was a source of `NullPointerExceptions` when
using Gson.

The conversion broadly consisted of:

- Adding `@JsonClass(generateAdapter = true)` to data classes that
marshall to/from JSON.

- Replacing `@SerializedName(value = ...)` with `@Json(name = ...)`.

- Replacing Gson instances with Moshi in Retrofit, Hilt, and tests.

- Using Moshi adapters to marshall to/from JSON instead of Gson `toJson`
/ `fromJson`.

- Deleting `Rfc3339DateJsonAdapter` and related code, and using the
equivalent adapter bundled with Moshi.

- Rewriting `GuardedBooleanAdapter` as a more generic `GuardedAdapter`.

- Deleting unused ProGuard rules; Moshi generates adapters using code
generation, not runtime reflection.

The conversion surfaced some bugs which have been fixed.

- Not all audio attachments have attachment size metadata. Don't show
the attachment preview if the metadata is missing.

- Some `throwable` were not being logged correctly.

- The wrong type was being used when parsing the response when sending a
scheduled status.

- Exceptions other than `HttpException` or `IoException` would also
cause a status to be resent. If there's a JSON error parsing a response
the status would be repeatedly sent.

- In tests strings containing error responses were not valid JSON.

- Workaround Mastodon a bug and ensure `filter.keywords` is populated,
mastodon/mastodon#29142
  • Loading branch information
nikclayton authored Feb 9, 2024
1 parent d01c72b commit a3d45ca
Show file tree
Hide file tree
Showing 87 changed files with 731 additions and 888 deletions.
4 changes: 3 additions & 1 deletion app/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ dependencies {

implementation(libs.android.material)

implementation(libs.gson)
implementation(libs.moshi)
implementation(libs.moshi.adapters)
ksp(libs.moshi.codegen)

implementation(libs.bundles.retrofit)

Expand Down
13 changes: 0 additions & 13 deletions app/proguard-rules.pro
Original file line number Diff line number Diff line change
Expand Up @@ -57,19 +57,6 @@
# error reports
-keepnames class *

# https://github.com/google/gson/blob/master/examples/android-proguard-example/proguard.cfg

# Prevent proguard from stripping interface information from TypeAdapter, TypeAdapterFactory,
# JsonSerializer, JsonDeserializer instances (so they can be used in @JsonAdapter)
-keep class * extends com.google.gson.TypeAdapter
-keep class * implements com.google.gson.TypeAdapterFactory
-keep class * implements com.google.gson.JsonSerializer
-keep class * implements com.google.gson.JsonDeserializer

# Retain generic signatures of TypeToken and its subclasses with R8 version 3.0 and higher.
-keep,allowobfuscation,allowshrinking class com.google.gson.reflect.TypeToken
-keep,allowobfuscation,allowshrinking class * extends com.google.gson.reflect.TypeToken

# Retain generic signatures of classes used in MastodonApi so Retrofit works
-keep,allowobfuscation,allowshrinking class io.reactivex.rxjava3.core.Single
-keep,allowobfuscation,allowshrinking class retrofit2.Response
Expand Down
6 changes: 2 additions & 4 deletions app/src/fdroid/kotlin/app/pachli/di/UpdateCheckModule.kt
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,14 @@ package app.pachli.di

import app.pachli.updatecheck.FdroidService
import at.connyduck.calladapter.networkresult.NetworkResultCallAdapterFactory
import com.google.gson.Gson
import dagger.Module
import dagger.Provides
import dagger.hilt.InstallIn
import dagger.hilt.components.SingletonComponent
import javax.inject.Singleton
import okhttp3.OkHttpClient
import retrofit2.Retrofit
import retrofit2.converter.gson.GsonConverterFactory
import retrofit2.converter.moshi.MoshiConverterFactory
import retrofit2.create

@InstallIn(SingletonComponent::class)
Expand All @@ -37,11 +36,10 @@ object UpdateCheckModule {
@Singleton
fun providesFdroidService(
httpClient: OkHttpClient,
gson: Gson,
): FdroidService = Retrofit.Builder()
.baseUrl("https://f-droid.org")
.client(httpClient)
.addConverterFactory(GsonConverterFactory.create(gson))
.addConverterFactory(MoshiConverterFactory.create())
.addCallAdapterFactory(NetworkResultCallAdapterFactory.create())
.build()
.create()
Expand Down
3 changes: 3 additions & 0 deletions app/src/fdroid/kotlin/app/pachli/updatecheck/FdroidService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,19 @@ package app.pachli.updatecheck

import androidx.annotation.Keep
import at.connyduck.calladapter.networkresult.NetworkResult
import com.squareup.moshi.JsonClass
import retrofit2.http.GET
import retrofit2.http.Path

@Keep
@JsonClass(generateAdapter = true)
data class FdroidPackageVersion(
val versionName: String,
val versionCode: Int,
)

@Keep
@JsonClass(generateAdapter = true)
data class FdroidPackage(
val packageName: String,
val suggestedVersionCode: Int,
Expand Down
5 changes: 1 addition & 4 deletions app/src/github/kotlin/app/pachli/di/UpdateCheckModule.kt
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,13 @@ package app.pachli.di

import app.pachli.updatecheck.GitHubService
import at.connyduck.calladapter.networkresult.NetworkResultCallAdapterFactory
import com.google.gson.Gson
import dagger.Module
import dagger.Provides
import dagger.hilt.InstallIn
import dagger.hilt.components.SingletonComponent
import javax.inject.Singleton
import okhttp3.OkHttpClient
import retrofit2.Retrofit
import retrofit2.converter.gson.GsonConverterFactory
import retrofit2.create

@InstallIn(SingletonComponent::class)
Expand All @@ -37,11 +35,10 @@ object UpdateCheckModule {
@Singleton
fun providesGitHubService(
httpClient: OkHttpClient,
gson: Gson,
): GitHubService = Retrofit.Builder()
.baseUrl("https://api.github.com")
.client(httpClient)
.addConverterFactory(GsonConverterFactory.create(gson))
.addConverterFactory(MoshiConverterFactory.create())
.addCallAdapterFactory(NetworkResultCallAdapterFactory.create())
.build()
.create()
Expand Down
9 changes: 6 additions & 3 deletions app/src/github/kotlin/app/pachli/updatecheck/GithubService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,26 @@ package app.pachli.updatecheck

import androidx.annotation.Keep
import at.connyduck.calladapter.networkresult.NetworkResult
import com.google.gson.annotations.SerializedName
import com.squareup.moshi.Json
import com.squareup.moshi.JsonClass
import retrofit2.http.GET
import retrofit2.http.Path

@Keep
@JsonClass(generateAdapter = true)
data class GitHubReleaseAsset(
/** File name for the asset, e.g., "113.apk" */
val name: String,

/** MIME content type for the asset, e.g., "application/vnd.android.package-archive" */
@SerializedName("content_type") val contentType: String,
@Json(name = "content_type") val contentType: String,
)

@Keep
@JsonClass(generateAdapter = true)
data class GitHubRelease(
/** URL for the release's web page */
@SerializedName("html_url") val htmlUrl: String,
@Json(name = "html_url") val htmlUrl: String,
val assets: List<GitHubReleaseAsset>,
)

Expand Down
5 changes: 1 addition & 4 deletions app/src/main/java/app/pachli/ViewMediaActivity.kt
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,7 @@ import android.view.MenuItem
import android.view.View
import android.webkit.MimeTypeMap
import android.widget.Toast
import androidx.core.app.ActivityCompat.invalidateOptionsMenu
import androidx.core.app.ActivityCompat.requestPermissions
import androidx.core.app.ShareCompat
import androidx.core.content.ContextCompat.getSystemService
import androidx.core.content.FileProvider
import androidx.fragment.app.FragmentActivity
import androidx.lifecycle.Lifecycle
Expand Down Expand Up @@ -89,7 +86,7 @@ class ViewMediaActivity : BaseActivity(), ViewImageFragment.PhotoActionsListener
var isToolbarVisible = true
private set

private var attachments: ArrayList<AttachmentViewData>? = null
private var attachments: List<AttachmentViewData>? = null
private val toolbarVisibilityListeners = mutableListOf<ToolbarVisibilityListener>()
private var imageUrl: String? = null

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class ReportNotificationViewHolder(
binding.notificationSummary.text = itemView.context.getString(
R.string.notification_summary_report_format,
getRelativeTimeSpanString(itemView.context, report.createdAt.time, Date().time),
report.status_ids?.size ?: 0,
report.statusIds?.size ?: 0,
)
binding.notificationCategory.text = getTranslatedCategory(itemView.context, report.category)

Expand Down
2 changes: 2 additions & 0 deletions app/src/main/java/app/pachli/adapter/StatusBaseViewHolder.kt
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,8 @@ abstract class StatusBaseViewHolder<T : IStatusViewData> protected constructor(i
protected fun hasPreviewableAttachment(attachments: List<Attachment>): Boolean {
for (attachment in attachments) {
if (attachment.type == Attachment.Type.UNKNOWN) return false

if (attachment.meta?.original?.width == null && attachment.meta?.small?.width == null) return false
}
return true
}
Expand Down
9 changes: 6 additions & 3 deletions app/src/main/java/app/pachli/appstore/CacheUpdater.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,22 @@ package app.pachli.appstore

import app.pachli.core.accounts.AccountManager
import app.pachli.core.database.dao.TimelineDao
import com.google.gson.Gson
import app.pachli.core.network.model.Poll
import com.squareup.moshi.Moshi
import com.squareup.moshi.adapter
import javax.inject.Inject
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancel
import kotlinx.coroutines.launch

@OptIn(ExperimentalStdlibApi::class)
class CacheUpdater @Inject constructor(
eventHub: EventHub,
accountManager: AccountManager,
timelineDao: TimelineDao,
gson: Gson,
moshi: Moshi,
) {
private val scope = CoroutineScope(Dispatchers.IO + SupervisorJob())

Expand All @@ -34,7 +37,7 @@ class CacheUpdater @Inject constructor(
is StatusDeletedEvent ->
timelineDao.delete(accountId, event.statusId)
is PollVoteEvent -> {
val pollString = gson.toJson(event.poll)
val pollString = moshi.adapter<Poll>().toJson(event.poll)
timelineDao.setVoted(accountId, event.statusId, pollString)
}
is PinEvent ->
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/app/pachli/appstore/Events.kt
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ data class StatusDeletedEvent(val statusId: String) : Event
/** A status the user wrote was successfully sent */
// TODO: Rename, calling it "Composed" does not imply anything about the sent state
data class StatusComposedEvent(val status: Status) : Event
data class StatusScheduledEvent(val status: Status) : Event
data object StatusScheduledEvent : Event
data class StatusEditedEvent(val originalId: String, val status: Status) : Event
data class ProfileEditedEvent(val newProfileData: Account) : Event
data class FilterChangedEvent(val filterKind: Filter.Kind) : Event
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ import app.pachli.core.network.model.Error
import app.pachli.core.network.model.Links
import app.pachli.core.network.model.Notification
import app.pachli.core.network.retrofit.MastodonApi
import com.google.gson.Gson
import com.squareup.moshi.Moshi
import com.squareup.moshi.adapter
import javax.inject.Inject
import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope
Expand All @@ -37,9 +38,10 @@ private val INVALID = LoadResult.Invalid<String, Notification>()
/** [PagingSource] for Mastodon Notifications, identified by the Notification ID */
class NotificationsPagingSource @Inject constructor(
private val mastodonApi: MastodonApi,
private val gson: Gson,
private val moshi: Moshi,
private val notificationFilter: Set<Notification.Type>,
) : PagingSource<String, Notification>() {
@OptIn(ExperimentalStdlibApi::class)
override suspend fun load(params: LoadParams<String>): LoadResult<String, Notification> {
Timber.d("load() with ${params.javaClass.simpleName} for key: ${params.key}")

Expand Down Expand Up @@ -67,12 +69,12 @@ class NotificationsPagingSource @Inject constructor(
if (errorBody.isBlank()) return@let "no reason given"

val error = try {
gson.fromJson(errorBody, Error::class.java)
moshi.adapter<Error>().fromJson(errorBody)!!
} catch (e: Exception) {
return@let "$errorBody ($e)"
}

when (val desc = error.error_description) {
when (val desc = error.errorDescription) {
null -> error.error
else -> "${error.error}: $desc"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import androidx.paging.PagingSource
import app.pachli.core.common.di.ApplicationScope
import app.pachli.core.network.model.Notification
import app.pachli.core.network.retrofit.MastodonApi
import com.google.gson.Gson
import com.squareup.moshi.Moshi
import javax.inject.Inject
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.async
Expand All @@ -36,7 +36,7 @@ import timber.log.Timber

class NotificationsRepository @Inject constructor(
private val mastodonApi: MastodonApi,
private val gson: Gson,
private val moshi: Moshi,
@ApplicationScope private val externalScope: CoroutineScope,
) {
private var factory: InvalidatingPagingSourceFactory<String, Notification>? = null
Expand All @@ -53,7 +53,7 @@ class NotificationsRepository @Inject constructor(
Timber.d("getNotificationsStream(), filtering: $filter")

factory = InvalidatingPagingSourceFactory {
NotificationsPagingSource(mastodonApi, gson, filter)
NotificationsPagingSource(mastodonApi, moshi, filter)
}

return Pager(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class ScheduledStatusViewModel @Inject constructor(
pagingSourceFactory.remove(status)
},
{ throwable ->
Timber.w("Error deleting scheduled status", throwable)
Timber.w("Error deleting scheduled status: $throwable")
},
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import app.pachli.components.search.SearchType
import app.pachli.core.network.model.SearchResult
import app.pachli.core.network.retrofit.MastodonApi
import at.connyduck.calladapter.networkresult.getOrElse
import timber.log.Timber

class SearchPagingSource<T : Any>(
private val mastodonApi: MastodonApi,
Expand Down Expand Up @@ -61,7 +62,10 @@ class SearchPagingSource<T : Any>(
limit = params.loadSize,
offset = currentKey,
following = false,
).getOrElse { return LoadResult.Error(it) }
).getOrElse {
Timber.w(it)
return LoadResult.Error(it)
}

val res = parser(data)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ abstract class SearchFragment<T : Any> :
}

adapter.addLoadStateListener { loadState ->

if (loadState.refresh is LoadState.Error) {
showError()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import app.pachli.util.EmptyPagingSource
import app.pachli.viewdata.StatusViewData
import at.connyduck.calladapter.networkresult.NetworkResult
import at.connyduck.calladapter.networkresult.fold
import com.google.gson.Gson
import com.squareup.moshi.Moshi
import javax.inject.Inject
import javax.inject.Singleton
import kotlinx.coroutines.CoroutineScope
Expand All @@ -66,7 +66,7 @@ class CachedTimelineRepository @Inject constructor(
val timelineDao: TimelineDao,
private val remoteKeyDao: RemoteKeyDao,
private val translatedStatusDao: TranslatedStatusDao,
private val gson: Gson,
private val moshi: Moshi,
@ApplicationScope private val externalScope: CoroutineScope,
) {
private var factory: InvalidatingPagingSourceFactory<Int, TimelineStatusWithAccount>? = null
Expand Down Expand Up @@ -118,7 +118,7 @@ class CachedTimelineRepository @Inject constructor(
transactionProvider,
timelineDao,
remoteKeyDao,
gson,
moshi,
),
pagingSourceFactory = factory!!,
).flow
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import app.pachli.core.database.model.TimelineStatusWithAccount
import app.pachli.core.network.model.Links
import app.pachli.core.network.model.Status
import app.pachli.core.network.retrofit.MastodonApi
import com.google.gson.Gson
import com.squareup.moshi.Moshi
import java.io.IOException
import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope
Expand All @@ -54,7 +54,7 @@ class CachedTimelineRemoteMediator(
private val transactionProvider: TransactionProvider,
private val timelineDao: TimelineDao,
private val remoteKeyDao: RemoteKeyDao,
private val gson: Gson,
private val moshi: Moshi,
) : RemoteMediator<Int, TimelineStatusWithAccount>() {
private val activeAccount = accountManager.activeAccount!!

Expand Down Expand Up @@ -254,17 +254,17 @@ class CachedTimelineRemoteMediator(
@Transaction
private suspend fun insertStatuses(statuses: List<Status>) {
for (status in statuses) {
timelineDao.insertAccount(TimelineAccountEntity.from(status.account, activeAccount.id, gson))
timelineDao.insertAccount(TimelineAccountEntity.from(status.account, activeAccount.id, moshi))
status.reblog?.account?.let {
val account = TimelineAccountEntity.from(it, activeAccount.id, gson)
val account = TimelineAccountEntity.from(it, activeAccount.id, moshi)
timelineDao.insertAccount(account)
}

timelineDao.insertStatus(
TimelineStatusEntity.from(
status,
timelineUserId = activeAccount.id,
gson = gson,
moshi = moshi,
),
)
}
Expand Down
Loading

0 comments on commit a3d45ca

Please sign in to comment.