compose: Use max_topic_length instead of hardcoded limit of 60
#1955
Conversation
lib/model/realm.dart
Outdated
| bool get realmEnableReadReceipts; | ||
| bool get realmMandatoryTopics; | ||
| int get maxFileUploadSizeMib; | ||
| int get maxTopicLength; |
There was a problem hiding this comment.
Searching for "max_topic_length" at https://zulip.com/api/get-events , I think this one isn't updated by the realm/update_dict event, and it belongs in the "Realm settings that lack events." section.
There was a problem hiding this comment.
Good catch. Probably good to mention that in the thread #issues > Resolve-unresolve can lose information @ 💬 — we might start sending an event if it turns into a real realm setting.
chrisbobbe
added
integration review
There was a problem hiding this comment.
Thanks @sm-sayedi for taking care of this, and @chrisbobbe for the previous review!
It turns out this is a very timely issue 🙂 — just today we decided we might want to actually change this value on the server side sometime soon: #issues > Resolve-unresolve can lose information @ 💬.
Small comments below, in addition to Chris's comment above.
test/widgets/compose_box_test.dart
Outdated
| realmMandatoryTopics: mandatoryTopics, | ||
| realmAllowMessageEditing: true, | ||
| realmMessageContentEditLimitSeconds: null, | ||
| maxTopicLength: maxTopicLength |
There was a problem hiding this comment.
nit:
| maxTopicLength: maxTopicLength | |
| maxTopicLength: maxTopicLength, |
test/widgets/compose_box_test.dart
Outdated
| expectedMessage: 'Topic length shouldn\'t be greater than $maxTopicLength ${maxTopicLength == 1 ? 'character' : 'characters'}.'))); | ||
| } | ||
|
|
||
| final variants = ValueVariant({30, 60, 90}); |
There was a problem hiding this comment.
I think it's not necessary to run each of these test cases in triplicate with different lengths.
Instead, pick just one value — but make it some number that isn't the historical default (60) and isn't a round number. E.g., 37.
(If we wanted the test to be a bit more paranoid, we could use two different values. But I'm content with just one, and counting on the implementation to not have a hard-coded value that just happens to match the odd value chosen in the test.)
test/widgets/compose_box_test.dart
Outdated
| await prepareWithTopic(tester, | ||
| makeStringWithCodePoints(kMaxTopicLengthCodePoints + 1)); | ||
| final maxTopicLength = variants.currentValue!; | ||
| await prepareWithTopic(tester, makeStringWithCodePoints(maxTopicLength + 1), | ||
| maxTopicLength: maxTopicLength); |
There was a problem hiding this comment.
Another consequence of that approach is that these test bodies can avoid getting any more complex than they were: just instead of saying kMaxTopicLengthCodePoints, they say maxTopicLength, where that's a local constant in this group body.
sm-sayedi
force-pushed
the
307-max-topic-length
branch
from
8fd5628 to
cc98cf8
Compare
|
Thanks @chrisbobbe and @gnprice for the reviews. Revision pushed. |
3 participants
This will be good to have for the second part of #124 (which will be topic-link autocomplete), as well. Specifically, how far back we will want to look for a topic-link autocomplete interaction.
Fixes: #307