From 8f5fb5b35ca85993b75bd62f38bdfb27e467a552 Mon Sep 17 00:00:00 2001 From: Konrad Pozniak Date: Fri, 28 Jan 2022 07:44:38 +0100 Subject: [PATCH] Fix some weird behavior when clicking links in statuses (#2304) * Fix some weird behavior when clicking links in statuses * open browser when user clicks a status link again --- .../tusky/BottomSheetActivity.kt | 6 +- .../tusky/adapter/StatusBaseViewHolder.java | 11 +- .../adapter/StatusDetailedViewHolder.java | 2 +- .../tusky/fragment/ViewThreadFragment.java | 17 +++ .../tusky/BottomSheetActivityTest.kt | 131 +++++++++--------- 5 files changed, 97 insertions(+), 70 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/BottomSheetActivity.kt b/app/src/main/java/com/keylesspalace/tusky/BottomSheetActivity.kt index 2f0f38216..599322a0f 100644 --- a/app/src/main/java/com/keylesspalace/tusky/BottomSheetActivity.kt +++ b/app/src/main/java/com/keylesspalace/tusky/BottomSheetActivity.kt @@ -180,6 +180,8 @@ abstract class BottomSheetActivity : BaseActivity() { // https://friendica.foo.bar/profile/user // https://friendica.foo.bar/display/d4643c42-3ae0-4b73-b8b0-c725f5819207 // https://misskey.foo.bar/notes/83w6r388br (always lowercase) +// https://pixelfed.social/p/connyduck/391263492998670833 +// https://pixelfed.social/connyduck fun looksLikeMastodonUrl(urlString: String): Boolean { val uri: URI try { @@ -203,7 +205,9 @@ fun looksLikeMastodonUrl(urlString: String): Boolean { path.matches("^/objects/[-a-f0-9]+$".toRegex()) || path.matches("^/notes/[a-z0-9]+$".toRegex()) || path.matches("^/display/[-a-f0-9]+$".toRegex()) || - path.matches("^/profile/\\w+$".toRegex()) + path.matches("^/profile/\\w+$".toRegex()) || + path.matches("^/p/\\w+/\\d+$".toRegex()) || + path.matches("^/\\w+$".toRegex()) } enum class PostLookupFallbackBehavior { diff --git a/app/src/main/java/com/keylesspalace/tusky/adapter/StatusBaseViewHolder.java b/app/src/main/java/com/keylesspalace/tusky/adapter/StatusBaseViewHolder.java index 734f4e58f..f45667eb0 100644 --- a/app/src/main/java/com/keylesspalace/tusky/adapter/StatusBaseViewHolder.java +++ b/app/src/main/java/com/keylesspalace/tusky/adapter/StatusBaseViewHolder.java @@ -771,7 +771,7 @@ public abstract class StatusBaseViewHolder extends RecyclerView.ViewHolder { } if (cardView != null) { - setupCard(status, statusDisplayOptions.cardViewMode(), statusDisplayOptions); + setupCard(status, statusDisplayOptions.cardViewMode(), statusDisplayOptions, listener); } setupButtons(listener, actionable.getAccount().getId(), status.getContent().toString(), @@ -1034,7 +1034,12 @@ public abstract class StatusBaseViewHolder extends RecyclerView.ViewHolder { return pollDescription.getContext().getString(R.string.poll_info_format, votesText, pollDurationInfo); } - protected void setupCard(StatusViewData.Concrete status, CardViewMode cardViewMode, StatusDisplayOptions statusDisplayOptions) { + protected void setupCard( + StatusViewData.Concrete status, + CardViewMode cardViewMode, + StatusDisplayOptions statusDisplayOptions, + final StatusActionListener listener + ) { final Card card = status.getActionable().getCard(); if (cardViewMode != CardViewMode.NONE && status.getActionable().getAttachments().size() == 0 && @@ -1125,7 +1130,7 @@ public abstract class StatusBaseViewHolder extends RecyclerView.ViewHolder { cardImage.setImageResource(R.drawable.card_image_placeholder); } - View.OnClickListener visitLink = v -> LinkHelper.openLink(card.getUrl(), v.getContext()); + View.OnClickListener visitLink = v -> listener.onViewUrl(card.getUrl()); View.OnClickListener openImage = v -> cardView.getContext().startActivity(ViewMediaActivity.newSingleImageIntent(cardView.getContext(), card.getEmbed_url())); cardInfo.setOnClickListener(visitLink); diff --git a/app/src/main/java/com/keylesspalace/tusky/adapter/StatusDetailedViewHolder.java b/app/src/main/java/com/keylesspalace/tusky/adapter/StatusDetailedViewHolder.java index ef2c704d6..56adfcada 100644 --- a/app/src/main/java/com/keylesspalace/tusky/adapter/StatusDetailedViewHolder.java +++ b/app/src/main/java/com/keylesspalace/tusky/adapter/StatusDetailedViewHolder.java @@ -106,7 +106,7 @@ class StatusDetailedViewHolder extends StatusBaseViewHolder { StatusDisplayOptions statusDisplayOptions, @Nullable Object payloads) { super.setupWithStatus(status, listener, statusDisplayOptions, payloads); - setupCard(status, CardViewMode.FULL_WIDTH, statusDisplayOptions); // Always show card for detailed status + setupCard(status, CardViewMode.FULL_WIDTH, statusDisplayOptions, listener); // Always show card for detailed status if (payloads == null) { if (!statusDisplayOptions.hideStats()) { diff --git a/app/src/main/java/com/keylesspalace/tusky/fragment/ViewThreadFragment.java b/app/src/main/java/com/keylesspalace/tusky/fragment/ViewThreadFragment.java index bfbb18ed5..378fc68be 100644 --- a/app/src/main/java/com/keylesspalace/tusky/fragment/ViewThreadFragment.java +++ b/app/src/main/java/com/keylesspalace/tusky/fragment/ViewThreadFragment.java @@ -60,6 +60,7 @@ import com.keylesspalace.tusky.network.FilterModel; import com.keylesspalace.tusky.network.MastodonApi; import com.keylesspalace.tusky.settings.PrefKeys; import com.keylesspalace.tusky.util.CardViewMode; +import com.keylesspalace.tusky.util.LinkHelper; import com.keylesspalace.tusky.util.ListStatusAccessibilityDelegate; import com.keylesspalace.tusky.util.PairedList; import com.keylesspalace.tusky.util.StatusDisplayOptions; @@ -319,6 +320,22 @@ public final class ViewThreadFragment extends SFragment implements super.viewThread(status.getActionableId(), status.getActionableStatus().getUrl()); } + @Override + public void onViewUrl(String url) { + Status status = null; + if (!statuses.isEmpty()) { + status = statuses.get(statusIndex); + } + if (status != null && status.getUrl().equals(url)) { + // already viewing the status with this url + // probably just a preview federated and the user is clicking again to view more -> open the browser + // this can happen with some friendica statuses + LinkHelper.openLink(url, requireContext()); + return; + } + super.onViewUrl(url); + } + @Override public void onOpenReblog(int position) { // there should be no reblogs in the thread but let's implement it to be sure diff --git a/app/src/test/java/com/keylesspalace/tusky/BottomSheetActivityTest.kt b/app/src/test/java/com/keylesspalace/tusky/BottomSheetActivityTest.kt index 5c77de765..6d761b38b 100644 --- a/app/src/test/java/com/keylesspalace/tusky/BottomSheetActivityTest.kt +++ b/app/src/test/java/com/keylesspalace/tusky/BottomSheetActivityTest.kt @@ -23,22 +23,24 @@ import com.keylesspalace.tusky.entity.Account import com.keylesspalace.tusky.entity.SearchResult import com.keylesspalace.tusky.entity.Status import com.keylesspalace.tusky.network.MastodonApi +import com.nhaarman.mockitokotlin2.doReturn +import com.nhaarman.mockitokotlin2.mock import io.reactivex.rxjava3.android.plugins.RxAndroidPlugins import io.reactivex.rxjava3.core.Single import io.reactivex.rxjava3.plugins.RxJavaPlugins import io.reactivex.rxjava3.schedulers.TestScheduler -import org.junit.Assert +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.junit.runners.Parameterized -import org.mockito.ArgumentMatchers -import org.mockito.Mockito.`when` +import org.mockito.ArgumentMatchers.anyBoolean import org.mockito.Mockito.eq import org.mockito.Mockito.mock import java.util.ArrayList -import java.util.Collections import java.util.Date import java.util.concurrent.TimeUnit @@ -56,46 +58,42 @@ class BottomSheetActivityTest { private val testScheduler = TestScheduler() private val account = Account( - "1", - "admin", - "admin", - "Ad Min", - SpannedString(""), - "http://mastodon.foo.bar", - "", - "", - false, - 0, - 0, - 0, - null, - false, - emptyList(), - emptyList() + id = "1", + localUsername = "admin", + username = "admin", + displayName = "Ad Min", + note = SpannedString(""), + url = "http://mastodon.foo.bar", + avatar = "", + header = "", + locked = false, + followersCount = 0, + followingCount = 0, + statusesCount = 0 ) private val accountSingle = Single.just(SearchResult(listOf(account), emptyList(), emptyList())) private val status = Status( - "1", - statusQuery, - account, - null, - null, - null, - SpannedString("omgwat"), - Date(), - Collections.emptyList(), - 0, - 0, - false, - false, - false, - false, - "", - Status.Visibility.PUBLIC, - ArrayList(), - listOf(), - null, + id = "1", + url = statusQuery, + account = account, + inReplyToId = null, + inReplyToAccountId = null, + reblog = null, + content = SpannedString("omgwat"), + createdAt = Date(), + emojis = emptyList(), + reblogsCount = 0, + favouritesCount = 0, + reblogged = false, + favourited = false, + bookmarked = false, + sensitive = false, + spoilerText = "", + visibility = Status.Visibility.PUBLIC, + attachments = ArrayList(), + mentions = emptyList(), + application = null, pinned = false, muted = false, poll = null, @@ -109,10 +107,11 @@ class BottomSheetActivityTest { RxJavaPlugins.setIoSchedulerHandler { testScheduler } RxAndroidPlugins.setMainThreadSchedulerHandler { testScheduler } - apiMock = mock(MastodonApi::class.java) - `when`(apiMock.searchObservable(eq(accountQuery), eq(null), ArgumentMatchers.anyBoolean(), eq(null), eq(null), eq(null))).thenReturn(accountSingle) - `when`(apiMock.searchObservable(eq(statusQuery), eq(null), ArgumentMatchers.anyBoolean(), eq(null), eq(null), eq(null))).thenReturn(statusSingle) - `when`(apiMock.searchObservable(eq(nonMastodonQuery), eq(null), ArgumentMatchers.anyBoolean(), eq(null), eq(null), eq(null))).thenReturn(emptyCallback) + apiMock = mock { + on { searchObservable(eq(accountQuery), eq(null), anyBoolean(), eq(null), eq(null), eq(null)) } doReturn accountSingle + on { searchObservable(eq(statusQuery), eq(null), anyBoolean(), eq(null), eq(null), eq(null)) } doReturn statusSingle + on { searchObservable(eq(nonMastodonQuery), eq(null), anyBoolean(), eq(null), eq(null), eq(null)) } doReturn emptyCallback + } activity = FakeBottomSheetActivity(apiMock) } @@ -168,21 +167,23 @@ class BottomSheetActivityTest { arrayOf("https://friendica.foo.bar/profile/@mew/", false), arrayOf("https://misskey.foo.bar/notes/@nyan", false), arrayOf("https://misskey.foo.bar/notes/NYAN123", false), - arrayOf("https://misskey.foo.bar/notes/meow123/", false) + arrayOf("https://misskey.foo.bar/notes/meow123/", false), + arrayOf("https://pixelfed.social/p/connyduck/391263492998670833", true), + arrayOf("https://pixelfed.social/connyduck", true) ) } } @Test fun test() { - Assert.assertEquals(expectedResult, looksLikeMastodonUrl(url)) + assertEquals(expectedResult, looksLikeMastodonUrl(url)) } } @Test fun beginEndSearch_setIsSearching_isSearchingAfterBegin() { activity.onBeginSearch("https://mastodon.foo.bar/@User") - Assert.assertTrue(activity.isSearching()) + assertTrue(activity.isSearching()) } @Test @@ -190,7 +191,7 @@ class BottomSheetActivityTest { val validUrl = "https://mastodon.foo.bar/@User" activity.onBeginSearch(validUrl) activity.onEndSearch(validUrl) - Assert.assertFalse(activity.isSearching()) + assertFalse(activity.isSearching()) } @Test @@ -200,7 +201,7 @@ class BottomSheetActivityTest { activity.onBeginSearch(validUrl) activity.onEndSearch(invalidUrl) - Assert.assertTrue(activity.isSearching()) + assertTrue(activity.isSearching()) } @Test @@ -209,7 +210,7 @@ class BottomSheetActivityTest { activity.onBeginSearch(url) activity.cancelActiveSearch() - Assert.assertFalse(activity.isSearching()) + assertFalse(activity.isSearching()) } @Test @@ -221,29 +222,29 @@ class BottomSheetActivityTest { activity.cancelActiveSearch() activity.onBeginSearch(secondUrl) - Assert.assertTrue(activity.getCancelSearchRequested(firstUrl)) - Assert.assertFalse(activity.getCancelSearchRequested(secondUrl)) + assertTrue(activity.getCancelSearchRequested(firstUrl)) + assertFalse(activity.getCancelSearchRequested(secondUrl)) } @Test fun search_inIdealConditions_returnsRequestedResults_forAccount() { activity.viewUrl(accountQuery) testScheduler.advanceTimeBy(100, TimeUnit.MILLISECONDS) - Assert.assertEquals(account.id, activity.accountId) + assertEquals(account.id, activity.accountId) } @Test fun search_inIdealConditions_returnsRequestedResults_forStatus() { activity.viewUrl(statusQuery) testScheduler.advanceTimeBy(100, TimeUnit.MILLISECONDS) - Assert.assertEquals(status.id, activity.statusId) + assertEquals(status.id, activity.statusId) } @Test fun search_inIdealConditions_returnsRequestedResults_forNonMastodonURL() { activity.viewUrl(nonMastodonQuery) testScheduler.advanceTimeBy(100, TimeUnit.MILLISECONDS) - Assert.assertEquals(nonMastodonQuery, activity.link) + assertEquals(nonMastodonQuery, activity.link) } @Test @@ -251,32 +252,32 @@ class BottomSheetActivityTest { for (fallbackBehavior in listOf(PostLookupFallbackBehavior.OPEN_IN_BROWSER, PostLookupFallbackBehavior.DISPLAY_ERROR)) { activity.viewUrl(nonMastodonQuery, fallbackBehavior) testScheduler.advanceTimeBy(100, TimeUnit.MILLISECONDS) - Assert.assertEquals(nonMastodonQuery, activity.link) - Assert.assertEquals(fallbackBehavior, activity.fallbackBehavior) + assertEquals(nonMastodonQuery, activity.link) + assertEquals(fallbackBehavior, activity.fallbackBehavior) } } @Test fun search_withCancellation_doesNotLoadUrl_forAccount() { activity.viewUrl(accountQuery) - Assert.assertTrue(activity.isSearching()) + assertTrue(activity.isSearching()) activity.cancelActiveSearch() - Assert.assertFalse(activity.isSearching()) - Assert.assertEquals(null, activity.accountId) + assertFalse(activity.isSearching()) + assertEquals(null, activity.accountId) } @Test fun search_withCancellation_doesNotLoadUrl_forStatus() { activity.viewUrl(accountQuery) activity.cancelActiveSearch() - Assert.assertEquals(null, activity.accountId) + assertEquals(null, activity.accountId) } @Test fun search_withCancellation_doesNotLoadUrl_forNonMastodonURL() { activity.viewUrl(nonMastodonQuery) activity.cancelActiveSearch() - Assert.assertEquals(null, activity.searchUrl) + assertEquals(null, activity.searchUrl) } @Test @@ -289,15 +290,15 @@ class BottomSheetActivityTest { activity.viewUrl(statusQuery) // ensure that search is still ongoing - Assert.assertTrue(activity.isSearching()) + assertTrue(activity.isSearching()) // return searchResults testScheduler.advanceTimeBy(100, TimeUnit.MILLISECONDS) // ensure that the result of the status search was recorded // and the account search wasn't - Assert.assertEquals(status.id, activity.statusId) - Assert.assertEquals(null, activity.accountId) + assertEquals(status.id, activity.statusId) + assertEquals(null, activity.accountId) } class FakeBottomSheetActivity(api: MastodonApi) : BottomSheetActivity() {