Skip to content
Closed
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,166 @@
package ai.elimu.content_provider.ui.audio

import ai.elimu.content_provider.BaseApplication
import ai.elimu.content_provider.R
import ai.elimu.content_provider.rest.AudiosService
import ai.elimu.content_provider.room.GsonToRoomConverter
import ai.elimu.content_provider.room.db.RoomDb
import ai.elimu.content_provider.util.FileHelper
import ai.elimu.content_provider.util.MultimediaDownloader
import ai.elimu.model.v2.gson.content.AudioGson
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.io.FileNotFoundException
import java.io.FileOutputStream
import java.io.IOException
import java.util.concurrent.Executors

class AudiosFragment : Fragment() {
private var audiosViewModel: AudiosViewModel? = null

private var progressBar: ProgressBar? = null

private var textView: TextView? = null

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

audiosViewModel = ViewModelProvider(this).get(AudiosViewModel::class.java)
val root = inflater.inflate(R.layout.fragment_audios, container, false)
progressBar = root.findViewById(R.id.progress_bar_audios)
textView = root.findViewById(R.id.text_audios) as? TextView
audiosViewModel!!.text.observe(viewLifecycleOwner, object : Observer<String?> {
override fun onChanged(s: String?) {
Log.i(javaClass.name, "onChanged")
textView?.text = s
}
})
return root
}

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

// Download Audios from REST API, and store them in the database
val baseApplication = activity!!.application as BaseApplication
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

Replace non-null assertion with safe call or null check.

Using !! on activity can lead to NullPointerException if the fragment is not attached to an activity.

-val baseApplication = activity!!.application as BaseApplication
+activity?.let {
+    val baseApplication = it.application as BaseApplication
+    // Continue with the rest of the code
+} ?: run {
+    Log.w(javaClass.name, "Fragment not attached to an activity")
+    return
+}
📝 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
val baseApplication = activity!!.application as BaseApplication
activity?.let {
val baseApplication = it.application as BaseApplication
// Continue with the rest of the code
} ?: run {
Log.w(javaClass.name, "Fragment not attached to an activity")
return
}

val retrofit = baseApplication.retrofit
val audiosService = retrofit.create(AudiosService::class.java)
val audioGsonsCall = audiosService.listAudios()
Log.i(javaClass.name, "audioGsonsCall.request(): " + audioGsonsCall.request())
audioGsonsCall.enqueue(object : Callback<List<AudioGson>> {
override fun onResponse(
call: Call<List<AudioGson>>,
response: Response<List<AudioGson>>
) {
Log.i(javaClass.name, "onResponse")

Log.i(javaClass.name, "response: $response")
if (response.isSuccessful) {
val audioGsons = response.body()!!
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

Safely handle nullable response body.

Using !! on response.body() can lead to NullPointerException if the response body is null.

-val audioGsons = response.body()!!
+val audioGsons = response.body() ?: emptyList()
📝 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
val audioGsons = response.body()!!
val audioGsons = response.body() ?: emptyList()

Log.i(javaClass.name, "audioGsons.size(): " + audioGsons.size)

if (audioGsons.size > 0) {
processResponseBody(audioGsons)
}
} 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
Comment on lines +84 to +87
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

Avoid non-null assertion and use more idiomatic resource access.

Using !! on textView and direct color access is not the safest approach.

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

}
}

override fun onFailure(call: Call<List<AudioGson>>, t: Throwable) {
Log.e(javaClass.name, "onFailure", t)

Log.e(javaClass.name, "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 +97 to +100
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

Handle potential null cause in error message.

The cause of the throwable might be null, leading to a potential NullPointerException.

-Snackbar.make(textView!!, t.cause.toString(), Snackbar.LENGTH_LONG)
-    .setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
-    .show()
+textView?.let { view ->
+    val errorMessage = t.cause?.toString() ?: t.message ?: "Unknown error"
+    Snackbar.make(view, errorMessage, Snackbar.LENGTH_LONG)
+        .setBackgroundTint(requireContext().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
Snackbar.make(textView!!, t.cause.toString(), Snackbar.LENGTH_LONG)
.setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
.show()
progressBar!!.visibility = View.GONE
textView?.let { view ->
val errorMessage = t.cause?.toString() ?: t.message ?: "Unknown error"
Snackbar.make(view, errorMessage, Snackbar.LENGTH_LONG)
.setBackgroundTint(requireContext().getColor(R.color.deep_orange_darken_4))
.show()
}
progressBar!!.visibility = View.GONE

}
})
}

private fun processResponseBody(audioGsons: List<AudioGson>) {
Log.i(javaClass.name, "processResponseBody")

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

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

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

// TODO: also delete corresponding audio files (only those that are no longer used)
for (audioGson in audioGsons) {
Log.i(javaClass.name, "audioGson.getId(): " + audioGson.id)

val audio = GsonToRoomConverter.getAudio(audioGson)

// Check if the corresponding audio file has already been downloaded
val audioFile = FileHelper.getAudioFile(audioGson, context)
Log.i(javaClass.name, "audioFile: $audioFile")
Log.i(javaClass.name, "audioFile.exists(): " + audioFile.exists())
if (!audioFile.exists()) {
// Download file bytes
val baseApplication = activity!!.application as BaseApplication
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

Replace non-null assertion with safe call.

Using !! on activity can lead to NullPointerException if the fragment is not attached to an activity.

-val baseApplication = activity!!.application as BaseApplication
+val baseApplication = requireActivity().application as BaseApplication

Using requireActivity() will throw a more descriptive IllegalStateException if the fragment is not attached, which is easier to debug than a NullPointerException.

📝 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
val baseApplication = activity!!.application as BaseApplication
val baseApplication = requireActivity().application as BaseApplication

val downloadUrl = baseApplication.baseUrl + audioGson.bytesUrl
Log.i(javaClass.name, "downloadUrl: $downloadUrl")
val bytes = MultimediaDownloader.downloadFileBytes(downloadUrl)
Log.i(javaClass.name, "bytes.length: " + bytes.size)

// Store the downloaded file in the external storage directory
try {
val fileOutputStream = FileOutputStream(audioFile)
fileOutputStream.write(bytes)
} catch (e: FileNotFoundException) {
Log.e(javaClass.name, null, e)
} catch (e: IOException) {
Log.e(javaClass.name, null, e)
}
Log.i(javaClass.name, "audioFile.exists(): " + audioFile.exists())
Comment on lines +138 to +146
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

Use Kotlin's use extension function to properly close resources.

The current implementation doesn't properly close the FileOutputStream, which could lead to resource leaks.

-try {
-    val fileOutputStream = FileOutputStream(audioFile)
-    fileOutputStream.write(bytes)
-} catch (e: FileNotFoundException) {
-    Log.e(javaClass.name, null, e)
-} catch (e: IOException) {
-    Log.e(javaClass.name, null, e)
-}
+try {
+    FileOutputStream(audioFile).use { fileOutputStream ->
+        fileOutputStream.write(bytes)
+    }
+} catch (e: IOException) {
+    Log.e(javaClass.name, "Error writing to file", e)
+}

The use function automatically closes the resource after the block is executed, even if an exception occurs.

📝 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
try {
val fileOutputStream = FileOutputStream(audioFile)
fileOutputStream.write(bytes)
} catch (e: FileNotFoundException) {
Log.e(javaClass.name, null, e)
} catch (e: IOException) {
Log.e(javaClass.name, null, e)
}
Log.i(javaClass.name, "audioFile.exists(): " + audioFile.exists())
try {
FileOutputStream(audioFile).use { fileOutputStream ->
fileOutputStream.write(bytes)
}
} catch (e: IOException) {
Log.e(javaClass.name, "Error writing to file", e)
}
Log.i(javaClass.name, "audioFile.exists(): " + audioFile.exists())

}

// Store the Audio in the database
audioDao.insert(audio)
Log.i(javaClass.name, "Stored Audio in database with ID " + audio.id)
}

// Update the UI
val audios = audioDao.loadAll()
Log.i(javaClass.name, "audios.size(): " + audios.size)
activity!!.runOnUiThread {
textView!!.text = "audios.size(): " + audios.size
Snackbar.make(textView!!, "audios.size(): " + audios.size, Snackbar.LENGTH_LONG)
.show()
progressBar!!.visibility = View.GONE
}
Comment on lines +157 to +162
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

Use proper fragment lifecycle-aware UI updates.

Using activity!!.runOnUiThread with non-null assertion is not the safest approach. Consider using viewLifecycleOwner to ensure UI updates only happen when the fragment's view is active.

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

Or better, use a more modern approach with coroutines:

lifecycleScope.launch(Dispatchers.Main) {
    if (isAdded) {
        textView?.text = "audios.size(): ${audios.size}"
        textView?.let { view ->
            Snackbar.make(view, "audios.size(): ${audios.size}", Snackbar.LENGTH_LONG)
                .show()
        }
        progressBar?.visibility = View.GONE
    }
}

}
})
}
}