Offline capabilities by bryanporcello · Pull Request #125 · SDP-BeGreen/BeGreen

bryanporcello · 2023-05-24T17:08:07Z

caching the Firebase Database reference which is in the BeGreenApp.kt class, by using Firebase persistence and Keep Sync.
caching the SendPost method, it is caching the Bitmap and metadata of posts if there is no internet connection using the TinyDB.kt class.
checking if the internet is reconnected, if Yes, it will post the cached post data to Firebase.

- caching the Firebase Database reference which is in the BeGreenApp.kt class, by using Firebase persistence and Keep Sync. - caching the SendPost method, it is caching the Bitmap and metadata of posts if there is no internet connection. - checking if the internet is reconnected, if Yes, it will post the cached post data to Firebase.

david-schroeter

Thanks for this first implementation of the offline capabilities to store the picture when we don't have connection to post it later.

However, I had quite a few comments regarding things in your code.

I think like this, it will be hard to reach the 80% of coverage in tests, I would suggest you to refactor the code that you wrote in the BeGreenApp to be able to more easily test it.

Let us know on the group chat if you have any questions while refactoring your code and adding more tests.

david-schroeter · 2023-05-25T11:59:40Z

+     * @param mClass a nullable class object
+     * @return ArrayList of String
+     */
+    fun getListObject(key: String?, mClass: Class<*>?): ArrayList<Any> {


Here I don't think it's a good idea to accept a every possible class and then return Any, the cast on the other side would be unsafe, and could potentially result in crash of the application. I would enforce the type using a generic param.

The signature of the function could be something like:

Suggested change

fun getListObject(key: String?, mClass: Class<*>?): ArrayList<Any> {

fun <K> getListObject(key: String?, mClass: Class<K>?): List<K> {

And I am not sure to understand why the mClass param is nullable ? Is there a particular case when we would want this parameter to be null ?

I would as well return a List<String. I think it is safer to return immutable collection, it may help to prevent weird behavior to work with immutable data if possible.

Indeed, there is no need to make mClass nullable. While your proposed return type of List is immutable and might prevent weird behavior, due to time restriction and the good behavior of the current return type, I will leave it as it is. Otherwise, changes made as proposed, thanks a lot!

david-schroeter · 2023-05-25T12:24:40Z

+        val objects = ArrayList<Any>()
+        for (jObjString in objStrings) {
+            val value = gson.fromJson(jObjString, mClass)
+            objects.add(value)
+        }
+        return objects


If you return a List<String, you can write this code in a more functional way, and do it in one line like this:

Suggested change

val objects = ArrayList<Any>()

for (jObjString in objStrings) {

val value = gson.fromJson(jObjString, mClass)

objects.add(value)

}

return objects

return objStrings.map { gson.fromJson(it, mClass) }

As I still return List, this change could not be applied but thanks very much for the suggestion.

david-schroeter · 2023-05-25T12:55:30Z

I am not sure this is the correct place to setup the firebase connection listener. I think it won't be possible to test this code at all as we have a different application class to run the tests, because we have to start another instance of Koin, in the tests. Therefore this file is not testable.

I would factor out this code in another file, and run it in the MainActivity for example, as this activity run the whole time. It would probably be the best place to do it.

What do you think of it ?

I agree! Unfortunately, I will not have time to implement it on time

david-schroeter · 2023-05-25T12:57:17Z

+                    if (bitmaps.isNotEmpty()) {
+                        val b: ByteArray = Base64.decode(bitmaps[0], Base64.DEFAULT)
+                        val bitmap = BitmapFactory.decodeByteArray(b, 0, b.size)
+
+                        // Launch a coroutine to update user with new metadata
+                        applicationScope.launch { updateUser(metas[0] as TrashPhotoMetadata?
+                            , users[0] as User, bitmap
+                        ) }
+
+                        Toast.makeText(this@BeGreenApp, "Resending Pending Posts No."+metas.size, Toast.LENGTH_LONG).show()


Here it looks like you are only posting the first picture that the user took while being offline. What would happen if a User take more than one picture ?

All the cast can be removed as well if you make the method generic in the TinyDB file, as suggested in another comment.

Tried many different things to debug this with no success.

david-schroeter · 2023-05-25T13:06:51Z

+
+                            // Retrieve existing metadata and add current one.
+                            val metas = tinyDB.getListObject("metas", TrashPhotoMetadata::class.java)
+                            metas.add(metadata!!)


Here, if you make getListObject return a List as suggested, the add method won't exist anymore, as the List is immutable. Instead, I would suggest you to do something like this: val newMetas = metas + metadata!!

Change made as proposed, thanks!

david-schroeter · 2023-05-25T13:14:37Z

+                                val b: ByteArray = baos.toByteArray()
+                                val encoded: String = Base64.encodeToString(b, Base64.DEFAULT)
+                                val bitmaps = tinyDB.getListString("bitmaps")
+                                bitmaps.add(encoded)
+                                tinyDB.putListString("bitmaps", bitmaps)


Here, instead of encoding the whole image to String, to retrieve it later, I would suggest to have a closer look at how Xavier passed the image from the CameraWithUIFragment to the SendPostFragment. He is already storing the taken picture on the phone, and he is using its uri to get it. He is doing that in the method InitView.

And then I would suggest you to only store the URI, and then you will be able to retrieve the picture later on when the connection is back.

iliasmerigh

Thank you Bryan for this PR. It's very good to have an offline mode so the user can finally share a post without having a wifi connection. The Toast that indicates that my post are pending is an excellent feature. However, it seems that when I take more than one photo in offline mode, only (the first) one is shared. Moreover, the picture remains blank in the feed or profile fragment.

When I looked at the database to try to understand why, it seems that indeed only one post is shared (the first one), and it's shared without the pictureId field. But the photo is correctly stored in Firebase Storage. Therefore there are two things to solve :

Share all photos that have been taken and not only the first one
Don't loose the pictureId in the trashPhotoMetadata

Screen of the blank photo :

Screen of the problem in database :

Screen of photo in storage :

bryanporcello · 2023-05-26T00:07:48Z

Thanks, David and Ilias for your review. I tried to implement it the most I could but unfortunately, without success. and I cannot invest a lot of time in it before tomorrow. This PR would most likely not be merged on time, sorry for this guys! I'll inform the TA accordingly to take full responsibility for this failure.

david-schroeter · 2023-05-26T08:13:07Z

+import java.util.*
+
+// This class is a utility to simplify the use of SharedPreferences for Android.
+class TinyDB(appContext: Context) {


I think you should cite your source when you copy past a file...

https://github.com/kcochibili/TinyDB--Android-Shared-Preferences-Turbo/blob/master/TinyDB.java#L287

Indeed, you are right! I should cite the most possible when I take existing code even if I made changes on it. However, as the code is not functioning and not yet well written in terms of methods declaration and comments, I did not consider it as urgent as of now. But I agree I should take the habit to do it at the very beginning of coding to follow good standard practice and save time later on. Thanks for pointing that out!

bryanporcello added 2 commits May 24, 2023 19:05

Added test cases

1e33f0c

david-schroeter requested changes May 25, 2023

View reviewed changes

david-schroeter reviewed May 25, 2023

View reviewed changes

Comment thread app/src/main/java/com/github/sdp_begreen/begreen/fragments/SendPostFragment.kt Outdated

iliasmerigh reviewed May 25, 2023

View reviewed changes

david-schroeter reviewed May 25, 2023

View reviewed changes

Comment thread app/src/main/java/com/github/sdp_begreen/begreen/BeGreenApp.kt Outdated

bryanporcello added 2 commits May 26, 2023 00:08

Added proposed changes

0b0c311

Minor modification in method signature

e93755d

david-schroeter reviewed May 26, 2023

View reviewed changes

bryanporcello marked this pull request as draft May 26, 2023 08:13

	fun getListObject(key: String?, mClass: Class<*>?): ArrayList<Any> {
	fun <K> getListObject(key: String?, mClass: Class<K>?): List<K> {

Conversation

bryanporcello commented May 24, 2023

Uh oh!

david-schroeter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iliasmerigh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bryanporcello commented May 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iliasmerigh left a comment •

edited

Loading