fix: pagers leak listeners on wide-screen devices

Phillip Davis created 1 month ago

On wide screens, Android re-uses the ViewPager + TabView
in between conversations, which means `Conversation.onDestroyView`
doesn't get called, which results in `mCurrentTab` getting set for
previously opened chats. This manifested as [0]

[0]: https://todo.sr.ht/~singpolyma/soprani.ca/554

Change summary

src/main/java/eu/siacs/conversations/entities/Conversation.java     |  1 
src/test/java/eu/siacs/conversations/entities/ConversationTest.java | 85 
2 files changed, 86 insertions(+)

Detailed changes

src/main/java/eu/siacs/conversations/entities/Conversation.java

@@ -1856,6 +1856,7 @@ public class Conversation extends AbstractEntity
             }
             pager.removeView(page1.get());
             pager.removeView(page2.get());
+            pager.clearOnPageChangeListeners();
             pager.setAdapter(this);
             tabs.setupWithViewPager(pager);
             pager.post(() -> pager.setCurrentItem(getCurrentTab()));

src/test/java/eu/siacs/conversations/entities/ConversationTest.java

@@ -1,5 +1,7 @@
 package eu.siacs.conversations.entities;
 
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -10,11 +12,20 @@ import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.robolectric.RobolectricTestRunner;
+import org.robolectric.RuntimeEnvironment;
+import org.robolectric.Shadows;
 import org.robolectric.annotation.Config;
 import org.robolectric.annotation.ConscryptMode;
 
 import android.os.Build;
+import android.os.Looper;
+import android.view.View;
+import android.widget.ListView;
+import android.widget.RelativeLayout;
+import androidx.viewpager.widget.ViewPager;
+import com.google.android.material.tabs.TabLayout;
 import eu.siacs.conversations.Conversations;
+import eu.siacs.conversations.R;
 import eu.siacs.conversations.xml.Namespace;
 import eu.siacs.conversations.xmpp.Jid;
 import im.conversations.android.xmpp.model.disco.info.Feature;
@@ -88,4 +99,78 @@ public class ConversationTest {
         var cache = withoutOccupantId.getMucOccupantCache();
         Assert.assertNotNull("Should return cache even when occupant-id is not supported", cache);
     }
+
+    @Test
+    public void setupViewPagerListenerDoesNotLeakBetweenConversations() {
+        final var context = RuntimeEnvironment.getApplication();
+        final var pager = new ViewPager(context);
+        final var tabs = mock(TabLayout.class);
+
+        final int[] selectedTabPosition = {0};
+        doAnswer(invocation -> {
+            ViewPager vp = invocation.getArgument(0);
+            vp.addOnPageChangeListener(new ViewPager.OnPageChangeListener() {
+                public void onPageScrollStateChanged(int state) {}
+                public void onPageScrolled(int p, float o, int opx) {}
+                public void onPageSelected(int position) {
+                    selectedTabPosition[0] = position;
+                }
+            });
+            return null;
+        }).when(tabs).setupWithViewPager(any(ViewPager.class));
+        when(tabs.getSelectedTabPosition()).thenAnswer(inv -> selectedTabPosition[0]);
+
+        final var page1 = new RelativeLayout(context);
+        final var page2 = new RelativeLayout(context);
+        final var commandsView = new ListView(context);
+        commandsView.setId(R.id.commands_view);
+        page2.addView(commandsView);
+        pager.addView(page1);
+        pager.addView(page2);
+
+        final var account = mock(Account.class);
+        when(account.getJid()).thenReturn(Jid.ofLocalAndDomain("testAccount", "example.org"));
+        final var roster = mock(Roster.class);
+        when(account.getRoster()).thenReturn(roster);
+
+        final var mucJid = Jid.ofLocalAndDomain("operations", "conference.soprani.ca");
+        final var mucContact = mock(Contact.class);
+        when(mucContact.isApp()).thenReturn(false);
+        when(mucContact.getJid()).thenReturn(mucJid);
+        when(roster.getContact(mucJid)).thenReturn(mucContact);
+
+        final var appJid = Jid.ofDomain("jmp.chat");
+        final var appContact = mock(Contact.class);
+        when(appContact.isApp()).thenReturn(true);
+        when(appContact.getJid()).thenReturn(appJid);
+        when(roster.getContact(appJid)).thenReturn(appContact);
+
+        final var muc = new Conversation("MUC", account, mucJid, Conversation.MODE_MULTI);
+        final var app = new Conversation("App", account, appJid, Conversation.MODE_SINGLE);
+
+        muc.setupViewPager(pager, tabs, false, null);
+        muc.showViewPager();
+
+        pager.layout(0, 0, 1024, 768);
+        Shadows.shadowOf(Looper.getMainLooper()).idle();
+
+        Assert.assertEquals("MUC should start on Conversation tab", 0, muc.getCurrentTab());
+
+        app.setupViewPager(pager, tabs, false, muc);
+        app.showViewPager();
+
+        Shadows.shadowOf(Looper.getMainLooper()).idle();
+
+        pager.setCurrentItem(1);
+
+        Assert.assertEquals(
+            "Page change on app conversation must not corrupt MUC's tab state",
+            0,
+            muc.getCurrentTab());
+
+        Assert.assertEquals(
+            "Tab indicator must stay in sync with the selected page",
+            1,
+            tabs.getSelectedTabPosition());
+    }
 }