From 098983f40193ddb352539f528353284e041e17a6 Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Tue, 12 Dec 2023 16:53:09 +0100 Subject: [PATCH] fix: Calculate length of posts and polls with emojis correctly (#315) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mastodon counts post lengths by considering emojis to be single characters, no matter how many unicode code points they are composed of. So "๐Ÿ˜œ" has length 1. Pachli was using `String.length`, which considers "๐Ÿ˜œ" as length 2. Correct the calculation by using a BreakIterator to count the characters in the string, which treats multi-character emojis as a length 1. Poll options had a similar problem, exacerbated by the Mastodon web UI also having the same problem, see https://github.com/mastodon/mastodon/issues/28336. Fix that by creating `MastodonLengthFilter`, an `InputFilter` that does the right thing for regular text that may contain emojis. See also https://github.com/tuskyapp/Tusky/pull/4152, which has the fix for status length but not polls. --------- Co-authored-by: Konrad Pozniak --- app/lint-baseline.xml | 82 ++++----- .../components/compose/ComposeActivity.kt | 45 ++++- .../compose/dialog/AddPollOptionsAdapter.kt | 4 +- app/src/test/java/app/pachli/SpanUtilsTest.kt | 53 +----- .../components/compose/ComposeActivityTest.kt | 43 +++++ .../compose/MastodonLengthFilterTest.kt | 169 ++++++++++++++++++ .../components/compose/StatusLengthTest.kt | 8 +- .../pachli/core/common/string/StringUtils.kt | 15 ++ core/testing/lint-baseline.xml | 13 +- .../core/testing/fakes/FakeSpannable.kt | 71 ++++++++ 10 files changed, 399 insertions(+), 104 deletions(-) create mode 100644 app/src/test/java/app/pachli/components/compose/MastodonLengthFilterTest.kt create mode 100644 core/testing/src/main/kotlin/app/pachli/core/testing/fakes/FakeSpannable.kt diff --git a/app/lint-baseline.xml b/app/lint-baseline.xml index d7d98cce9..46b6ef2b0 100644 --- a/app/lint-baseline.xml +++ b/app/lint-baseline.xml @@ -872,7 +872,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"> @@ -2082,7 +2082,7 @@ errorLine2=" ~~~~~~~"> @@ -2093,7 +2093,7 @@ errorLine2=" ~~~~~~~"> @@ -2104,7 +2104,7 @@ errorLine2=" ~~~~~~~"> @@ -2115,7 +2115,7 @@ errorLine2=" ~~~~~~~"> @@ -2126,7 +2126,7 @@ errorLine2=" ~~~~~~~"> @@ -2137,7 +2137,7 @@ errorLine2=" ~~~~~~~"> @@ -2159,7 +2159,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2170,7 +2170,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -2324,7 +2324,7 @@ errorLine2=" ~~~~~~~~~"> @@ -3138,7 +3138,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~"> @@ -3149,7 +3149,7 @@ errorLine2=" ~~~~~~~"> @@ -3158,20 +3158,20 @@ message="Access to `private` method `secondaryDrawerItem` of class `MainActivityKt` requires synthetic accessor" errorLine1=" secondaryDrawerItem {" errorLine2=" ~~~~~~~~~~~~~~~~~~~"> + + + + - - - - @@ -3182,7 +3182,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~"> @@ -3193,7 +3193,7 @@ errorLine2=" ~~~~~~~"> @@ -3204,7 +3204,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -3215,7 +3215,7 @@ errorLine2=" ~~~~~~~"> @@ -3226,7 +3226,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~"> @@ -3237,7 +3237,7 @@ errorLine2=" ~~~~~~~"> @@ -3248,7 +3248,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~"> @@ -3259,7 +3259,7 @@ errorLine2=" ~~~~~~~"> @@ -3270,7 +3270,7 @@ errorLine2=" ~~~~~~~"> @@ -3281,7 +3281,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~"> @@ -3292,7 +3292,7 @@ errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~~~"> @@ -3904,7 +3904,7 @@ errorLine2=" ~~~~~~~~"> @@ -3915,7 +3915,7 @@ errorLine2=" ~~~~~~~~"> @@ -3926,7 +3926,7 @@ errorLine2=" ~~~~~~~~"> @@ -3937,7 +3937,7 @@ errorLine2=" ~~~~~~~~"> @@ -3948,7 +3948,7 @@ errorLine2=" ~~~~~~~~"> @@ -3959,7 +3959,7 @@ errorLine2=" ~~~~~~~~"> @@ -3970,7 +3970,7 @@ errorLine2=" ~~~~~~~~"> diff --git a/app/src/main/java/app/pachli/components/compose/ComposeActivity.kt b/app/src/main/java/app/pachli/components/compose/ComposeActivity.kt index 558cf36f5..dd7881001 100644 --- a/app/src/main/java/app/pachli/components/compose/ComposeActivity.kt +++ b/app/src/main/java/app/pachli/components/compose/ComposeActivity.kt @@ -28,6 +28,7 @@ import android.net.Uri import android.os.Build import android.os.Bundle import android.provider.MediaStore +import android.text.InputFilter import android.text.Spanned import android.text.style.URLSpan import android.view.KeyEvent @@ -74,6 +75,7 @@ import app.pachli.components.compose.dialog.showAddPollDialog import app.pachli.components.compose.view.ComposeOptionsListener import app.pachli.components.compose.view.ComposeScheduleView import app.pachli.components.instanceinfo.InstanceInfoRepository +import app.pachli.core.common.string.mastodonLength import app.pachli.core.database.model.AccountEntity import app.pachli.core.navigation.ComposeActivityIntent import app.pachli.core.navigation.ComposeActivityIntent.ComposeOptions @@ -1323,6 +1325,7 @@ class ComposeActivity : * (https://docs.joinmastodon.org/user/posting/#mentions) * - Hashtags are always treated as their actual length, including the "#" * (https://docs.joinmastodon.org/user/posting/#hashtags) + * - Emojis are treated as a single character * * Content warning text is always treated as its full length, URLs and other entities * are not treated differently. @@ -1332,9 +1335,8 @@ class ComposeActivity : * @param urlLength the number of characters attributed to URLs * @return the effective status length */ - @JvmStatic fun statusLength(body: Spanned, contentWarning: Spanned?, urlLength: Int): Int { - var length = body.length - body.getSpans(0, body.length, URLSpan::class.java) + var length = body.toString().mastodonLength() - body.getSpans(0, body.length, URLSpan::class.java) .fold(0) { acc, span -> // Accumulate a count of characters to be *ignored* in the final length acc + when (span) { @@ -1347,15 +1349,50 @@ class ComposeActivity : } else -> { // Expected to be negative if the URL length < maxUrlLength - span.url.length - urlLength + span.url.mastodonLength() - urlLength } } } // Content warning text is treated as is, URLs or mentions there are not special - contentWarning?.let { length += it.length } + contentWarning?.let { length += it.toString().mastodonLength() } return length } + + /** + * [InputFilter] that uses the "Mastodon" length of a string, where emojis always + * count as a single character. + * + * Unlike [InputFilter.LengthFilter] the source input is not trimmed if it is + * too long, it's just rejected. + */ + class MastodonLengthFilter(private val maxLength: Int) : InputFilter { + override fun filter( + source: CharSequence?, + start: Int, + end: Int, + dest: Spanned?, + dstart: Int, + dend: Int, + ): CharSequence? { + val destRemovedLength = dest?.subSequence(dstart, dend).toString().mastodonLength() + val available = maxLength - dest.toString().mastodonLength() + destRemovedLength + val sourceLength = source?.subSequence(start, end).toString().mastodonLength() + + // Not enough space to insert the new text + if (sourceLength > available) return REJECT + + return ALLOW + } + + companion object { + /** Filter result allowing the changes */ + val ALLOW = null + + /** Filter result preventing the changes */ + const val REJECT = "" + } + } } } diff --git a/app/src/main/java/app/pachli/components/compose/dialog/AddPollOptionsAdapter.kt b/app/src/main/java/app/pachli/components/compose/dialog/AddPollOptionsAdapter.kt index cbf90d671..08294a73b 100644 --- a/app/src/main/java/app/pachli/components/compose/dialog/AddPollOptionsAdapter.kt +++ b/app/src/main/java/app/pachli/components/compose/dialog/AddPollOptionsAdapter.kt @@ -16,13 +16,13 @@ package app.pachli.components.compose.dialog -import android.text.InputFilter import android.view.LayoutInflater import android.view.View import android.view.ViewGroup import androidx.core.widget.doOnTextChanged import androidx.recyclerview.widget.RecyclerView import app.pachli.R +import app.pachli.components.compose.ComposeActivity.Companion.MastodonLengthFilter import app.pachli.databinding.ItemAddPollOptionBinding import app.pachli.util.BindingHolder import app.pachli.util.visible @@ -45,7 +45,7 @@ class AddPollOptionsAdapter( override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): BindingHolder { val binding = ItemAddPollOptionBinding.inflate(LayoutInflater.from(parent.context), parent, false) val holder = BindingHolder(binding) - binding.optionEditText.filters = arrayOf(InputFilter.LengthFilter(maxOptionLength)) + binding.optionEditText.filters = arrayOf(MastodonLengthFilter(maxOptionLength)) binding.optionEditText.doOnTextChanged { s, _, _, _ -> val pos = holder.bindingAdapterPosition diff --git a/app/src/test/java/app/pachli/SpanUtilsTest.kt b/app/src/test/java/app/pachli/SpanUtilsTest.kt index a605bea65..0888e4f80 100644 --- a/app/src/test/java/app/pachli/SpanUtilsTest.kt +++ b/app/src/test/java/app/pachli/SpanUtilsTest.kt @@ -1,6 +1,6 @@ package app.pachli -import android.text.Spannable +import app.pachli.core.testing.fakes.FakeSpannable import app.pachli.util.highlightSpans import org.junit.Assert import org.junit.Test @@ -127,55 +127,4 @@ class SpanUtilsTest { Assert.assertEquals(expectedEndIndex, span.end) } } - - class FakeSpannable(private val text: String) : Spannable { - val spans = mutableListOf() - - override fun setSpan(what: Any?, start: Int, end: Int, flags: Int) { - spans.add(BoundedSpan(what, start, end)) - } - - override fun getSpans(start: Int, end: Int, type: Class): Array { - return spans.filter { it.start >= start && it.end <= end && type.isInstance(it.span) } - .map { it.span } - .toTypedArray() as Array - } - - override fun removeSpan(what: Any?) { - spans.removeIf { span -> span.span == what } - } - - override fun toString(): String { - return text - } - - override val length: Int - get() = text.length - - class BoundedSpan(val span: Any?, val start: Int, val end: Int) - - override fun nextSpanTransition(start: Int, limit: Int, type: Class<*>?): Int { - throw NotImplementedError() - } - - override fun getSpanEnd(tag: Any?): Int { - throw NotImplementedError() - } - - override fun getSpanFlags(tag: Any?): Int { - throw NotImplementedError() - } - - override fun get(index: Int): Char { - throw NotImplementedError() - } - - override fun subSequence(startIndex: Int, endIndex: Int): CharSequence { - throw NotImplementedError() - } - - override fun getSpanStart(tag: Any?): Int { - throw NotImplementedError() - } - } } diff --git a/app/src/test/java/app/pachli/components/compose/ComposeActivityTest.kt b/app/src/test/java/app/pachli/components/compose/ComposeActivityTest.kt index 6763d3bbc..f485e7831 100644 --- a/app/src/test/java/app/pachli/components/compose/ComposeActivityTest.kt +++ b/app/src/test/java/app/pachli/components/compose/ComposeActivityTest.kt @@ -238,6 +238,49 @@ class ComposeActivityTest { } } + @Test + fun whenTextContainsEmoji_emojisAreCountedAsOneCharacter() { + val content = "Test ๐Ÿ˜œ" + rule.launch() + rule.getScenario().onActivity { + insertSomeTextInContent(it, content) + assertEquals(6, it.calculateTextLength()) + } + } + + @Test + fun whenTextContainsConesecutiveEmoji_emojisAreCountedAsSeparateCharacters() { + val content = "Test ๐Ÿ˜œ๐Ÿ˜œ" + rule.launch() + rule.getScenario().onActivity { + insertSomeTextInContent(it, content) + assertEquals(7, it.calculateTextLength()) + } + } + + @Test + fun whenTextContainsUrlWithEmoji_ellipsizedUrlIsCountedCorrectly() { + val content = "https://๐Ÿคช.com" + rule.launch() + rule.getScenario().onActivity { + insertSomeTextInContent(it, content) + assertEquals( + InstanceInfoRepository.DEFAULT_CHARACTERS_RESERVED_PER_URL, + it.calculateTextLength(), + ) + } + } + + @Test + fun whenTextContainsNonEnglishCharacters_lengthIsCountedCorrectly() { + val content = "ใ“ใ‚“ใซใกใฏ. General Kenobi" // "Hello there. General Kenobi" + rule.launch() + rule.getScenario().onActivity { + insertSomeTextInContent(it, content) + assertEquals(21, it.calculateTextLength()) + } + } + @Test fun whenTextContainsUrl_onlyEllipsizedURLIsCounted() { val url = "https://www.google.dk/search?biw=1920&bih=990&tbm=isch&sa=1&ei=bmDrWuOoKMv6kwWOkIaoDQ&q=indiana+jones+i+hate+snakes+animated&oq=indiana+jones+i+hate+snakes+animated&gs_l=psy-ab.3...54174.55443.0.55553.9.7.0.0.0.0.255.333.1j0j1.2.0....0...1c.1.64.psy-ab..7.0.0....0.40G-kcDkC6A#imgdii=PSp15hQjN1JqvM:&imgrc=H0hyE2JW5wrpBM:" diff --git a/app/src/test/java/app/pachli/components/compose/MastodonLengthFilterTest.kt b/app/src/test/java/app/pachli/components/compose/MastodonLengthFilterTest.kt new file mode 100644 index 000000000..5020d40df --- /dev/null +++ b/app/src/test/java/app/pachli/components/compose/MastodonLengthFilterTest.kt @@ -0,0 +1,169 @@ +/* + * Copyright 2023 Pachli Association + * + * This file is a part of Pachli. + * + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 3 of the + * License, or (at your option) any later version. + * + * Pachli is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even + * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General + * Public License for more details. + * + * You should have received a copy of the GNU General Public License along with Pachli; if not, + * see . + */ + +package app.pachli.components.compose + +import app.pachli.components.compose.ComposeActivity.Companion.MastodonLengthFilter +import app.pachli.components.compose.ComposeActivity.Companion.MastodonLengthFilter.Companion.ALLOW +import app.pachli.components.compose.ComposeActivity.Companion.MastodonLengthFilter.Companion.REJECT +import app.pachli.core.testing.fakes.FakeSpannable +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.Parameterized +import org.junit.runners.Parameterized.Parameters + +/** + * Parameters to pass to [MastodonLengthFilter.filter] and the expected result. + */ +data class TestData( + val source: CharSequence?, + val start: Int, + val end: Int, + val dest: String, + val dstart: Int, + val dend: Int, + /** The expected result */ + val want: String?, +) + +@RunWith(Parameterized::class) +class MastodonLengthFilterTest(private val testData: TestData) { + companion object { + @Parameters(name = "{0}") + @JvmStatic + fun data(): Iterable { + return listOf( + // Inserting 9 characters to empty string + TestData( + "some text", + 0, + 9, + "", + 0, + 0, + ALLOW, + ), + // Appending 1 character to length 5 string + TestData( + "6", + 0, + 1, + "12345", + 0, + 5, + ALLOW, + ), + // Replacing 1 character in middle of length 5 string + TestData( + "x", + 0, + 1, + "12345", + 2, + 3, + ALLOW, + ), + // Replacing all characters in length 5 string + TestData( + "xxxxx", + 0, + 4, + "12345", + 0, + 4, + ALLOW, + ), + // Deleting characters with a zero-length replacement + TestData( + "", + 0, + 0, + "12345", + 0, + 3, + ALLOW, + ), + // Appending a 2 character emoji to a 9 character string + TestData( + "๐Ÿ˜œ", + 0, + 2, + "123456789", + 9, + 9, + ALLOW, + ), + // Extending a 10 character string by 1 + TestData( + "x", + 0, + 1, + "1234567890", + 10, + 10, + REJECT, + ), + // Extending a 9 character string by 2 + TestData( + "xx", + 0, + 2, + "123456789", + 9, + 9, + REJECT, + ), + // Replacing 2 characters in a 10 character string with 3 + TestData( + "xxx", + 0, + 3, + "1234567890", + 5, + 7, + REJECT, + ), + // Appending 2 x 2 character emojis to a 9 character string + TestData( + "๐Ÿ˜œ๐Ÿ˜œ", + 0, + 3, + "123456789", + 9, + 9, + REJECT, + ), + ) + } + } + + @Test + fun filter_matchesExpectations() { + val filter = MastodonLengthFilter(10) + assertThat( + filter.filter( + testData.source, + testData.start, + testData.end, + FakeSpannable(testData.dest), + testData.dstart, + testData.dend, + ), + ).isEqualTo(testData.want) + } +} diff --git a/app/src/test/java/app/pachli/components/compose/StatusLengthTest.kt b/app/src/test/java/app/pachli/components/compose/StatusLengthTest.kt index 44f4c2681..4f39f8bbf 100644 --- a/app/src/test/java/app/pachli/components/compose/StatusLengthTest.kt +++ b/app/src/test/java/app/pachli/components/compose/StatusLengthTest.kt @@ -17,7 +17,7 @@ package app.pachli.components.compose -import app.pachli.SpanUtilsTest +import app.pachli.core.testing.fakes.FakeSpannable import app.pachli.util.highlightSpans import org.junit.Assert.assertEquals import org.junit.Test @@ -53,7 +53,7 @@ class StatusLengthTest( @Test fun statusLength_matchesExpectations() { - val spannedText = SpanUtilsTest.FakeSpannable(text) + val spannedText = FakeSpannable(text) highlightSpans(spannedText, 0) assertEquals( @@ -64,10 +64,10 @@ class StatusLengthTest( @Test fun statusLength_withCwText_matchesExpectations() { - val spannedText = SpanUtilsTest.FakeSpannable(text) + val spannedText = FakeSpannable(text) highlightSpans(spannedText, 0) - val cwText = SpanUtilsTest.FakeSpannable( + val cwText = FakeSpannable( "a @example@example.org #hashtagmention and http://example.org URL", ) assertEquals( diff --git a/core/common/src/main/kotlin/app/pachli/core/common/string/StringUtils.kt b/core/common/src/main/kotlin/app/pachli/core/common/string/StringUtils.kt index dca836ad6..e067974a7 100644 --- a/core/common/src/main/kotlin/app/pachli/core/common/string/StringUtils.kt +++ b/core/common/src/main/kotlin/app/pachli/core/common/string/StringUtils.kt @@ -1,6 +1,7 @@ package app.pachli.core.common.string import android.text.Spanned +import java.text.BreakIterator import java.util.Random private const val POSSIBLE_CHARS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ" @@ -44,6 +45,20 @@ fun String.isLessThanOrEqual(other: String): Boolean { return this == other || isLessThan(other) } +/** + * @return the "Mastodon" length of a string. [String.length] counts emojis as + * multiple characters, but Mastodon treats them as a single character. + */ +fun String.mastodonLength(): Int { + val breakIterator = BreakIterator.getCharacterInstance() + breakIterator.setText(this) + var count = 0 + while (breakIterator.next() != BreakIterator.DONE) { + count++ + } + return count +} + fun Spanned.trimTrailingWhitespace(): Spanned { var i = length do { diff --git a/core/testing/lint-baseline.xml b/core/testing/lint-baseline.xml index a32ee8243..a2ca26aaa 100644 --- a/core/testing/lint-baseline.xml +++ b/core/testing/lint-baseline.xml @@ -1,5 +1,5 @@ - + + + + + . + */ + +package app.pachli.core.testing.fakes + +import android.text.Spannable + +class FakeSpannable(private val text: String) : Spannable { + val spans = mutableListOf() + + override fun setSpan(what: Any?, start: Int, end: Int, flags: Int) { + spans.add(BoundedSpan(what, start, end)) + } + + override fun getSpans(start: Int, end: Int, type: Class): Array { + return spans.filter { it.start >= start && it.end <= end && type.isInstance(it.span) } + .map { it.span } + .toTypedArray() as Array + } + + override fun removeSpan(what: Any?) { + spans.removeIf { span -> span.span == what } + } + + override fun toString(): String { + return text + } + + override val length: Int + get() = text.length + + class BoundedSpan(val span: Any?, val start: Int, val end: Int) + + override fun nextSpanTransition(start: Int, limit: Int, type: Class<*>?): Int { + throw NotImplementedError() + } + + override fun getSpanEnd(tag: Any?): Int { + throw NotImplementedError() + } + + override fun getSpanFlags(tag: Any?): Int { + throw NotImplementedError() + } + + override fun get(index: Int): Char { + throw NotImplementedError() + } + + override fun subSequence(startIndex: Int, endIndex: Int): CharSequence { + return text.subSequence(startIndex, endIndex) + } + + override fun getSpanStart(tag: Any?): Int { + throw NotImplementedError() + } +}