Filter attachment URIs in controller.

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java

@@ -6,7 +6,6 @@ import android.arch.lifecycle.ViewModelProvider;
 import android.arch.lifecycle.ViewModelProviders;
 import android.content.DialogInterface;
 import android.content.Intent;
-import android.net.Uri;
 import android.os.Bundle;
 import android.os.Parcelable;
 import android.support.annotation.Nullable;
@@ -653,15 +652,11 @@ public class ConversationActivity extends BriarActivity
 	}
 
 	@Override
-	public List<Uri> filterAttachmentUris(List<Uri> uris) {
-		if (uris.size() > MAX_ATTACHMENTS_PER_MESSAGE) {
-			String format = getResources().getString(
-					R.string.messaging_too_many_attachments_toast);
-			String warning = String.format(format, MAX_ATTACHMENTS_PER_MESSAGE);
-			Toast.makeText(this, warning, LENGTH_SHORT).show();
-			uris = uris.subList(0, MAX_ATTACHMENTS_PER_MESSAGE);
-		}
-		return new ArrayList<>(uris);
+	public void onTooManyAttachments() {
+		String format = getResources().getString(
+				R.string.messaging_too_many_attachments_toast);
+		String warning = String.format(format, MAX_ATTACHMENTS_PER_MESSAGE);
+		Toast.makeText(this, warning, LENGTH_SHORT).show();
 	}
 
 	@Override

briar-android/src/main/java/org/briarproject/briar/android/view/TextAttachmentController.java

@@ -43,6 +43,7 @@ import static android.view.View.GONE;
 import static android.widget.Toast.LENGTH_LONG;
 import static org.briarproject.briar.android.util.UiUtils.resolveColorAttribute;
 import static org.briarproject.briar.api.messaging.MessagingConstants.IMAGE_MIME_TYPES;
+import static org.briarproject.briar.api.messaging.MessagingConstants.MAX_ATTACHMENTS_PER_MESSAGE;
 import static uk.co.samuelwall.materialtaptargetprompt.MaterialTapTargetPrompt.STATE_DISMISSED;
 import static uk.co.samuelwall.materialtaptargetprompt.MaterialTapTargetPrompt.STATE_FINISHED;
 
@@ -153,25 +154,28 @@ public class TextAttachmentController extends TextSendController
 	public void onImageReceived(@Nullable Intent resultData) {
 		if (resultData == null) return;
 		if (loadingUris || !imageUris.isEmpty()) throw new AssertionError();
+		List<Uri> newUris = new ArrayList<>();
 		if (resultData.getData() != null) {
-			imageUris.add(resultData.getData());
-			onNewUris(false);
+			newUris.add(resultData.getData());
+			onNewUris(false, newUris);
 		} else if (SDK_INT >= 18 && resultData.getClipData() != null) {
 			ClipData clipData = resultData.getClipData();
 			for (int i = 0; i < clipData.getItemCount(); i++) {
-				imageUris.add(clipData.getItemAt(i).getUri());
+				newUris.add(clipData.getItemAt(i).getUri());
 			}
-			onNewUris(false);
+			onNewUris(false, newUris);
 		}
 	}
 
-	private void onNewUris(boolean restart) {
-		if (imageUris.isEmpty()) return;
+	private void onNewUris(boolean restart, List<Uri> newUris) {
+		if (newUris.isEmpty()) return;
 		if (loadingUris) throw new AssertionError();
 		loadingUris = true;
-		List<Uri> filtered = attachmentListener.filterAttachmentUris(imageUris);
-		imageUris.clear();
-		imageUris.addAll(filtered);
+		if (newUris.size() > MAX_ATTACHMENTS_PER_MESSAGE) {
+			newUris = newUris.subList(0, MAX_ATTACHMENTS_PER_MESSAGE);
+			attachmentListener.onTooManyAttachments();
+		}
+		imageUris.addAll(newUris);
 		updateViewState();
 		textInput.setHint(R.string.image_caption_hint);
 		List<ImagePreviewItem> items = ImagePreviewItem.fromUris(imageUris);
@@ -244,8 +248,7 @@ public class TextAttachmentController extends TextSendController
 	public Parcelable onRestoreInstanceState(Parcelable inState) {
 		SavedState state = (SavedState) inState;
 		if (!imageUris.isEmpty()) throw new AssertionError();
-		if (state.imageUris != null) imageUris.addAll(state.imageUris);
-		onNewUris(true);
+		if (state.imageUris != null) onNewUris(true, state.imageUris);
 		return state.getSuperState();
 	}
 
@@ -325,7 +328,6 @@ public class TextAttachmentController extends TextSendController
 
 		void onAttachImage(Intent intent);
 
-		List<Uri> filterAttachmentUris(List<Uri> uris);
+		void onTooManyAttachments();
 	}
-
 }

[Torsten Grote]

Looks like we are not clearing the imageUris anymore. Are we keep adding new Uris to them?

[Torsten Grote]

@akwizgran looks like this comment isn't attached properly to the MR!?

[akwizgran]

imageUris is still cleared in reset() so we shouldn't accumulate URIs as far as I can see. Previously it was populated before calling this method, then we had to clear and repopulate it with the filtered list. Now we're passing in the list, filtering it, then populating imageUris, which removes the need to clear it.

@akwizgran looks like this comment isn't attached properly to the MR!?

Ah, the sweet familiar taste of GitLab. :-)

[Torsten Grote]

So even when rotating the screen, there's no issue?

[akwizgran]

Is there a specific case you want me to test? It looks to me like there should be no behaviour change.

In the old code we had to clear and repopulate imageUris because we couldn't assign the filtered list to imageUris, which is final. In the new code we delay populating imageUris until after filtering, so that we don't have to populate, clear and repopulate.

Previously:

• We checked that imageUris was empty in onImageReceived() or onRestoreInstanceState()
• URIs were added to imageUris in onImageReceived() or onRestoreInstanceState()
• onNewUris() was called from onImageReceived() or onRestoreInstanceState()
• At this point, imageUris contained the unfiltered list
• imageUris was passed to the filter
• imageUris was cleared and repopulated with the filtered list
• At this point, imageUris contained the filtered list

Now:

• We check that imageUris is empty in onImageReceived() or onRestoreInstanceState()
• onNewUris() is called from onImageReceived() or onRestoreInstanceState() with a list of URIs
• At this point, imageUris is empty and newUris contains the unfiltered list
• newUris is filtered
• imageUris is populated with the filtered list
• At this point, imageUris contains the filtered list

[Torsten Grote]

Right thanks for explaining! So when rotating the screen, the activity should get destroyed and hopefully its object and references as well (as opposed to just recreating the same instance), so the imageUris should be empty, so we only need to add to it in onNewUris().

[akwizgran]

I think so - and if we're wrong, the bug also exists on master.

[akwizgran]

Since we can't resolve this dangling discussion, please merge if/when everything looks OK. :-)