Skip to content

Convert ImagesFragment to Kotlin #165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 5, 2025
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,189 @@
package ai.elimu.content_provider.ui.image

import ai.elimu.content_provider.BaseApplication
import ai.elimu.content_provider.R
import ai.elimu.content_provider.rest.ImagesService
import ai.elimu.content_provider.room.GsonToRoomConverter
import ai.elimu.content_provider.room.db.RoomDb
import ai.elimu.content_provider.room.entity.Image_Word
import ai.elimu.content_provider.util.FileHelper
import ai.elimu.content_provider.util.MultimediaDownloader
import ai.elimu.model.v2.gson.content.ImageGson
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 ImagesFragment : Fragment() {
private var imagesViewModel: ImagesViewModel? = null

private var progressBar: ProgressBar? = null

private var textView: TextView? = null

Comment on lines +31 to +37
Copy link

@coderabbitai coderabbitai bot Mar 5, 2025

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 lateinit for non-null properties that are initialized later.

The current implementation uses nullable types with ? for class properties that will always be initialized in onCreateView(). This leads to unnecessary null checks and non-null assertions throughout the code.

I suggest using lateinit for these properties:

-private var imagesViewModel: ImagesViewModel? = null
-private var progressBar: ProgressBar? = null
-private var textView: TextView? = null
+private lateinit var imagesViewModel: ImagesViewModel
+private lateinit var progressBar: ProgressBar
+private lateinit var textView: TextView
📝 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
class ImagesFragment : Fragment() {
private var imagesViewModel: ImagesViewModel? = null
private var progressBar: ProgressBar? = null
private var textView: TextView? = null
class ImagesFragment : Fragment() {
private lateinit var imagesViewModel: ImagesViewModel
private lateinit var progressBar: ProgressBar
private lateinit var textView: TextView
// ... rest of the class implementation
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@coderabbitai Create a separate issue for this and assign to me.

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

@tuancoltech I've created a new issue titled "Refactor: Use lateinit for non-null properties in ImagesFragment.kt" and assigned it to you. The issue contains details about the refactoring suggestion to replace nullable property types with lateinit in ImagesFragment.kt. You can track this improvement separately from this PR.


🏁 Script executed:

#!/bin/bash
# Create a GitHub issue for the lateinit refactoring suggestion and assign it to tuancoltech

gh issue create \
  --title "Refactor: Use lateinit for non-null properties in ImagesFragment.kt" \
  --body "## Description

Refactoring suggestion to replace nullable property types with \`lateinit\` in \`ImagesFragment.kt\`.

The current implementation uses nullable types with \`?\` for class properties that will always be initialized in \`onCreateView()\`. This leads to unnecessary null checks and non-null assertions throughout the code.

### Proposed changes:

\`\`\`diff
-private var imagesViewModel: ImagesViewModel? = null
-private var progressBar: ProgressBar? = null
-private var textView: TextView? = null
+private lateinit var imagesViewModel: ImagesViewModel
+private lateinit var progressBar: ProgressBar
+private lateinit var textView: TextView
\`\`\`

### Related Pull Request
https://github.com/elimu-ai/content-provider/pull/165

### Related Comment
https://github.com/elimu-ai/content-provider/pull/165#discussion_r1980826051
" \
  --assignee "tuancoltech"

Length of output: 1009


@tuancoltech Issue #166 has been successfully created at #166 and assigned to you. The issue contains all the details about the refactoring suggestion to use Kotlin's lateinit for non-null properties in ImagesFragment.kt as requested.


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

private val TAG = ImagesFragment::class.java.name

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

imagesViewModel = ViewModelProvider(this).get(ImagesViewModel::class.java)
val root = inflater.inflate(R.layout.fragment_images, container, false)
progressBar = root.findViewById(R.id.progress_bar_images)
textView = root.findViewById(R.id.text_images) as? TextView
imagesViewModel!!.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 Images from REST API, and store them in the database
val baseApplication = activity!!.application as BaseApplication
val retrofit = baseApplication.retrofit
Comment on lines +65 to +66
Copy link

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 safer alternative.

Using activity!! can lead to a NullPointerException if the fragment is no longer attached to an activity.

Use a safer alternative:

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

val imagesService = retrofit.create(ImagesService::class.java)
val imageGsonsCall = imagesService.listImages()
Log.i(TAG, "imageGsonsCall.request(): " + imageGsonsCall.request())
imageGsonsCall.enqueue(object : Callback<List<ImageGson>> {
override fun onResponse(
call: Call<List<ImageGson>>,
response: Response<List<ImageGson>>
) {
Log.i(TAG, "onResponse")

Log.i(TAG, "response: $response")
if (response.isSuccessful) {
val imageGsons = response.body()!!
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle potential null response body safely.

Direct null assertion on the response body could cause a NullPointerException if the body is null.

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

Log.i(TAG, "imageGsons.size(): " + imageGsons.size)

if (imageGsons.size > 0) {
processResponseBody(imageGsons)
}
} 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 +87 to +90
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use ContextCompat for compatibility and remove non-null assertions.

The current code uses a deprecated method resources.getColor() and has unnecessary non-null assertions.

Use ContextCompat for backward compatibility:

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

}
}

override fun onFailure(call: Call<List<ImageGson>>, 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 +100 to +104
Copy link

@coderabbitai coderabbitai bot Mar 5, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle null cause gracefully and use ContextCompat.

The current implementation might throw an exception if t.cause is null. Also, it uses a deprecated color method.

Improve error handling and use ContextCompat:

-Snackbar.make(textView!!, t.cause.toString(), Snackbar.LENGTH_LONG)
-    .setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
-    .show()
-progressBar!!.visibility = View.GONE
+Snackbar.make(textView, t.cause?.toString() ?: t.toString(), Snackbar.LENGTH_LONG)
+    .setBackgroundTint(ContextCompat.getColor(requireContext(), R.color.deep_orange_darken_4))
+    .show()
+progressBar.visibility = View.GONE
📝 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
}
Snackbar.make(textView, t.cause?.toString() ?: t.toString(), Snackbar.LENGTH_LONG)
.setBackgroundTint(ContextCompat.getColor(requireContext(), R.color.deep_orange_darken_4))
.show()
progressBar.visibility = View.GONE
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@coderabbitai Create a separate issue for this and assign to me.

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

I'll create a separate issue for the refactoring suggestions and assign it to you.

@tuancoltech I've created a separate issue for the refactoring suggestions and assigned it to you. The issue includes the details about handling null cause gracefully, using ContextCompat for color resources, and removing unnecessary not-null assertions. This way, these improvements can be addressed separately from the current PR that's focused on converting ImagesFragment to Kotlin.


🏁 Script executed:

#!/bin/bash
# Create a GitHub issue for the refactoring suggestions

gh issue create \
  --title "Refactor ImagesFragment error handling to handle null cause and use ContextCompat" \
  --body "## Description

This issue is created as a follow-up to a code review comment in [PR #165](https://github.com/elimu-ai/content-provider/pull/165).

## Refactoring needed:

1. Handle null cause gracefully in the error handling section of ImagesFragment.kt
2. Replace deprecated resources.getColor() with ContextCompat.getColor()
3. Remove unnecessary not-null assertions (!!)

### Specific changes:

\`\`\`diff
-Snackbar.make(textView!!, t.cause.toString(), Snackbar.LENGTH_LONG)
-    .setBackgroundTint(resources.getColor(R.color.deep_orange_darken_4))
-    .show()
-progressBar!!.visibility = View.GONE
+Snackbar.make(textView, t.cause?.toString() ?: t.toString(), Snackbar.LENGTH_LONG)
+    .setBackgroundTint(ContextCompat.getColor(requireContext(), R.color.deep_orange_darken_4))
+    .show()
+progressBar.visibility = View.GONE
\`\`\`

## References
- PR: https://github.com/elimu-ai/content-provider/pull/165
- Comment: https://github.com/elimu-ai/content-provider/pull/165#discussion_r1980826066
" \
  --assignee "tuancoltech"

Length of output: 1195


@tuancoltech The issue has been successfully created at #168 and assigned to you. You can now track these refactoring improvements separately while focusing on the main objective of your current PR (converting ImagesFragment to Kotlin).


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

})
}

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

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

val roomDb = RoomDb.getDatabase(context)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use requireContext() instead of context to avoid potential NPE.

The context property could be null if the fragment is not attached to an activity. Using requireContext() is safer.

-                val roomDb = RoomDb.getDatabase(context)
+                val roomDb = RoomDb.getDatabase(requireContext())
📝 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 roomDb = RoomDb.getDatabase(context)
val roomDb = RoomDb.getDatabase(requireContext())

val imageDao = roomDb.imageDao()
val image_WordDao = roomDb.image_WordDao()

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

// TODO: also delete corresponding image files (only those that are no longer used)
for (imageGson in imageGsons) {
Log.i(TAG, "imageGson.getId(): " + imageGson.id)

val image = GsonToRoomConverter.getImage(imageGson)

// Check if the corresponding image file has already been downloaded
val imageFile = FileHelper.getImageFile(imageGson, context)
Log.i(TAG, "imageFile: $imageFile")
Log.i(TAG, "imageFile.exists(): " + imageFile.exists())
if (!imageFile.exists()) {
// Download file bytes
val baseApplication = activity!!.application as BaseApplication
Copy link

@coderabbitai coderabbitai bot Mar 5, 2025

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 requireActivity().

Using activity!! can lead to a NullPointerException if the fragment is no longer attached to an activity.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@coderabbitai Create a separate issue for this and assign to me.

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

@tuancoltech I've created a new issue for refactoring the use of activity!! to requireActivity() in ImagesFragment.kt and assigned it to you. The issue includes details about the proposed change and the rationale behind it. This approach is safer and follows Android's recommended practices for fragment-activity interactions.

🐰 scribbling notes in a tiny notebook


🏁 Script executed:

#!/bin/bash
# Create a GitHub issue for the refactoring suggestion
gh issue create \
  --title "Refactor: Replace activity!! with requireActivity() in ImagesFragment.kt" \
  --body "## Description

During code review of PR #165 (Convert ImagesFragment to Kotlin), it was identified that the code uses non-null assertion (\`activity!!\`) which can lead to NullPointerException if the fragment is no longer attached to an activity.

## Proposed Solution
Replace instances of \`activity!!\` with \`requireActivity()\` in ImagesFragment.kt:

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

## References
- PR: https://github.com/elimu-ai/content-provider/pull/165
- Comment: https://github.com/elimu-ai/content-provider/pull/165#discussion_r1980826046

## Reasoning
Using \`requireActivity()\` is safer and more idiomatic in Kotlin Android development as it:
- Provides a clear error message if the fragment is not attached
- Follows Android's recommended practices for fragment-activity interactions
- Makes the code more robust against lifecycle-related crashes
" \
  --assignee "tuancoltech"

Length of output: 1164


@tuancoltech I've created a new issue for replacing activity!! with requireActivity() in ImagesFragment.kt and assigned it to you as requested. You can find the issue at: #167

The issue includes details about why this change would be beneficial:

  • It provides a clear error message if the fragment is not attached
  • It follows Android's recommended practices for fragment-activity interactions
  • It makes the code more robust against lifecycle-related crashes

🐰 happily bounces and gives a thumbs up


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

val downloadUrl = if (imageGson.bytesUrl.startsWith("http"))
imageGson.bytesUrl
else
baseApplication.baseUrl + imageGson.bytesUrl
Log.i(TAG, "downloadUrl: $downloadUrl")
val bytes = MultimediaDownloader.downloadFileBytes(downloadUrl)
Log.i(TAG, "bytes.length: " + bytes.size)

// Store the downloaded file in the external storage directory
try {
val fileOutputStream = FileOutputStream(imageFile)
fileOutputStream.write(bytes)
} catch (e: FileNotFoundException) {
Log.e(TAG, null, e)
} catch (e: IOException) {
Log.e(TAG, null, e)
}
Comment on lines +146 to +153
Copy link

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 function for automatically closing resources.

The current implementation doesn't explicitly close the FileOutputStream, which could lead to resource leaks. Kotlin's use function handles this automatically.

-                        try {
-                            val fileOutputStream = FileOutputStream(imageFile)
-                            fileOutputStream.write(bytes)
-                        } catch (e: FileNotFoundException) {
-                            Log.e(TAG, null, e)
-                        } catch (e: IOException) {
-                            Log.e(TAG, null, e)
-                        }
+                        try {
+                            FileOutputStream(imageFile).use { it.write(bytes) }
+                        } catch (e: IOException) {
+                            Log.e(TAG, "Error writing image file", e)
+                        }
📝 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(imageFile)
fileOutputStream.write(bytes)
} catch (e: FileNotFoundException) {
Log.e(TAG, null, e)
} catch (e: IOException) {
Log.e(TAG, null, e)
}
try {
FileOutputStream(imageFile).use { it.write(bytes) }
} catch (e: IOException) {
Log.e(TAG, "Error writing image file", e)
}

Log.i(TAG, "imageFile.exists(): " + imageFile.exists())
}

// Store the Image in the database
imageDao.insert(image)
Log.i(TAG, "Stored Image in database with ID " + image.id)

// Store all the Image's Word labels in the database
val wordGsons = imageGson.words
Log.i(TAG, "wordGsons.size(): " + wordGsons.size)
for (wordGson in wordGsons) {
Log.i(TAG, "wordGson.getId(): " + wordGson.id)
val image_Word = Image_Word()
image_Word.image_id = imageGson.id
image_Word.words_id = wordGson.id
image_WordDao.insert(image_Word)
Log.i(
TAG,
"Stored Image_Word in database. Image_id: " + image_Word.image_id + ", words_id: " + image_Word.words_id
)
}
}

// Update the UI
val images = imageDao.loadAll()
Log.i(TAG, "images.size(): " + images.size)
activity!!.runOnUiThread {
textView!!.text = "images.size(): " + images.size
Snackbar.make(textView!!, "images.size(): " + images.size, Snackbar.LENGTH_LONG)
.show()
progressBar!!.visibility = View.GONE
}
Comment on lines +180 to +185
Copy link

@coderabbitai coderabbitai bot Mar 5, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace activity!! with requireActivity() and simplify UI updates.

Using activity!! can lead to a NullPointerException. Also, there's repetition in setting text and showing a Snackbar with the same message.

-activity!!.runOnUiThread {
-    textView!!.text = "images.size(): " + images.size
-    Snackbar.make(textView!!, "images.size(): " + images.size, Snackbar.LENGTH_LONG)
-        .show()
-    progressBar!!.visibility = View.GONE
+requireActivity().runOnUiThread {
+    val message = "images.size(): ${images.size}"
+    textView.text = message
+    Snackbar.make(textView, message, Snackbar.LENGTH_LONG).show()
+    progressBar.visibility = View.GONE
}
📝 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
activity!!.runOnUiThread {
textView!!.text = "images.size(): " + images.size
Snackbar.make(textView!!, "images.size(): " + images.size, Snackbar.LENGTH_LONG)
.show()
progressBar!!.visibility = View.GONE
}
requireActivity().runOnUiThread {
val message = "images.size(): ${images.size}"
textView.text = message
Snackbar.make(textView, message, Snackbar.LENGTH_LONG).show()
progressBar.visibility = View.GONE
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@coderabbitai Create a separate issue for this and assign to me.

}
})
}
}