From 2655f2283956c459c8e7ee61cb79f484a7a64d3e Mon Sep 17 00:00:00 2001 From: Phillip Davis Date: Thu, 2 Apr 2026 16:48:03 -0400 Subject: [PATCH] fix: pagers leak listeners on wide-screen devices 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 --- .../conversations/entities/Conversation.java | 1 + .../entities/ConversationTest.java | 85 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/src/main/java/eu/siacs/conversations/entities/Conversation.java b/src/main/java/eu/siacs/conversations/entities/Conversation.java index 175c67be0208f3c1666553a14086403d458d4b33..4415977d934b495eec2179e6e132b6911a6921f8 100644 --- a/src/main/java/eu/siacs/conversations/entities/Conversation.java +++ b/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())); diff --git a/src/test/java/eu/siacs/conversations/entities/ConversationTest.java b/src/test/java/eu/siacs/conversations/entities/ConversationTest.java index fd33012115720a5068d6ced0d71a78317c2f262f..bb043246b6c491afec94c17a2d91e5a7357ac80c 100644 --- a/src/test/java/eu/siacs/conversations/entities/ConversationTest.java +++ b/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()); + } }