-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Replace non-null assertion with safer alternative. Using Use a safer alternative: -val baseApplication = activity!!.application as BaseApplication
+val baseApplication = requireActivity().application as BaseApplication 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
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()!! | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 - val imageGsons = response.body()!!
+ val imageGsons = response.body() ?: emptyList() 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
Suggested change
|
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai Create a separate issue for this and assign to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainI'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:
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? |
||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use requireContext() instead of context to avoid potential NPE. The - val roomDb = RoomDb.getDatabase(context)
+ val roomDb = RoomDb.getDatabase(requireContext()) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Replace non-null assertion with requireActivity(). Using -val baseApplication = activity!!.application as BaseApplication
+val baseApplication = requireActivity().application as BaseApplication 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai Create a separate issue for this and assign to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 🐰 scribbling notes in a tiny notebook 🏁 Script executed:
Length of output: 1164 @tuancoltech I've created a new issue for replacing The issue includes details about why this change would be beneficial:
🐰 happily bounces and gives a thumbs up If you found this review helpful, would you consider giving us a shout-out on X? |
||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use Kotlin's The current implementation doesn't explicitly close the - 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
Suggested change
|
||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Replace activity!! with requireActivity() and simplify UI updates. Using -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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai Create a separate issue for this and assign to me. |
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 inonCreateView()
. This leads to unnecessary null checks and non-null assertions throughout the code.I suggest using
lateinit
for these properties:📝 Committable suggestion
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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!