[android] fix concurrency issues when attachments are received delayed

Do not observe attachment live data multiple times
and don't miss received attachments in ImageActivity resp. ImageViewModel.
This commit is contained in:
Torsten Grote
2020-02-11 10:11:09 -03:00
parent 7c22016b81
commit c1cf6f61b9
6 changed files with 88 additions and 18 deletions

View File

@@ -1109,6 +1109,10 @@ public class ConversationActivity extends BriarActivity
attachmentRetriever.getAttachmentItems(h); attachmentRetriever.getAttachmentItems(h);
List<AttachmentItem> items = new ArrayList<>(liveDataList.size()); List<AttachmentItem> items = new ArrayList<>(liveDataList.size());
for (LiveData<AttachmentItem> liveData : liveDataList) { for (LiveData<AttachmentItem> liveData : liveDataList) {
// first remove all our observers to avoid having more than one
// in case we reload the conversation, e.g. after deleting messages
liveData.removeObservers(this);
// add a new observer
liveData.observe(this, new AttachmentObserver(h.getId(), liveData)); liveData.observe(this, new AttachmentObserver(h.getId(), liveData));
items.add(requireNonNull(liveData.getValue())); items.add(requireNonNull(liveData.getValue()));
} }

View File

@@ -103,9 +103,20 @@ public class ImageActivity extends BriarActivity
setSceneTransitionAnimation(transition, null, transition); setSceneTransitionAnimation(transition, null, transition);
} }
// Intent Extras
Intent i = getIntent();
attachments =
requireNonNull(i.getParcelableArrayListExtra(ATTACHMENTS));
int position = i.getIntExtra(ATTACHMENT_POSITION, -1);
if (position == -1) throw new IllegalStateException();
String name = i.getStringExtra(NAME);
long time = i.getLongExtra(DATE, 0);
byte[] messageIdBytes = requireNonNull(i.getByteArrayExtra(ITEM_ID));
// get View Model // get View Model
viewModel = ViewModelProviders.of(this, viewModelFactory) viewModel = ViewModelProviders.of(this, viewModelFactory)
.get(ImageViewModel.class); .get(ImageViewModel.class);
viewModel.expectAttachments(attachments);
viewModel.getSaveState().observeEvent(this, viewModel.getSaveState().observeEvent(this,
this::onImageSaveStateChanged); this::onImageSaveStateChanged);
@@ -129,17 +140,11 @@ public class ImageActivity extends BriarActivity
TextView contactName = toolbar.findViewById(R.id.contactName); TextView contactName = toolbar.findViewById(R.id.contactName);
TextView dateView = toolbar.findViewById(R.id.dateView); TextView dateView = toolbar.findViewById(R.id.dateView);
// Intent Extras // Set contact name and message time
Intent i = getIntent();
attachments = i.getParcelableArrayListExtra(ATTACHMENTS);
int position = i.getIntExtra(ATTACHMENT_POSITION, -1);
if (position == -1) throw new IllegalStateException();
String name = i.getStringExtra(NAME);
long time = i.getLongExtra(DATE, 0);
String date = formatDateAbsolute(this, time); String date = formatDateAbsolute(this, time);
contactName.setText(name); contactName.setText(name);
dateView.setText(date); dateView.setText(date);
conversationMessageId = new MessageId(i.getByteArrayExtra(ITEM_ID)); conversationMessageId = new MessageId(messageIdBytes);
// Set up image ViewPager // Set up image ViewPager
viewPager = findViewById(R.id.viewPager); viewPager = findViewById(R.id.viewPager);

View File

@@ -95,8 +95,6 @@ public class ImageFragment extends Fragment
viewModel = ViewModelProviders.of(requireNonNull(getActivity()), viewModel = ViewModelProviders.of(requireNonNull(getActivity()),
viewModelFactory).get(ImageViewModel.class); viewModelFactory).get(ImageViewModel.class);
viewModel.getOnAttachmentLoaded()
.observeEvent(this, this::onAttachmentLoaded);
photoView = v.findViewById(R.id.photoView); photoView = v.findViewById(R.id.photoView);
photoView.setScaleLevels(1, 2, 4); photoView.setScaleLevels(1, 2, 4);
@@ -111,6 +109,9 @@ public class ImageFragment extends Fragment
} else { } else {
photoView.setImageResource(R.drawable.ic_image_missing); photoView.setImageResource(R.drawable.ic_image_missing);
startPostponedTransition(); startPostponedTransition();
// state is not final, so observe state changes
viewModel.getOnAttachmentReceived(attachment.getMessageId())
.observeEvent(this, this::onAttachmentReceived);
} }
return v; return v;
@@ -127,8 +128,8 @@ public class ImageFragment extends Fragment
.into(photoView); .into(photoView);
} }
private void onAttachmentLoaded(MessageId messageId) { private void onAttachmentReceived(Boolean received) {
if (attachment.getMessageId().equals(messageId)) loadImage(); if (received) loadImage();
} }
@Override @Override

View File

@@ -27,7 +27,9 @@ import java.io.InputStream;
import java.io.OutputStream; import java.io.OutputStream;
import java.text.SimpleDateFormat; import java.text.SimpleDateFormat;
import java.util.Date; import java.util.Date;
import java.util.List;
import java.util.Locale; import java.util.Locale;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Executor; import java.util.concurrent.Executor;
import java.util.logging.Logger; import java.util.logging.Logger;
@@ -40,6 +42,7 @@ import androidx.lifecycle.AndroidViewModel;
import static android.media.MediaScannerConnection.scanFile; import static android.media.MediaScannerConnection.scanFile;
import static android.os.Environment.DIRECTORY_PICTURES; import static android.os.Environment.DIRECTORY_PICTURES;
import static android.os.Environment.getExternalStoragePublicDirectory; import static android.os.Environment.getExternalStoragePublicDirectory;
import static java.util.Objects.requireNonNull;
import static java.util.logging.Level.WARNING; import static java.util.logging.Level.WARNING;
import static java.util.logging.Logger.getLogger; import static java.util.logging.Logger.getLogger;
import static org.briarproject.bramble.util.IoUtils.copyAndClose; import static org.briarproject.bramble.util.IoUtils.copyAndClose;
@@ -57,8 +60,10 @@ public class ImageViewModel extends AndroidViewModel implements EventListener {
@IoExecutor @IoExecutor
private final Executor ioExecutor; private final Executor ioExecutor;
private final MutableLiveEvent<MessageId> attachmentLoaded = private volatile boolean receivedAttachmentsInitialized = false;
new MutableLiveEvent<>(); private ConcurrentHashMap<MessageId, MutableLiveEvent<Boolean>>
receivedAttachments = new ConcurrentHashMap<>();
/** /**
* true means there was an error saving the image, false if image was saved. * true means there was an error saving the image, false if image was saved.
*/ */
@@ -91,13 +96,36 @@ public class ImageViewModel extends AndroidViewModel implements EventListener {
@Override @Override
public void eventOccurred(Event e) { public void eventOccurred(Event e) {
if (e instanceof AttachmentReceivedEvent) { if (e instanceof AttachmentReceivedEvent) {
attachmentLoaded MessageId id = ((AttachmentReceivedEvent) e).getMessageId();
.postEvent(((AttachmentReceivedEvent) e).getMessageId()); MutableLiveEvent<Boolean> oldEvent;
if (receivedAttachmentsInitialized) {
oldEvent = receivedAttachments.get(id);
} else {
oldEvent = receivedAttachments
.putIfAbsent(id, new MutableLiveEvent<>(true));
}
if (oldEvent != null) oldEvent.postEvent(true);
} }
} }
LiveEvent<MessageId> getOnAttachmentLoaded() { @UiThread
return attachmentLoaded; public void expectAttachments(List<AttachmentItem> attachments) {
for (AttachmentItem item : attachments) {
// no need to track items that are in a final state already
if (item.getState().isFinal()) continue;
// add new live events, if not added concurrently by Event
MessageId id = item.getMessageId();
receivedAttachments.putIfAbsent(id, new MutableLiveEvent<>());
}
receivedAttachmentsInitialized = true;
}
@UiThread
LiveEvent<Boolean> getOnAttachmentReceived(MessageId messageId) {
if (receivedAttachments.size() == 0) {
throw new IllegalStateException("expectAttachments() not called");
}
return requireNonNull(receivedAttachments.get(messageId));
} }
void clickImage() { void clickImage() {

View File

@@ -12,6 +12,22 @@ import androidx.lifecycle.Observer;
@NotNullByDefault @NotNullByDefault
public class LiveEvent<T> extends LiveData<LiveEvent.ConsumableEvent<T>> { public class LiveEvent<T> extends LiveData<LiveEvent.ConsumableEvent<T>> {
/**
* Creates a LiveEvent initialized with the given {@code value}.
*
* @param value initial value
*/
public LiveEvent(T value) {
super(new ConsumableEvent<>(value));
}
/**
* Creates a LiveEvent with no value assigned to it.
*/
public LiveEvent() {
super();
}
public void observeEvent(LifecycleOwner owner, public void observeEvent(LifecycleOwner owner,
LiveEventHandler<T> handler) { LiveEventHandler<T> handler) {
LiveEventObserver<T> observer = new LiveEventObserver<>(handler); LiveEventObserver<T> observer = new LiveEventObserver<>(handler);

View File

@@ -5,6 +5,22 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
@NotNullByDefault @NotNullByDefault
public class MutableLiveEvent<T> extends LiveEvent<T> { public class MutableLiveEvent<T> extends LiveEvent<T> {
/**
* Creates a MutableLiveEvent initialized with the given {@code value}.
*
* @param value initial value
*/
public MutableLiveEvent(T value) {
super(value);
}
/**
* Creates a MutableLiveEvent with no value assigned to it.
*/
public MutableLiveEvent() {
super();
}
public void postEvent(T value) { public void postEvent(T value) {
super.postValue(new ConsumableEvent<>(value)); super.postValue(new ConsumableEvent<>(value));
} }