Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compose: Enforce max topic/content length by code points, following API #1239

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

chrisbobbe
Copy link
Collaborator

Fixes #1238.

Done by computing String.runes (the number of Unicode code points) unless we know that the result can't exceed the threshold number of code points. In particular, we don't compute it unless String.length (the number of UTF-16 code units) exceeds the threshold number of code points.

We have the same comment where we check the content length:

  if (textNormalized.length > kMaxMessageLengthCodePoints)

and it applies here, too; the API doc on max_topic_length in
/register also says it's in Unicode code points:
  https://zulip.com/api/register-queue

And give our max-topic-length variable a more descriptive name,
again like we do with kMaxMessageLengthCodePoints.
With zulip#996, these tests will have to start checking for separate
per-platform flavors of alert dialog. Best if they all do so through
this one codepath.
Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I read all the commits and it looks good to me overall. Left some comments.

@@ -95,6 +95,23 @@ void main() {
..url.path.equals('/api/v1/users/me/${narrow.streamId}/topics');
}

/// Set the content input's text to [content], using [WidgetTester.enterText].
Future<void> enterContent(WidgetTester tester, {
required ChannelNarrow narrow,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is unused. For entering content, just having await tester.enterText(contentInputFinder, content); should be fine.

/// is more expensive than getting the number of UTF-16 code units
/// ([String.length]), so we avoid it when the result definitely won't exceed
/// [maxLengthUnicodeCodePoints].
int? get lengthUnicodeCodePointsIfLong => _lengthUnicodeCodePointsIfLong;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we maintain the length internally, and expose only a method that tests if the content is too long? We already have access to the length limit via maxLengthUnicodeCodePoints and hiding this cache might simplify the checks a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compose: Enforce max topic/content length by Unicode code points, not UTF-16 code units
2 participants