Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
package ai.elimu.content_provider.ui.letter

import ai.elimu.content_provider.BaseApplication
import ai.elimu.content_provider.R
import ai.elimu.content_provider.rest.LettersService
import ai.elimu.content_provider.room.GsonToRoomConverter
import ai.elimu.content_provider.room.db.RoomDb
import ai.elimu.model.v2.gson.content.LetterGson
import android.os.Bundle
import android.util.Log
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import android.widget.ProgressBar
import android.widget.TextView
import androidx.fragment.app.Fragment
import androidx.lifecycle.Observer
import androidx.lifecycle.ViewModelProvider
import com.google.android.material.snackbar.Snackbar
import retrofit2.Call
import retrofit2.Callback
import retrofit2.Response
import java.util.concurrent.Executors

class LettersFragment : Fragment() {
private var lettersViewModel: LettersViewModel? = null

private var progressBar: ProgressBar? = null

private var textView: TextView? = null

private val TAG = javaClass.name

override fun onCreateView(
inflater: LayoutInflater,
container: ViewGroup?,
savedInstanceState: Bundle?
): View? {
Log.i(TAG, "onCreateView")

lettersViewModel = ViewModelProvider(this).get(
LettersViewModel::class.java
)
val root = inflater.inflate(R.layout.fragment_letters, container, false)
progressBar = root.findViewById(R.id.progress_bar_letters)
textView = root.findViewById(R.id.text_letters) as? TextView
lettersViewModel!!.text.observe(viewLifecycleOwner, object : Observer<String?> {
override fun onChanged(s: String?) {
Log.i(TAG, "onChanged")
textView?.text = s
}
})
return root
}

override fun onStart() {
Log.i(TAG, "onStart")
super.onStart()

// Download Letters from REST API, and store them in the database
val baseApplication = activity!!.application as BaseApplication
val retrofit = baseApplication.retrofit
val lettersService = retrofit.create(LettersService::class.java)
val letterGsonsCall = lettersService.listLetters()
Log.i(TAG, "letterGsonsCall.request(): " + letterGsonsCall.request())
letterGsonsCall.enqueue(object : Callback<List<LetterGson>> {
override fun onResponse(
call: Call<List<LetterGson>>,
response: Response<List<LetterGson>>
) {
Log.i(TAG, "onResponse")

Log.i(TAG, "response: $response")
if (response.isSuccessful) {
val letterGsons = response.body()!!
Log.i(TAG, "letterGsons.size(): " + letterGsons.size)

if (letterGsons.size > 0) {
processResponseBody(letterGsons)
}
} else {
// Handle error
Snackbar.make(textView!!, response.toString(), Snackbar.LENGTH_LONG)
.setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
.show()
progressBar!!.visibility = View.GONE
}
}

override fun onFailure(call: Call<List<LetterGson>>, t: Throwable) {
Log.e(TAG, "onFailure", t)

Log.e(TAG, "t.getCause():", t.cause)

// Handle error
Snackbar.make(textView!!, t.cause.toString(), Snackbar.LENGTH_LONG)
.setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
.show()
progressBar!!.visibility = View.GONE
}
})
}
Comment on lines +56 to +102
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using non-null assertions (!!) and handle activity lifecycle safely.

The code uses multiple non-null assertions which could lead to NullPointerExceptions. Also, accessing activity!! is risky if the fragment is detached.

-        val baseApplication = activity!!.application as BaseApplication
+        val activity = activity ?: return
+        val baseApplication = activity.application as BaseApplication

-        Snackbar.make(textView!!, response.toString(), Snackbar.LENGTH_LONG)
+        textView?.let { view ->
+            Snackbar.make(view, response.toString(), Snackbar.LENGTH_LONG)
+                .setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
+                .show()
+        }
-        progressBar!!.visibility = View.GONE
+        progressBar?.visibility = View.GONE

Also, consider handling potential null response body:

-                    val letterGsons = response.body()!!
+                    val letterGsons = response.body() ?: return

And in the onFailure method:

-                Snackbar.make(textView!!, t.cause.toString(), Snackbar.LENGTH_LONG)
+                textView?.let { view ->
+                    Snackbar.make(view, t.cause?.toString() ?: t.message ?: "Unknown error", Snackbar.LENGTH_LONG)
+                        .setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
+                        .show()
+                }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
override fun onStart() {
Log.i(TAG, "onStart")
super.onStart()
// Download Letters from REST API, and store them in the database
val baseApplication = activity!!.application as BaseApplication
val retrofit = baseApplication.retrofit
val lettersService = retrofit.create(LettersService::class.java)
val letterGsonsCall = lettersService.listLetters()
Log.i(TAG, "letterGsonsCall.request(): " + letterGsonsCall.request())
letterGsonsCall.enqueue(object : Callback<List<LetterGson>> {
override fun onResponse(
call: Call<List<LetterGson>>,
response: Response<List<LetterGson>>
) {
Log.i(TAG, "onResponse")
Log.i(TAG, "response: $response")
if (response.isSuccessful) {
val letterGsons = response.body()!!
Log.i(TAG, "letterGsons.size(): " + letterGsons.size)
if (letterGsons.size > 0) {
processResponseBody(letterGsons)
}
} else {
// Handle error
Snackbar.make(textView!!, response.toString(), Snackbar.LENGTH_LONG)
.setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
.show()
progressBar!!.visibility = View.GONE
}
}
override fun onFailure(call: Call<List<LetterGson>>, t: Throwable) {
Log.e(TAG, "onFailure", t)
Log.e(TAG, "t.getCause():", t.cause)
// Handle error
Snackbar.make(textView!!, t.cause.toString(), Snackbar.LENGTH_LONG)
.setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
.show()
progressBar!!.visibility = View.GONE
}
})
}
override fun onStart() {
Log.i(TAG, "onStart")
super.onStart()
// Download Letters from REST API, and store them in the database
val activity = activity ?: return
val baseApplication = activity.application as BaseApplication
val retrofit = baseApplication.retrofit
val lettersService = retrofit.create(LettersService::class.java)
val letterGsonsCall = lettersService.listLetters()
Log.i(TAG, "letterGsonsCall.request(): " + letterGsonsCall.request())
letterGsonsCall.enqueue(object : Callback<List<LetterGson>> {
override fun onResponse(
call: Call<List<LetterGson>>,
response: Response<List<LetterGson>>
) {
Log.i(TAG, "onResponse")
Log.i(TAG, "response: $response")
if (response.isSuccessful) {
val letterGsons = response.body() ?: return
Log.i(TAG, "letterGsons.size(): " + letterGsons.size)
if (letterGsons.size > 0) {
processResponseBody(letterGsons)
}
} else {
// Handle error
textView?.let { view ->
Snackbar.make(view, response.toString(), Snackbar.LENGTH_LONG)
.setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
.show()
}
progressBar?.visibility = View.GONE
}
}
override fun onFailure(call: Call<List<LetterGson>>, t: Throwable) {
Log.e(TAG, "onFailure", t)
Log.e(TAG, "t.getCause():", t.cause)
// Handle error
textView?.let { view ->
Snackbar.make(
view,
t.cause?.toString() ?: t.message ?: "Unknown error",
Snackbar.LENGTH_LONG
)
.setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
.show()
}
progressBar?.visibility = View.GONE
}
})
}


private fun processResponseBody(letterGsons: List<LetterGson>) {
Log.i(TAG, "processResponseBody")

val executorService = Executors.newSingleThreadExecutor()
executorService.execute(object : Runnable {
override fun run() {
Log.i(TAG, "run")

val roomDb = RoomDb.getDatabase(context)
val letterDao = roomDb.letterDao()

// Empty the database table before downloading up-to-date content
letterDao.deleteAll()

for (letterGson in letterGsons) {
Log.i(TAG, "letterGson.getId(): " + letterGson.id)

// Store the Letter in the database
val letter = GsonToRoomConverter.getLetter(letterGson)
letterDao.insert(letter)
Log.i(TAG, "Stored Letter in database with ID " + letter.id)
}

// Update the UI
val letters = letterDao.loadAllOrderedByUsageCount()
Log.i(TAG, "letters.size(): " + letters.size)
activity!!.runOnUiThread {
textView!!.text = "letters.size(): " + letters.size
Snackbar.make(
textView!!,
"letters.size(): " + letters.size,
Snackbar.LENGTH_LONG
).show()
progressBar!!.visibility = View.GONE
}
}
})
}
Comment on lines +104 to +141
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using Kotlin Coroutines instead of ExecutorService for background operations.

Using Kotlin Coroutines would make the code more concise and easier to manage for asynchronous operations. Also, there are multiple non-null assertions on UI elements that should be handled safely.

-        val executorService = Executors.newSingleThreadExecutor()
-        executorService.execute(object : Runnable {
-            override fun run() {
+        viewLifecycleOwner.lifecycleScope.launch(Dispatchers.IO) {
                // Database operations...

-                activity!!.runOnUiThread {
-                    textView!!.text = "letters.size(): " + letters.size
-                    Snackbar.make(
-                        textView!!,
-                        "letters.size(): " + letters.size,
-                        Snackbar.LENGTH_LONG
-                    ).show()
-                    progressBar!!.visibility = View.GONE
-                }
+                withContext(Dispatchers.Main) {
+                    textView?.text = "letters.size(): " + letters.size
+                    textView?.let { view ->
+                        Snackbar.make(
+                            view,
+                            "letters.size(): " + letters.size,
+                            Snackbar.LENGTH_LONG
+                        ).show()
+                    }
+                    progressBar?.visibility = View.GONE
+                }
-            }
-        })

First, add the necessary imports:

import androidx.lifecycle.lifecycleScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext

⚠️ Potential issue

Handle potential null context safely when accessing the database.

The code accesses the context without null checking when getting the database instance, which could cause a crash if the fragment is detached.

-                val roomDb = RoomDb.getDatabase(context)
+                val context = context ?: return@launch // or return@execute if not using coroutines
+                val roomDb = RoomDb.getDatabase(context)

Committable suggestion skipped: line range outside the PR's diff.

}