hey-deepak/mychat

mychat's Issues

Wierd outline on OutlinedTextField

Square Crop functionality not available on custom profile picture

Login errors not displayed

MyChat/app/src/main/java/com/dc/mychat/MainActivity.kt

Lines 79 to 82 in e55ca28

	if (result.resultCode == Activity.RESULT_OK) {
	Log.d("TAG","Inside ResultLambda ")
	loginHandler()
	}

For login, you have handled only success case & left the failure case unhandled (else block). That's very optimistic approach. Anything may go wrong while Login. such errors must be reported to the user.

Sending new message while offline : Message shown in UI

Expected : Message sent while offline should not be shown in UI because it will fail.

Actual : It is shown in the UI :

WhatsApp.Video.2022-07-19.at.12.36.41.PM.mp4

Weird blue background for profile image - change image button visibility issue

UI itself is preparing the data for it (instead of ViewModel)

Following code segments needs attention :

MyChat/app/src/main/java/com/dc/mychat/ui/screens/MessageScreen.kt

Lines 75 to 102 in dd6b19a

	for (i in messagesViewModel.allMessagesState.indices) {
	val message = messagesViewModel.allMessagesState[i]
	if (
	i + 1 <= messagesViewModel.allMessagesState.size - 1 &&
	messagesViewModel.allMessagesState[i].timestamp.toDate().date !=
	messagesViewModel.allMessagesState[i + 1].timestamp.toDate().date
	) {
	item {
	MessageCard(message = message, sharedViewModel = sharedViewModel)
	Row(
	modifier = Modifier.fillMaxWidth(),
	horizontalArrangement = Arrangement.Center
	) {
	Text(
	text = SimpleDateFormat(
	"dd MMMM",
	Locale.US
	).format(messagesViewModel.allMessagesState[i + 1].timestamp.toDate()),
	fontWeight = FontWeight.Medium
	)
	}
	}
	} else {
	item {
	MessageCard(message = message, sharedViewModel = sharedViewModel)
	}
	}
	}

MyChat/app/src/main/java/com/dc/mychat/ui/screens/components/MessageCard.kt

Lines 68 to 71 in dd6b19a

	text = SimpleDateFormat(
	"h:mm a",
	Locale.US
	).format(message.timestamp.toDate()),

MyChat/app/src/main/java/com/dc/mychat/ui/screens/components/MessageCard.kt

Line 24 in dd6b19a

val isRight = sharedViewModel.senderProfile!!.mailId == message.senderMailId

All the above code comes under "preparing the data for UI" and it is the responsibility of ViewModel to do so. Composables are supposed to just render the prepared data & not prepare data (which in your case it is doing).

---

Why is this a big deal?

• Because this code gets executed on the Main thread affecting the UI performance. If you do so much of such tasks, you might see system logs like "I/Choreographer: Skipped <number> frames! The application may be doing too much work on its main thread."

• Because it is performed inside a composable. Composable are very dynamic & reactive to state changes. A small change (like sending a new message) might trigger a recomposition multiple times. And on every recomposition, your code gets executed (even though no data related to dates might have changed).

Note : You can add logs under for loop and observe the number of times this data processing happens! Now imagine the amount of processing that might happen when there are 1000s of messages (& no pagination).

---

Even worse situation!

A far worse thing happens when user receives a new message while the screen is open i.e. the snapshotListener is triggered. The complete messages list is recreated i.e. new objects at new addresses are created :

MyChat/app/src/main/java/com/dc/mychat/data/repository/remote/MessageRepositoryImp.kt

Lines 73 to 74 in dd6b19a

	listOfMessages =
	snapshot.toObject(Messages::class.java)?.messageArray ?: emptyList()

And guess what... Compose thinks that the data is changed when address is changed even though the content remains the same. As a result of which Above Code segments No.2 & No.3 get re-executed, that too on the Main thread.

These things might seem small but they have the potential to cause your app to slow down because Compound interest applies here also!

---

What's the solution then?

As a rule of thumb, always prepare all the major data for UI inside ViewModel and pass it "as-is" to the Composable. Make sure that the composable need not do any transformation on the data it receives.

Recall that we have 3 layers : Domain, Data & UI. It is not necessary that UI has to use Domain models "as-is". UI can have models of its own based on requirements. Example: You have Message.Kt model in Domain layer but in addition to these message models, you also need dates in UI layer. So, you can have a new UI model like this :

sealed class MessageUiModel {
   class Message(message: String?, imageUrl: String?, type: MessageType, time: String): MessageUiModel()
   class Date(date: String): MessageUiModel()
}


enum class MessageType {
    SENT, RECEIVED
}

And then ViewModel needs to transform listOf<Message> to listOf<MessageUiModel> & then pass it to the composable.

Note : Make sure that this transformation happens only once when the screen is loaded. When a new message is sent, DO NOT transform the entire data, instead just transform the last sent message & add it to the existing list.

However, because of the architecture you have chosen for saving messages in Firestore (all in one document), we can't avoid the useless re-transformation of the entire list when a new message is received. If you can find a way to avoid this, it would be great!

ReceiverMailId shown as notification title instead of Sender’s name

UI hangs - no pogress indicator when uploading image & creating profile

Receiver name not shown - no app bar on MessagesScreen

Poor Error Handling

Currently if any exception occurs in ViewModel, then the app crashes, which is bad experience for user. User must be notified of the error.
In case of error, dialog should appear displaying the error message to user & an optional Retry button (if applicable).
You can use flow to emit ErrorUiEvent to the Activity & then activity can display the dialog.

FCMReceiverService : Using Utils instead of LocalRepository to read from prefs

MyChat/app/src/main/java/com/dc/mychat/other/fcm/FCMReceiverService.kt

Line 38 in ea3d0b1

val userProfile = Utils.getProfileFromPrefs(this)

App crashes when not changing the profile picture & saving

Login state not saved

Login → DON'T save profile → go back → reopen app → need to relogin.

Login state should be saved. On reopening, ProfileScreen should be shown if profile is not yet created

Blank notification for new image message

Expected : Image received should be shown in the notification

Actual :

Remove runBlocking { } calls

runBlocking calls hangs the UI. It should never be used in Android apps.

I understand that you want to wait until the IO operation completes and then navigate to another screen.

A better approach to the same thing without runBlocking is to pass the navController or a lambda (onComplete) to the function, do the IO task in viewModelScope & then invoke the lambda or navigate after the IO task completes.

MyChat/app/src/main/java/com/dc/mychat/ui/viewmodel/MainViewModel.kt

Lines 101 to 103 in a7d4a94

	runBlocking {
	senderMailIdState.value = userRepository.getProfileFromPrefs().mailId
	}

MyChat/app/src/main/java/com/dc/mychat/ui/viewmodel/MainViewModel.kt

Lines 107 to 115 in a7d4a94

	runBlocking {
	Log.d("TAG 9.5", "Mainviewmodel, Create Profile ")
	val uri = Uri.parse(profile.displayPhoto)
	val downloadedUrl = serverRepository.uploadProfilePicture(imageUriState.value!!, profile)
	Log.d("TAG", "createProfile: $downloadedUrl")
	serverRepository.createProfile(profile.copy(displayPhoto = downloadedUrl))
	saveLoginStatusToPrefs(true)
	Log.d("TAG 9.5.2", "Inside MainViewModel createProfile End")
	}

MyChat/app/src/main/java/com/dc/mychat/ui/viewmodel/MainViewModel.kt

Lines 132 to 135 in a7d4a94

	runBlocking {
	loginStatusState.value = userRepository.getLoginStatusFromPrefs()
	Log.d("TAG 9.3", loginStatusState.toString())
	}

MyChat/app/src/main/java/com/dc/mychat/ui/viewmodel/MainViewModel.kt

Lines 145 to 147 in a7d4a94

	runBlocking {
	val email = it.email!!
	val displayPhoto = it.photoUrl.toString()

Rename LoggedInScreen to LoginScreen

Only 1 God ViewModel - Refactor MainViewModel

Problem :

Currently there is only 1 god ViewModel in the entire app that is doing all the operations for all the screens. The problems with this approach are :

• ViewModel becomes complex (+ more lines of code), hard to understand & modify
• Large number of states as it hosts state for more than one screens
• More states in memory than required e.g. - Login states are still in memory when the user is on MessagesScreen

Solution :

Divide the MainViewModel into multiple ViewModels - one for each screen:

• LoginViewModel
• ProfileViewModel
• AllUsersViewModel
• MessagesViewModel

Now that the viewModel is divided, data sharing between screens will stop. In that case, you can use navigation arguments to pass data from one sceen to another.

OutlinedTextField - Hint textSize > input textSize (Do not override default text sizes)

Invalid back navigation after login

User is able to navigate back to LoginScreen from ProfileScreen after logging in.

Inifinite loader : Send new message while offline

Adding a check for internet connection can prevent this!

Duplicate declaration of the same fields

Why are displayName, email, displayPhoto redeclared in MainViewModel when they are part of profile model? Can't we access them directly from profile object?

MyChat/app/src/main/java/com/dc/mychat/ui/viewmodel/MainViewModel.kt

Lines 146 to 149 in a7d4a94

	val email = it.email!!
	val displayPhoto = it.photoUrl.toString()
	val displayName = it.displayName.toString()
	val profile = Profile(displayName, email, displayPhoto)

Why create a new reference to exceptionHandler in other ViewModels?

MyChat/app/src/main/java/com/dc/mychat/ui/viewmodel/ProfileViewModel.kt

Line 24 in ea3d0b1

private val profileExceptionHandler = exceptionHandler

Person is able to chat with himself & receive notifications

Custom profile picture not fetched on relogin

Login → Update Profile picture → Clear data → Relogin → Google's profile picture is displayed instead of updated custom profile picture

FCMReceiverService is not a repository

Move it to the root package or other or root/fcm package

Tap change image but don’t select image - default image gets removed