Contacts implementation #107

tomas-kovacs · 2021-11-05T18:34:41Z

One Line Summary

Get and display the user's contacts

Issue(s) Addressed

Steps to Reproduce/Test

From the First Screen, tap on the three buttons menu and select Contacts to see the list of contacts (read contacts permission will be requested).
Tap on a contact from the list and a bottom sheet dialog will be displayed with the contact's name and phone number.

Checklist

Functionally tested
Unit tests created / updated
Backend dependencies deployed (if needed)

julianfalcionelli · 2021-11-08T09:51:23Z

app/src/main/java/com/rocketinsights/android/models/Contact.kt

+
+import android.os.Parcel
+import android.os.Parcelable
+


Are u implementing Parcebla by hand instead of using just the @Serializable annotation for it for any reason?

Parcelable is better than Serializable because it creates less temporary objects. I removed the unnecessary code with the @Parcelize annotation.

julianfalcionelli · 2021-11-08T09:52:44Z

app/src/main/java/com/rocketinsights/android/ui/ContactsFragment.kt

+import com.rocketinsights.android.viewmodels.PermissionsViewModel
+import org.koin.androidx.viewmodel.ext.android.viewModel
+
+class ContactsFragment : Fragment(R.layout.fragment_contacts), LoaderManager.LoaderCallbacks<Cursor> {


Please use the BaseFragment and override the life-cycle methods as needed.

julianfalcionelli · 2021-11-08T09:56:49Z

app/src/main/java/com/rocketinsights/android/ui/base/BaseBottomSheetFragment.kt

+    }
+
+    override fun onDestroyView() {
+        super.onDestroyView()


It is recommended that any triggered action in a life-cycle event that corresponds to the destruction of the component (like in this case "onDestroyView") be put before the super, to ensure that it is called even when the component is still "alive". So in this case please assign the _biding to null before calling the super.onDestroyView.

You are totally right. I fixed it but I'm going to remove this base class and start using the viewBinding extension functions as Ivan suggested.

IvanToplak

I've reviewed it and tested it. I've left some suggestions for improvements.
It runs on Samsung device (Android 11), but not on Xiaomi (Android 10).
On Xiaomi I get this exception:

E/AndroidRuntime: FATAL EXCEPTION: ModernAsyncTask #1
    Process: com.rocketinsights.android.dev, PID: 12910
    java.lang.RuntimeException: An error occurred while executing doInBackground()
        at androidx.loader.content.ModernAsyncTask$3.done(ModernAsyncTask.java:164)
        at java.util.concurrent.FutureTask.finishCompletion(FutureTask.java:383)
        at java.util.concurrent.FutureTask.setException(FutureTask.java:252)
        at java.util.concurrent.FutureTask.run(FutureTask.java:271)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:919)
     Caused by: android.database.sqlite.SQLiteException: no such column: true (code 1 SQLITE_ERROR): , while compiling: SELECT _id, display_name, data1 FROM view_data data LEFT OUTER JOIN (SELECT 0 as STAT_DATA_ID,0 as x_times_used, 0 as x_last_time_used,0 as times_used, 0 as last_time_used where 0) as data_usage_stat ON (STAT_DATA_ID=data._id) WHERE (1 AND mimetype_id=1 AND (1=1)) AND (((display_name NOTNULL) AND (display_name != '') AND (has_phone_number = true) AND (mimetype = 'vnd.android.cursor.item/phone_v2') AND (raw_contact_id = name_raw_contact_id))) ORDER BY display_name ASC
        at android.database.DatabaseUtils.readExceptionFromParcel(DatabaseUtils.java:184)
        at android.database.DatabaseUtils.readExceptionFromParcel(DatabaseUtils.java:140)
        at android.content.ContentProviderProxy.query(ContentProviderNative.java:423)
        at android.content.ContentResolver.query(ContentResolver.java:946)
        at android.content.ContentResolver.query(ContentResolver.java:881)
        at androidx.core.content.ContentResolverCompat.query(ContentResolverCompat.java:81)
        at androidx.loader.content.CursorLoader.loadInBackground(CursorLoader.java:63)
        at androidx.loader.content.CursorLoader.loadInBackground(CursorLoader.java:41)
        at androidx.loader.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:307)
        at androidx.loader.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:60)
        at androidx.loader.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:48)
        at androidx.loader.content.ModernAsyncTask$2.call(ModernAsyncTask.java:141)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) 
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) 
        at java.lang.Thread.run(Thread.java:919) 
I/Process: Sending signal. PID: 12910 SIG: 9

I've also noticed that app doesn't ask for permission if it was denied before.

IvanToplak · 2021-11-08T08:45:10Z

app/src/main/java/com/rocketinsights/android/models/Contact.kt

+import android.os.Parcel
+import android.os.Parcelable
+
+data class Contact(


I would suggest usage of @Parcelize annotation to reduce the boilerplate. More about it here.

IvanToplak · 2021-11-08T09:08:53Z

app/src/main/java/com/rocketinsights/android/ui/base/BaseBottomSheetFragment.kt

+import androidx.viewbinding.ViewBinding
+import com.google.android.material.bottomsheet.BottomSheetDialogFragment
+
+abstract class BaseBottomSheetFragment<B : ViewBinding> : BottomSheetDialogFragment() {


We have ViewBindingExt.kt that can be used on Fragments to reduce the boilerplate. Could you try to use it instead of this base class?

IvanToplak · 2021-11-08T09:20:03Z

app/src/main/java/com/rocketinsights/android/viewmodels/ContactsViewModel.kt

+    private val _contactsLiveData: MutableLiveData<List<Contact>> by lazy {
+        MutableLiveData<List<Contact>>()
+    }
+    val contactsLiveData: LiveData<List<Contact>> = _contactsLiveData


I would suggest implementing get() for this backing property to avoid generation of the backing field.

IvanToplak · 2021-11-08T09:28:32Z

app/src/main/res/layout/fragment_contacts.xml

+            android:layout_height="match_parent"
+            android:layout_margin="@dimen/margin" />
+
+        <androidx.core.widget.ContentLoadingProgressBar


There is also layout_progress.xml that can be used here so that we don't repeat ContentLoadingProgressBar on every screen that requires it.

IvanToplak · 2021-11-08T09:30:42Z

app/src/main/res/navigation/main_graph.xml

+    <fragment
+        android:id="@+id/contacts_fragment"
+        android:name="com.rocketinsights.android.ui.ContactsFragment"
+        android:label="@string/contacts"


For consistency it would be nice to put contacts_screen_title here.

IvanToplak · 2021-11-08T09:45:22Z

app/src/main/java/com/rocketinsights/android/ui/ContactsFragment.kt

+import com.rocketinsights.android.viewmodels.PermissionsViewModel
+import org.koin.androidx.viewmodel.ext.android.viewModel
+
+class ContactsFragment : Fragment(R.layout.fragment_contacts), LoaderManager.LoaderCallbacks<Cursor> {


Could we use BaseFragment here?

IvanToplak · 2021-11-08T09:48:51Z

app/src/main/res/layout/fragment_contacts.xml

+    android:layout_height="match_parent"
+    android:background="?android:attr/colorBackground"
+    android:fitsSystemWindows="true"
+    tools:context=".ui.MessagesFragment">


Copy-paste leftover :)

Nice catch ;)

IvanToplak · 2021-11-08T10:02:17Z

app/src/main/res/layout/fragment_contacts.xml

+            android:id="@+id/recycler_view_contacts"
+            android:layout_width="match_parent"
+            android:layout_height="match_parent"
+            android:layout_margin="@dimen/margin" />


I think it would look better without this margin. Maybe use some horizontal padding with android:clipToPadding="false"

julianfalcionelli · 2021-11-08T12:57:26Z

app/src/main/java/com/rocketinsights/android/viewmodels/ContactsViewModel.kt

+
+    fun getContactsQueryId(): Int = CONTACTS_QUERY_ID
+
+    companion object {


I am not sure, but I think that we were defining the companion object at the top of the class instead of at the bottom.

Done, moved up

IvanToplak · 2021-11-09T09:31:09Z

app/src/main/java/com/rocketinsights/android/ui/ContactsFragment.kt


    private val binding by viewBinding(FragmentContactsBinding::bind)
-    private val viewModel by viewModel<ContactsViewModel>()


Have you tried updating Koin to 3.1.3 and using this new feature?

Great new feature. I updated Koin version to 3.1.3 to support this

IvanToplak

Nicely done! :)

Contacts implementation

128d0ab

tomas-kovacs requested a review from julianfalcionelli November 5, 2021 18:34

julianfalcionelli reviewed Nov 8, 2021

View reviewed changes

IvanToplak reviewed Nov 8, 2021

View reviewed changes

tomas-kovacs added 3 commits November 8, 2021 09:22

Resolved comments

cb04b96

Lint fix

3b4e148

Merge branch 'master' into feature/contacts

e90002e

julianfalcionelli reviewed Nov 8, 2021

View reviewed changes

tomas-kovacs added 4 commits November 8, 2021 11:21

BaseFragment comment fix

9a3f1ec

Moved companion objects up

5fb1f22

Contacts selection query fix

7fb087e

Contacts save state

47736b3

IvanToplak reviewed Nov 9, 2021

View reviewed changes

Updated Koin version and ContactsViewModel injection

82aad87

IvanToplak approved these changes Nov 9, 2021

View reviewed changes

tomas-kovacs merged commit c0b39d7 into master Nov 9, 2021

tomas-kovacs deleted the feature/contacts branch November 9, 2021 18:08


		fun getContactsQueryId(): Int = CONTACTS_QUERY_ID

		companion object {


		private val binding by viewBinding(FragmentContactsBinding::bind)
		private val viewModel by viewModel<ContactsViewModel>()


		import android.os.Parcel
		import android.os.Parcelable

Contacts implementation #107

Contacts implementation #107

Uh oh!

Conversation

tomas-kovacs commented Nov 5, 2021 • edited by fhmajeed Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

One Line Summary

Issue(s) Addressed

Steps to Reproduce/Test

Checklist

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!

IvanToplak left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanToplak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomas-kovacs commented Nov 5, 2021 •

edited by fhmajeed

Loading