Removed Navigator from TermsOfServiceFragment, SurveySelectorFragment and OfflineAreasFragment

[anandwana001]

Fixes #2776

@shobhitagarwal1612 PTAL?

Removed Navigator from TermsOfServiceFragment, SurveySelectorFragment and OfflineAreasFragment

Part 2 - #2896

[codecov]

Codecov Report

Attention: Patch coverage is 62.22222% with 17 lines in your changes missing coverage. Please review.

Project coverage is 61.62%. Comparing base (9ab91bc) to head (72cb011).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...flineareas/selector/OfflineAreaSelectorFragment.kt	11.11%	7 Missing and 1 partial ⚠️
...oid/ground/ui/offlineareas/OfflineAreasFragment.kt	71.42%	1 Missing and 1 partial ⚠️
...lineareas/selector/OfflineAreaSelectorViewModel.kt	50.00%	2 Missing ⚠️
...android/ground/ui/offlineareas/selector/UiState.kt	0.00%	2 Missing ⚠️
...i/offlineareas/viewer/OfflineAreaViewerFragment.kt	66.66%	0 Missing and 1 partial ⚠️
...ground/ui/surveyselector/SurveySelectorFragment.kt	50.00%	0 Missing and 1 partial ⚠️
...le/android/ground/ui/tos/TermsOfServiceFragment.kt	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2889      +/-   ##
============================================
- Coverage     61.68%   61.62%   -0.06%     
- Complexity     1173     1178       +5     
============================================
  Files           268      269       +1     
  Lines          6344     6372      +28     
  Branches        876      883       +7     
============================================
+ Hits           3913     3927      +14     
- Misses         1890     1900      +10     
- Partials        541      545       +4     

Files with missing lines	Coverage Δ
...id/ground/ui/offlineareas/OfflineAreasViewModel.kt	100.00% <100.00%> (ø)
.../offlineareas/viewer/OfflineAreaViewerViewModel.kt	100.00% <100.00%> (ø)
...round/ui/surveyselector/SurveySelectorViewModel.kt	93.61% <100.00%> (-0.39%)	⬇️
...google/android/ground/ui/surveyselector/UiState.kt	100.00% <100.00%> (ø)
...i/offlineareas/viewer/OfflineAreaViewerFragment.kt	47.82% <66.66%> (+2.82%)	⬆️
...ground/ui/surveyselector/SurveySelectorFragment.kt	59.32% <50.00%> (-2.09%)	⬇️
...le/android/ground/ui/tos/TermsOfServiceFragment.kt	88.23% <85.71%> (-1.77%)	⬇️
...oid/ground/ui/offlineareas/OfflineAreasFragment.kt	70.37% <71.42%> (+6.73%)	⬆️
...lineareas/selector/OfflineAreaSelectorViewModel.kt	25.97% <50.00%> (+0.97%)	⬆️
...android/ground/ui/offlineareas/selector/UiState.kt	0.00% <0.00%> (ø)
... and 1 more

[shobhitagarwal1612]

What would be the side-effect of this fix? The navigation won't happen so the screen won't be moved to the next destination? Can you please try and replicate one scenario forcefully to understand what the user will see?

[anandwana001]

The user will remain on the screen. However, this type of condition comes to the user in a worst case where the naviations instance are having are issue.

[shobhitagarwal1612]

I'm not sure if we should silently let the app fail.

@gino-m Thoughts?

[shobhitagarwal1612]

@anandwana001 Can you look into options on how to resolve this problem?

[gino-m]

I'm not sure if we should silently let the app fail.

@gino-m Thoughts?

It might be ok if this is a degenerate case, but given the Crashlytics logs, it seems it's not @anandwana001 Why is this failing in the first place?

[anandwana001]

@gino-m @shobhitagarwal1612 I did take a look at all the logs again: (updated issue #2776) logs

1. submissionConfirmationDialogFragment - no more exists
2. showTermsOfService - the terms_of_service_fragment is trying to open itself again using the navigate() function
3. showOfflineAreas - the offline_areas_fragment is trying to open itself again using the navigate() function

Not reproable, but as per the understanding the _navigateRequests flow which is the source emitting the destinations is fixed in this PR, as earlier the scope was different.

[gino-m]

Not reproable, but as per the understanding the _navigateRequests flow which is the source emitting the destinations is fixed in this PR, as earlier the scope was different.

@anandwana001 In this PR, the lifecycle is still bound to the Activity, which is the wrong scope, since the fragment requesting the navigation may already be gone by the time the Activity receives the requests.

Can we instead remove Navigator and related code in MainActivity, an instead opt for the simpler approach of ViewModel-scoped navigation as described in https://developer.android.com/guide/navigation?

[gino-m]

In many cases case UiState isn't actually modeling states, but actually actions/events. Can we rename those to NavEvent in a fast-follow?

Also, in cases UiState classes are used in a SharedStateFlow, could we please switch those over the a SharedFlow if it makes sense?

Filed google/ground-platform#2105 to track.