Skip to content

Commit

Permalink
Use LiveData in RecordDetailsViewModel. Update gitignore.
Browse files Browse the repository at this point in the history 
I use Ctags+vim in addition to android studio so I updated gitignore to
ignore tags files.
  • Loading branch information
@scolsen
scolsen committed Mar 1, 2019
1 parent d9cb25a commit e678555
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 31 deletions.
3 changes: 3 additions & 0 deletions .gitignore
View file
Expand Up @@ -41,3 +41,6 @@ captures/

# IntelliJ
*.iml

# Ctags
**/tags
1 change: 1 addition & 0 deletions gnd/src/main/java/com/google/android/gnd/ui/recorddetails/RecordDetailsFragment.java
View file
Expand Up @@ -72,6 +72,7 @@ public View onCreateView(
RecordDetailsFragBinding binding =
RecordDetailsFragBinding.inflate(inflater, container, false);
binding.setViewModel(viewModel);
binding.setLifecycleOwner(this);
return binding.getRoot();
}

Expand Down
53 changes: 32 additions & 21 deletions gnd/src/main/java/com/google/android/gnd/ui/recorddetails/RecordDetailsViewModel.java
View file
Expand Up @@ -17,51 +17,62 @@
package com.google.android.gnd.ui.recorddetails;

import android.arch.lifecycle.LiveData;
import android.arch.lifecycle.MutableLiveData;
import android.databinding.ObservableInt;
import android.arch.lifecycle.LiveDataReactiveStreams;
import android.view.View;

import com.google.android.gnd.repository.DataRepository;
import com.google.android.gnd.repository.Resource;
import com.google.android.gnd.ui.common.AbstractViewModel;
import com.google.android.gnd.vo.Record;

import io.reactivex.BackpressureStrategy;
import io.reactivex.Observable;

import javax.inject.Inject;

public class RecordDetailsViewModel extends AbstractViewModel {

private final DataRepository dataRepository;
private final MutableLiveData<Resource<Record>> record;
public final ObservableInt progressBarVisibility = new ObservableInt();
private LiveData<Resource<Record>> record;
private LiveData<Integer> progressBarVisibility;

@Inject
RecordDetailsViewModel(DataRepository dataRepository) {
this.dataRepository = dataRepository;
this.record = new MutableLiveData<>();
}

public LiveData<Resource<Record>> getRecord() {
return record;
}

public LiveData<Integer> getProgressBarVisibility() {
return progressBarVisibility;
}

public void loadRecordDetails(String projectId, String featureId, String recordId) {
disposeOnClear(
dataRepository
.getRecordDetails(projectId, featureId, recordId)
.subscribe(this::onRecordUpdate));
Observable<Resource<Record>> recordDetials =
dataRepository.getRecordDetails(projectId, featureId, recordId);

progressBarVisibility =

This comment has been minimized.

Sign in to view

Copy link
@gino-m

gino-m Mar 6, 2019 •
edited

Collaborator

Deleted all my comments because I just realized we can't just replace the LiveDatas mid-lifecycle; previous LiveDatas will continue to live since there will already be observers, and the view will not know to look at the new LiveData.

For hot Observables, we would normally initialize these in the constructor/lifecycle method. But since these are cold (started and finished by the user action), that won't work unless we create something akin to a hot Observable "LoadRecordDetailsRequest" that would initiate getRecordDetails on emissions, switchMapping the result. While this feels a bit contrived, it might make sense if the UI clicks were treated as a hot Observable.

This comment has been minimized.

Sign in to view

Copy link
@scolsen

scolsen Mar 6, 2019

Author Contributor

Ahhh, I was going to ask you about this myself—the fact that I couldn't initiate these in the constructor since they're dependent on the UI interaction bothered me—but I'm not steeped deeply enough in OOP or even Reactive lore to know the means of refactoring the situation.

This sounds like a good approach. I would prefer to initiate these in the constructor too, as I think it's cleaner and makes more sense conceptually, so, even if it requires a little contrivance, I think this is worth a shot (especially if a pattern starts to emerge whereby we can abstract over any and all situations like this (where some portion of the VM is dependent on UI interaction) with some cold->hot observable helper). I'll take a stab at it.
LiveDataReactiveStreams.fromPublisher(
recordDetials
.map(this::toProgressBarVisibility)
.onErrorReturn(err -> View.GONE)
.toFlowable(BackpressureStrategy.LATEST));

record =
LiveDataReactiveStreams.fromPublisher(
recordDetials
.map(r -> r)
.onErrorReturn(err -> Resource.error(err))
.toFlowable(BackpressureStrategy.LATEST));
}

private void onRecordUpdate(Resource<Record> r) {
switch (r.operationState().get()) {
case LOADING:
progressBarVisibility.set(View.VISIBLE);
break;
case LOADED:
progressBarVisibility.set(View.GONE);
break;
case NOT_FOUND:
case ERROR:
break;
private Integer toProgressBarVisibility(Resource<Record> r) {
if(r.operationState().get() == Resource.Status.LOADING) {

This comment has been minimized.

Sign in to view

Copy link
@gino-m

gino-m Mar 6, 2019

Collaborator

Nit: I'd still prefer switch over if for enums, since Lint checks can then tell you if any enum values were missed. It also gives you syntactic sugar of not needing to namespace the enum with Resource.Status.

This comment has been minimized.

Sign in to view

Copy link
@scolsen

scolsen Mar 6, 2019

Author Contributor

Sure thing! Also that bit of syntactic sugar was weird to me at first—I was confused when I had to namespace the if statement lol. But it makes sense when you consider the semantics of each construct (a switch can be implicitly bound to a particular lexical context/closure, an if statement can't). It'd be cool if there were some "lambda if" form to restrict the lexical context in if statements too. Though I suppose the amount of convince you'd derive from something like that is negligent. Thanks for pointing this out!—it's a general semantics tip that is widely applicable, so I'm going to try and use it more often. I'll change this back (at least until Java 30 gives us pattern matching :P).
return View.VISIBLE;
} else {
return View.GONE;
}
record.setValue(r);
}
}
20 changes: 10 additions & 10 deletions gnd/src/main/res/layout/record_details_frag.xml
View file
Expand Up @@ -34,7 +34,16 @@
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical">


<ProgressBar
android:id="@+id/record_details_progress_bar"
style="?android:attr/progressBarStyle"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_gravity="center"
android:padding="48dp"
android:visibility="@{safeUnbox(viewModel.progressBarVisibility)}"/>

<!-- CardView is used to create the drop shadow effect -->
<android.support.v7.widget.CardView
android:id="@+id/record_details_header"
Expand Down Expand Up @@ -72,15 +81,6 @@

</android.support.v7.widget.CardView>

<ProgressBar
android:id="@+id/record_details_progress_bar"
style="?android:attr/progressBarStyle"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_gravity="center"
android:padding="48dp"
android:visibility="@{viewModel.progressBarVisibility}"/>

<ScrollView
android:layout_width="match_parent"
android:layout_height="match_parent"
Expand Down

1 comment on commit e678555

@gino-m
Copy link
Collaborator

@gino-m gino-m commented on e678555 Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts, and more questions. Thanks for your patience!

Please sign in to comment.