Get compose box hint texts ready for general chat

[PIG208]

Prepares for:

• general chat #1: add support at most places #1297

mandatory topics	non-mandatory topics

[chrisbobbe]

Thanks! Small comments below.

[chrisbobbe]

Huh, I'm surprised this test passes. When using the app, when the topic input is empty, I see the hint text "Topic" there—but I don't see (or expect to see) hint text after I've typed something; I just see the text I've typed. Do you know why this is passing?

[PIG208]

This helper checks TextField.decoration.hintText directly, but does not test if hintText is invisible when the input is non-empty (it does check if the input exists or not at all). We could figure out a way to construct the finder that distinguishes the content and the hint text, but I felt that upstream covers this aspect already. Maybe we can add a dartdoc for this helper?

[chrisbobbe]

This helper checks TextField.decoration.hintText directly

Does it? I don't see .hintText in its implementation:

    void checkComposeBoxHintTexts(WidgetTester tester, {
      String? topicHintText,
      required String contentHintText,
    }) {
      if (topicHintText != null) {
        check(find.descendant(
          of: topicInputFinder, matching: find.text(topicHintText))).findsOne();
      } else {
        check(topicInputFinder).findsNothing();
      }
      check(find.descendant(
        of: contentInputFinder, matching: find.text(contentHintText))).findsOne();
    }

[PIG208]

Ah! I was mistaken. It doesn't check for the controller but find.text still ignores that the hint text can be invisible.

I was probably thinking of a different revision commenting on this.

[chrisbobbe]

compose: Use alternative placeholder if topics are mandatory and missing

We have this pair of tests:

• group 'to ChannelNarrow, topics not mandatory'
  • test 'with empty topic'
  • test 'with non-empty topic'

And we have this pair of tests:

• test 'to ChannelNarrow, mandatory topics with empty topic'
• test 'to ChannelNarrow, mandatory topics with non-empty topic'

It seems cleaner if, for both pairs, it either uses group or doesn't, with consistent naming.

[PIG208]

Updated the PR. Thanks for the review!

[chrisbobbe]

Thanks! Comments below, all small.

[chrisbobbe]

I think the word you want is "vacuous" 🙂

[chrisbobbe]

Suggested change

	/// Whether [textNormalized] is invalid in terms of checks for mandatory
	/// topics.
	/// Whether [textNormalized] would fail a mandatory-topics check
	/// (see [mandatory]).

A bit more information-dense, and the [mandatory] reference could be helpful.

[chrisbobbe]

I'm thinking through the logic in this sentence and getting confused; I wonder if there's something clearer to say or if the function's behavior should change. Maybe this version?:

This checks the input's configured hint text without regard to whether it's currently visible, as it won't be if the user has entered some text.

When thinking about cases to cover in UI tests, and how to describe them, I think they should usually be cases that observant users might report from their experience of the UI. Is "present but invisible" a description we might hear from users?

When the hint text remains in the widget tree after the user types something, I see two plausible explanations:

• It's intentional but just not part of the visible interface; it's for screen-reader software
• It's just an implementation detail

Could you do a quick check to see if it seems like just an implementation detail? If so, then we can comment that the checks might start failing if that detail changes; that would be a helpful clue if it starts happening. If it's hard to tell, a similar comment could still be helpful but just noting more uncertainty.

[PIG208]

This comes from _InputDecoratorState.build:

    final TextStyle hintStyle = _getInlineHintStyle(themeData, defaults);
    final String? hintText = decoration.hintText;
    final bool maintainHintSize = decoration.maintainHintSize;
    Widget? hint;
    if (decoration.hint != null || hintText != null) {
      final Widget hintWidget =
          decoration.hint ??
          Text(
            hintText!,
            style: hintStyle,
            textDirection: decoration.hintTextDirection,
            overflow:
                hintStyle.overflow ??
                (decoration.hintMaxLines == null ? null : TextOverflow.ellipsis),
            textAlign: textAlign,
            maxLines: decoration.hintMaxLines,
          );
      final bool showHint = isEmpty && !_hasInlineLabel;
      hint =
          maintainHintSize
              ? AnimatedOpacity(
                opacity: showHint ? 1.0 : 0.0,
                duration: decoration.hintFadeDuration ?? _kHintFadeTransitionDuration,
                curve: _kTransitionCurve,
                child: hintWidget,
              )
              : AnimatedSwitcher(
                duration: decoration.hintFadeDuration ?? _kHintFadeTransitionDuration,
                transitionBuilder: _buildTransition,
                child: showHint ? hintWidget : const SizedBox.shrink(),
              );
    }

So for our test to work as-is it would require maintainHintSize to be true (the current default), and that they continue to just change the opacity when hiding it. It appears to be an implementation detail.

We could also change the behavior of the test, by making sure that the hint text is visible. I tried Using .hitTestable when appropriate, but it doesn't work (the check always fails). I think it has to do with how the hint text is rendered.

Alternatively, we could switch to checking .decoration on the TextField, so that we don't have to rely on this implementation detail. Will be pushing this in the next revision.

[chrisbobbe]

Thanks! LGTM, marking for Greg's review.

[gnprice]

Thanks both!

Have a few minutes at the moment, so I've read all but the tests; comments below.

[gnprice]

Suggested change

	/// The term "Vacuous" draws distinction from [String.isEmpty], in the sense
	/// that certain strings are empty but also indicate the absence of a topic.
	bool get isTopicVacuous => textNormalized == kNoTopicTopic;
	/// The term "Vacuous" draws distinction from [String.isEmpty], in the sense
	/// that certain strings are not empty but also indicate the absence of a topic.
	bool get isTopicVacuous => textNormalized == kNoTopicTopic;

? Or am I misunderstanding what you mean?

[gnprice]

compose: Use alternative placeholder if topics are mandatory and missing

Previously, the placeholder reads "#channel name > (no topic)" even if
topics are mandatory for the realm.

This change in behavior seems fine, but not especially helpful. It's fine for the hint to echo the thing the user has asked to do, even if that's something they won't ultimately be able to do.

Does this set up this code in a way that will be convenient in a later PR for "general chat"? That'd be a good reason for it.

[PIG208]

It's mainly to later prevent hinting the existence of "general chat" at all, if topics are required for the realm.

This is also meant to match web's behavior. On web, "general chat" never shows up in the hint text if topics are required;

and "(no topic)" never shows up in the content placeholder regardless if topics are required (this implementation differs from it by showing "(no topic)" in the placeholder conditionally).

[gnprice]

It's mainly to later prevent hinting the existence of "general chat" at all, if topics are required for the realm.

Cool, that's certainly good. Where can I read the code that builds on this to do that?

[PIG208]

The change is on a draft branch, #1365 (51772ec), where we extend the checks to initialize topicDisplayName, but some more work is needed to make this display name stateful.

[gnprice]

The reasoning in the last commit:

compose: Avoid translating Zulip message destinations

The '#channel > topic' style strings are not supposed to be translated
into different languages as they are Zulip's language of expressing the
desintation, not something bound to the English language.

See also:
  https://github.com/zulip/zulip-flutter/pull/1148#discussion_r1941990585

applies equally well to this string, right?

So maybe let's keep this as a single translated string, and just pass either "#general" or "#general > lunch" to it from app code.

[gnprice]

It looks like at least as of #1297, this line and the definition of isTopicVacuous are the same as in this PR. Is that what you're expecting in your current plan for how "general chat" will work in this file?

If that's the case, I think the isTopicVacuous getter isn't really doing much for us — it'd be clearer to just inline it here as well as its one other use site.

We discussed a getter along those lines last week, and the idea then was that in a "general chat" world the isVacuous getter would gain additional logic related to "general chat". If there's no such logic to put there, it's not needed.

[PIG208]

We start adding logic to this in #1364 (6886bc59), when implementing send for empty topics.

[gnprice]

The link in #1342 (comment) is helpful, thanks.

Can you write down what topicDisplayName being null is intended to mean? After staring at that later code for a few minutes, I think the answer is: it's null when the user hasn't yet picked a valid topic.

But then that doesn't seem to agree with the remark here:
#1342 (comment)
If the user hasn't yet picked a topic at all, but topics aren't mandatory, it looks like the logic in the current version of #1364 would show "general chat" in the hint text here, which as mentioned in that comment isn't what we want.

[gnprice]

Separately: I think it would be cleanest for this topicDisplayName logic to live on ComposeTopicController (as a getter there).

When I try reasoning through what it's doing — particularly in the eventual version found in #1364 — it's pretty intricately tied together with the details of what isTopicVacuous does. So it'd be helpful for their definitions to live next to each other.

[gnprice]

The mixing here of some data that this class takes from the topic controller and then stores locally (_topicTextNormalized) and other data that it gets directly from the topic controller (isTopicVacuous) makes me nervous — what if those are out of sync?

I think that isn't actually a possibility, because I think _topicTextNormalized should always be the latest value from whenever the topic controller last changed. But it'd be good to not have to worry about it.

Looking at the history, I think the right solution is that there's no reason this widget state needs to be keeping _topicTextNormalized around at all — sent #1367 to fix that. (It was needed originally, but that was before we started keeping the normalized value around on the controller.)

[PIG208]

Thanks for the review! I decided to drop the alternative placeholder commit because I think implementing the required refactoring in the later PRs helps with finding a better solution, as indicated in #1342 (comment).

[gnprice]

Thanks for the revision! I've now read the whole thing, and this all looks good now; just two small comments.

[gnprice]

nit: I think this is a case where much of the information in the commit message is also relevant to understanding the resulting code (not only to understanding its history). So:

Suggested change

	'#$streamName > ${topic.displayName}'));
	// No i18n of this use of "#" and ">" string; those are part of how
	// Zulip expresses channels and topics, not any normal English punctuation,
	// so don't make sense to translate. See:
	// https://github.com/zulip/zulip-flutter/pull/1148#discussion_r1941990585
	'#$streamName > ${topic.displayName}'));

[gnprice]

Hmm, this is a bit unsatisfying because it's checking a condition the user doesn't see. Ideally we'd be checking whether the user actually sees a hint, and if so then what it is.

From the thread at #1342 (comment) it sounds like getting that ideal behavior might be more complicated, though. In that case it's fine to let this be — the question of whether a hint is visible isn't critical for us to test, particularly as it's mostly the responsibility of the framework's underlying text-input code anyway.

[PIG208]

Thanks for the review! Updated the PR. I kept the commit message from the last commit, and added the proposed comments at the places that use the new string.

[gnprice]

Thanks! Looks good; merging.