ground-android icon indicating copy to clipboard operation
ground-android copied to clipboard

[Code health] Fix duplicate / unclear EditObservation* flows

Open gino-m opened this issue 3 years ago • 2 comments
trafficstars

There are several problems with the lifecycle of EditObservationFragment and EditObservationViewModel while working on #1077:

  • initialize() is called when the ViewModel is being restored, even if it was already initialized. As a result, the Form is re-rendered multiple times: once when the ViewModel is observed, and once when the Observation reloads.
  • There are also several click event streams which use LiveData, which replays events which could lead to unexpected behaviors.
  • New subscriptions are added each time the form is rebuilt, but are only disposed when the Fragment is destroyed. This causes a leak, and for event handlers to be called multiple times (once for each subscription created on each config change). One approach would be to add them to a CompositeDisposable that gets cleared each time the form is rebuilt.
  • How the form LiveData is derived from the initialize stream is unintuitive; might be better to rely on side effects in this case (MutableLiveData.setValue()).

@shobhitagarwal1612 FYI.

gino-m avatar Nov 24 '21 00:11 gino-m

Something we may also want to consider is moving the sync parts of the async flow into their own components. For example, if we could have view and view model contained in EditObservationFragment that is only created once the observation is loaded, we could simplify some of the async flows.

Also, ideally we would also persist the UI layout in the saved instance state state rather than reloading on each config change.

gino-m avatar Nov 24 '21 15:11 gino-m

Confirmed that rebuildForm is called twice on changing screen orientation.

shobhitagarwal1612 avatar Nov 25 '21 03:11 shobhitagarwal1612

@JSunde @shobhitagarwal1612 Is this still relevant?

gino-m avatar Feb 27 '23 17:02 gino-m

I think we can mark this as obsolete too. We haven't removed EditObservationFragment in a single shot but are constantly chipping away pieces from it when we implement new features to the redesigned data collection flow. Eventually, all of the bugs related to the previous edit/view observation flow should automatically get cleaned up.

shobhitagarwal1612 avatar Feb 27 '23 18:02 shobhitagarwal1612

I haven't necessarily been deleting things from the EditSubmissionFragment as I've been replicating the functionality in the new Data Collection flow. Would it be worth going in and deleting the functionality that we have working in the new flow, while keeping around anything remaining for context as we continue to build out the newer experience?

JSunde avatar Feb 27 '23 19:02 JSunde

Sounds good to me. We can create a list of functionality pending in the current implementation and remove the old code.

shobhitagarwal1612 avatar Feb 27 '23 20:02 shobhitagarwal1612

@JSunde Can we close this now?

shobhitagarwal1612 avatar May 01 '23 16:05 shobhitagarwal1612