Fix Navigation Bug on Device Rotation #119
Conversation
- Used factory instantiation for the view model that have it a disposal - Made HomeScreen use the global event view model instead of initializing a new one - Removed a code block that shouldn't be there (from a merge error perhaps)
- Moved login logic from MainActivity to WelcomeScreen - Added a UserRepositoryFirestore parameter to WelcomeScreen for the login logic - Adapted the existing tests to the addition of the parameter
|
All I have left is to test this logic in the Welcome test file. |
Added tests that check that the logic in the Auth state listener (that was moved to the Welcome screen) is well implemented. This includes: - Test that checks the behavior when the user returned is null - Test that checks the behavior when the user is authenticated and has verified his email - Test that checks the behavior when the user is authenticated and has verified his email, but does not have his name profile - Test that checks the behavior when the user is authenticated but hasn't verified his email - Test that checks the behavior when the user is not authenticated
This test corresponds to a slightly different login logic which was changed in the previous commit, hence this test deletion.
This was originally for a new test to test Toast displays, however it is not possible without Kaspresso.
|
Ready to be reviewed and merged. |
Redd87
left a comment
Redd87
left a comment
There was a problem hiding this comment.
A solid pull request, good work overall! When you merge you'll have conflict in the Main as I reordered the initialization of the repos and viewmodels, so be aware of that.
Thanks, will resolve the conflicts and merge. |
|
Fix does not work after updating the branch with main. Can't quite figure out why. |
This is to prevent the navController to execute a navigation to a screen it is already on.
This hosts the Auth State Listener logic that was originally implemented in the MainActivity.
These removed tests were written when the Welcome screen contained the login Auth State Listener (it is now in AuthViewModel).
|
A solution to the bug was found through the usage of an authentication view model. Please see
This effectively fixes the bug that redirects the user to the Home Screen whenever the app composable was refreshed, caused by, for example, a switch between light and dark mode or a rotation of the screen. |
oskar-codes
left a comment
oskar-codes
left a comment
There was a problem hiding this comment.
Incredible work on this 🎉
This is a much needed fix for an issue that could have caused us some real trouble. I pointed out a few minor changes you could address, but I gladly approve this pull request.
app/src/main/java/com/android/unio/model/authentication/AuthViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/android/unio/ui/authentication/Welcome.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/android/unio/ui/navigation/NavigationAction.kt
Outdated
Show resolved
Hide resolved
- Modified the if statement in navigateTo(screen: String) - Added edge case to navigateTo(tld: TopLevelDestination)
This should already be taken care of by the launchSingleTop property.
Thank you! Your points have been taken into account and I have made the necessary changes. |
|
Description
It was noted in the MS1 grading that there was an issue with device orientation: When the device is rotated in the Explore page, the app is redirected to the Home Screen.
The same bug is present when we switch from light to dark mode, and vice-versa.
Testing this shows that the bug is present in all screens. The bug cannot only be in the Explore screen as it does not define the behavior of our
NavHost.The addition of the auth state listener to is the problem. It is done each time the
UnioAppcomposable is recomputed.Changes
Edit 1:
Extracting the logic from the MainActivity and putting it into WelcomeScreen with the rest of the login logic fixes the bug.
⚠️ This modification also fixes the dark mode activation bug, which also caused the app to take us back to the Home Screen. It is expected that it fixes all bugs of this type, e.g. due to the refresh.
I am still having issues with writing tests for this logic, but the bug is gone!
Edit 2:
The changes from
Edit 1were not compatible with new updates that were made in the main branch during the development of this bug fix.This is what I ended up doing:
AuthViewModel, that handles the logic of the Auth State listener and its stateMainActivityin a manner that fixes the bug, e.g. it remembers the state of the login and which screen it was on.This solution works and the view model has been tested.
Testing
The login logic in
AuthViewModelhas been tested to ensure the state transitioning is correct.