From ab360e1e250988445d11c714d53be4e74ba94f82 Mon Sep 17 00:00:00 2001
From: akwizgran <michael@briarproject.org>
Date: Tue, 16 Aug 2022 13:36:15 +0100
Subject: [PATCH] Address some review comments.

---
 .../bramble/mailbox/MailboxClientManager.java       |  8 ++++----
 .../bramble/mailbox/MailboxClientManagerTest.java   | 13 ++++++-------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClientManager.java b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClientManager.java
index 3fa1c7046..7057f0cac 100644
--- a/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClientManager.java
+++ b/bramble-core/src/main/java/org/briarproject/bramble/mailbox/MailboxClientManager.java
@@ -95,7 +95,8 @@ class MailboxClientManager implements Service, EventListener {
 	private final TorReachabilityMonitor reachabilityMonitor;
 	private final AtomicBoolean used = new AtomicBoolean(false);
 
-	// The following mutable state must only be accessed on the event thread
+	// All of the following mutable state must only be accessed on the
+	// event thread
 	private final Map<ContactId, Updates> contactUpdates = new HashMap<>();
 	private final Map<ContactId, MailboxClient> contactClients =
 			new HashMap<>();
@@ -343,13 +344,12 @@ class MailboxClientManager implements Service, EventListener {
 				MailboxClient contactClient =
 						requireNonNull(contactClients.get(c));
 				contactClient.deassignContactForDownload(c);
-				assignContactToOwnMailboxForDownload(c, u);
 			} else {
 				// The contact doesn't have a usable mailbox, so assign the
-				// contact to our mailbox for upload and download
+				// contact to our mailbox for upload
 				assignContactToOwnMailboxForUpload(c, u);
-				assignContactToOwnMailboxForDownload(c, u);
 			}
+			assignContactToOwnMailboxForDownload(c, u);
 		}
 	}
 
diff --git a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxClientManagerTest.java b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxClientManagerTest.java
index 655f6952a..c658a6d76 100644
--- a/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxClientManagerTest.java
+++ b/bramble-core/src/test/java/org/briarproject/bramble/mailbox/MailboxClientManagerTest.java
@@ -281,12 +281,11 @@ public class MailboxClientManagerTest extends BrambleMockTestCase {
 		manager.eventOccurred(new TransportActiveEvent(ID));
 
 		// When we go offline, the manager should destroy the client for our
-		// own mailbox
+		// own mailbox.
 		expectDestroyClientForOwnMailbox();
 		manager.eventOccurred(new TransportInactiveEvent(ID));
 
-		// At shutdown the manager should destroy the client for our own
-		// mailbox and the reachability monitor.
+		// At shutdown the manager should destroy the reachability monitor.
 		expectRunTaskOnEventExecutor();
 		expectDestroyReachabilityMonitor();
 		manager.stopService();
@@ -322,8 +321,8 @@ public class MailboxClientManagerTest extends BrambleMockTestCase {
 	@Test
 	public void testReassignsContactToOurMailboxWhenPaired() throws Exception {
 		// At startup the manager should load the local and remote updates
-		// and our own mailbox properties. We have a mailbox but the contact
-		// doesn't.
+		// and our own mailbox properties. The contact has a mailbox but we
+		// don't.
 		//
 		// We're online, so the manager should create a client for the
 		// contact's mailbox and assign the contact to it for upload and
@@ -358,8 +357,8 @@ public class MailboxClientManagerTest extends BrambleMockTestCase {
 	public void testDoesNotAssignContactWhenPairedIfContactHasNotSentUpdate()
 			throws Exception {
 		// At startup the manager should load the local and remote updates
-		// and our own mailbox properties. We have a mailbox but the contact
-		// has never sent us an update, so the remote update is null.
+		// and our own mailbox properties. We don't have a mailbox and the
+		// contact has never sent us an update, so the remote update is null.
 		expectLoadUpdates(localUpdateWithoutMailbox, null, null);
 		expectCheckPluginState(ACTIVE);
 		manager.startService();