general chat #3: support alternative styling for topic input on edits/focus/blur

[PIG208]

The last PR of the sequence.

Previous ones:

• general chat #1: add support at most places #1297
• general chat #2: support sending to empty topic from channel narrow, hide resolve/unresolve conditionally #1364

Fixes: #1250

Screenshots:

focus	mandatory	non-mandatory	non-mandatory (legacy)
no focus
topic focused
content focused

[PIG208]

Alternatively, with switch:

final defaultTopicDisplayName = store.zulipFeatureLevel >= 334
  ? store.realmEmptyTopicDisplayName : kNoTopicTopic;

final decoration = switch ((
  store.realmMandatoryTopics,
  widget.controller.hasChosenTopic.value,
  widget.controller.topicFocusNode.hasFocus,
)) {
  (false, true, _) => InputDecoration(
    hintText: defaultTopicDisplayName,
    hintStyle: topicTextStyle.copyWith(
      fontStyle: store.zulipFeatureLevel >= 334 ? FontStyle.italic : null)),
  (false, false, true) => InputDecoration(
    hintText: zulipLocalizations.composeBoxEnterTopicOrSkipHintText(
      defaultTopicDisplayName),
    hintStyle: hintStyle),
  (_, _, _) => InputDecoration(
    hintText: zulipLocalizations.composeBoxTopicHintText,
    hintStyle: hintStyle),
};

[gnprice]

I think that switch would be easier to understand as an if/else chain — I feel like as the reader I'd be basically transforming it into that in my head in order to understand what it's saying.

[chrisbobbe]

Thanks! Small comments below.

[chrisbobbe]

How about "Whether the user seems to have made up their mind choosing a topic." or similar? It's impossible to know for sure, right :) they could come back and change it again before sending, or they could send accidentally before making up their mind.

Also I don't understand what "empirically" means here. The sentence sounds like we're an observer making notes from what we've seen of the app's behavior, rather than the ones deciding what the behavior is and causing it. 🙂

[chrisbobbe]

nit: spacing

[chrisbobbe]

compose: Track user interaction with inputs statefully

What's the behavior change in this commit?

[chrisbobbe]

Can we assert that cases (_, false, false) and (_, true, true) never happen? If those states can happen, they probably won't persist for more than a frame—but that would be a frame where the text would be flickering to something different, and it would look janky.

Also would it be helpful to mention realmMandatoryTopics in some of these comments?

I wonder if an if/else might be a better fit for this logic than a switch.

[PIG208]

I think (_ false, false) can represent the initial state when the user open a page without interacting with any of the inputs, so it is definitely possible.

(_, true, true) seems possible as a really intermediate state, but it is hard to tell without staring at the conditions for some time.

These three cases might be better represented with an enum:

• none when the user hasn't interacted with the inputs;
• isEditing when the topic input has focus;
• hasEdited when the content input has focus/topic input lost focus previously.

We can tweak the behavior a bit so none -> isEditing, none -> hasEdited, and isEditing <-> hasEdited are the only possible state transitions. This eliminates the possible flickering when more than one transition can happen between frames (namely the case when topic input is unfocused and content input gains focus, which can be represented as the isEditing -> hasEdited transition).

[PIG208]

Actually, if we implement the states but keeping the current behavior, it still seems to be conceptually subject to the flickering issue when the topic input loses focus and content input gains focus, if the changes to the focus node do not happen in the same frame.

However, it doesn't seem to be the case when testing this:

  void _topicFocusChanged() {
    print('topic changed: $buildCount');
    setState(() {
      if (widget.controller.topicFocusNode.hasFocus) {
        widget.controller.topicEditStatus.value = ComposeTopicEditStatus.isEditing;
      } else if (!widget.controller.contentFocusNode.hasFocus) {
        widget.controller.topicEditStatus.value = ComposeTopicEditStatus.notInteracted;
      }
    });
  }

  void _contentFocusChanged() {
    print('content changed: $buildCount');
    setState(() {
      if (widget.controller.contentFocusNode.hasFocus) {
        widget.controller.topicEditStatus.value = ComposeTopicEditStatus.hasChosen;
      }
    });
  }

Log message when the focus is shifted from topic to content input:

I/flutter (13922): topic changed: 2
I/flutter (13922): content changed: 2

The else if (!widget.controller.contentFocusNode.hasFocus) check should then be sufficient to prevent the flickering issue.

[PIG208]

Thanks for the review! I have updated the PR. This revision rewrites the feature using enums and a state machine.

[chrisbobbe]

Thanks! This seems to have gathered a rebase conflict; could you resolve that please?

[chrisbobbe]

Thanks! Here's a review of the first two commits:

90770e1 compose: Unskip test; handle TopicName with empty displayName for now
ec6622f compose [nfc]: Convert _TopicInput to a StatefulWidget

and I've read part of the third:

5132980 compose: Change topic input hint text

[chrisbobbe]

compose: Unskip test; handle TopicName with empty displayName for now

This logic was introduced in 769cc7df, which relied on the fact that
TopicName.displayName is `null` when the topic is empty.  Normally it is
fine to assume that the displayName is non-empty with the server flags,
but in this case the TopicName is constructed by our code.

I'm having trouble understanding the commit message. I'm having trouble with this part:

the fact that TopicName.displayName is null when the topic is empty

because that wasn't a fact at 769cc7d, and it's still not a fact in main. The TopicName.displayName field's type is String, not String?, so the value can't be null.

I'm also having trouble with the second sentence ("Normally…"). Is the empty string a valid value for TopicName.displayName or not? I suspect it isn't, and our code shouldn't be constructing a TopicName that way.

It looks like the only thing our code does with that TopicName is to get its displayName. How about skipping the trip through TopicName and just working only with strings? (This would mean the commit-message summary line needs updating.)

[chrisbobbe]

Also, I notice the code uses the store.realmEmptyTopicDisplayName getter. That getter has this in its dartdoc:

/// This should only be accessed when FL >= 334 […]

but it takes some nonlocal reasoning to confirm that this access indeed only happens when FL >= 334. How about at least adding an assert on the zulipFeatureLevel in that getter, so we can be more confident that we won't regress on that part of its contract?

[PIG208]

Yeah. the value of TopicName usually comes from the server, and before we start passing the "empty_topic_names" client capability, displayName being empty is unexpected. I think it should be fine to work with strings here.

Not sure if we want to switch back to TopicName once TopicName.displayName becomes nullable. In UI code, we usually handle empty topic names by checking if TopicName.displayName is null, which should also make grepping easier.

[gnprice]

TopicName is for requests we make (or might make) to the server, as much as it is for values we get from the server. That's the reason for its apiName getter:

  /// The string this topic is identified by in the Zulip API.
  ///
  /// This should be used in constructing HTTP requests to the server,
  /// but rarely for other purposes.  See [displayName] and [canonicalize].

and is why it appears in a number of places in the compose-box code.

As of the tip of the branch, I think it's helpful that this hintTopic local is a TopicName, because that allows the logic for falling back to "general chat" to look the same as the corresponding logic across the rest of the codebase:

      : '#$streamName > ${hintTopic.displayName ?? store.realmEmptyTopicDisplayName}';

Looking at references to realmEmptyTopicDisplayName, one sees that almost all of them are gated behind a condition of some TopicName.displayName being null, and the majority are in exactly this form with a ??.

I haven't looked closely at the individual commits in this branch to have a suggestion on the clearest way to break the changes down into steps, though.

[chrisbobbe]

compose [nfc]: Convert _TopicInput to a StatefulWidget

We will shortly start adding listeners for this.

What is "this" in the commit message? I think this commit message doesn't need a body, but as-is, it sounds like it's saying we want something to listen to the newly stateful widget itself, and I don't know what that means.

[chrisbobbe]

compose: Change topic input hint text

This is similar to web's behavior.  When topics are not mandatory:

- an alternative hint text "Enter a topic (skip for general chat)"
  is shown when the topic input has focus;

- an opaque placeholder text (e.g.: "general chat") is shown if
  the user skipped to content input;

Because the topic input is always shown in a message list page channel
narrow (assuming permission to send messages), this also adds an intial
state:

- a short hint text, "Topic", is shown if the user hasn't
  interacted with topic or content inputs at all.

This only changes the topic input's hint text.

See CZO discussion for design details:
  https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/general.20chat.20design.20.23F1297/near/2106736

Commit-message nits:

• 'Enter a topic (skip for “general chat”)'
• spelling of 'initial'

[chrisbobbe]

Since widget.controller keeps a reference to the old store (widget.controller.topic.store), we should remove that reference when there's a new store; _TopicInputState should use PerAccountStoreAwareStateMixin.

[PIG208]

I'm not sure what to include in the onNewStore method. _TopicInputState doesn't have listeners on widget.controller.topic. Its only reference to widget.controller.topic is in the build method, where it passes the widget.controller.topic instance as-is.

widget.controller comes from and is owned by _ComposeBoxState. It does implement a onNewStore method that swaps out widget.controller.topic.store with the new store. Maybe that should be sufficient?

[chrisbobbe]

I think "how a user has edited the topic" isn't the best description for what this represents. If I use this to try to straightforwardly describe each state, the results are misleading:

• "The user has edited the topic in the 'none' way"—or perhaps "The user has not edited the topic"…but looking at the diagram, that's definitely not what this state means.

Or awkward/confusing:

• "The user has edited the topic in the 'has chosen' way."
• "The user has edited the topic in the 'is editing' way."

Or from another angle: it's equally possible to be in any of the states after any given sequence of edits (if an edit means changing the text in the input).

[chrisbobbe]

Is the "topic input gains focus" label meant to apply equally to the hasChosen -> isEditing transition? It's positioned so it looks like it's just meant to apply to none -> isEditing.

[chrisbobbe]

/// - Otherwise, the edit status can be either of [none] or [hasChosen],
///   but never [isEditing].

"Otherwise" means neither the topic input nor the content input has focus, right? Looking at the diagram, I don't see how the state could be hasChosen in that "otherwise" case.

[PIG208]

Yeah. How about maybe ComposeTopicInteractionStatus? I'm also not happy with the none name. We can be a bit more literal about what that means. Updated the diagram:

/// ```
///                       (default)
///    Topic input            │          Content input
///    lost focus.            ▼          gained focus.
///   ┌────────────► notEditingNotChosen ────────────┐
///   │                                 │            │
///   │         Topic input             │            │
///   │         gained focus.           │            │
///   │       ◄─────────────────────────┘            ▼
/// isEditing ◄───────────────────────────── hasChosen
///   │         Focus moved from                     ▲
///   │         content to topic.                    │
///   │                                              │
///   └──────────────────────────────────────────────┘
///    Focus moved from
///    topic to content.
/// ```

The state can be hasChosen even after content input has lost focus. Will revise that to make it a bit clearer.

[PIG208]

Thanks for the review! Pushed an update.

[chrisbobbe]

Thanks! I've now read through all the commits. Small comments below.

[chrisbobbe]

compose: Change topic input hint text

When you rebase, this commit will need a flutter run to update the new Ukrainian translation file that exists in main.

[chrisbobbe]

nit: Since this is meant to be the self-account, I'd name it selfAccount instead of just account.

[chrisbobbe]

Would eg.recentZulipFeatureLevel (bumped so it's actually recent and >=334) be just as effective here and in the other places?

[chrisbobbe]

It's possible for the content input to lose focus without the focus moving to the topic input, but that transition doesn't have an arrow in the diagram. If we add one (which I think would be an arrow from hasChosen to itself, in a loop shape), that should help clarify this invariant:

/// - When neither inputs has focus, and content input was the last
///   input among the two to be focused, the status must be [hasChosen].

(also nit: "neither input has focus")

[chrisbobbe]

Here's a draft that takes some complexity out of the switch and removes some repetition (like hintStyle: hintStyle). It's more lines of code, but I think easier to read from top to bottom:

    final topicTextStyle = TextStyle(
      fontSize: 20,
      height: 22 / 20,
      color: designVariables.textInput.withFadedAlpha(0.9),
    ).merge(weightVariableTextStyle(context, wght: 600));

    // TODO(server-10) simplify away
    final emptyTopicsSupported = store.zulipFeatureLevel >= 334;

    late final String hintText;
    TextStyle hintStyle = topicTextStyle.copyWith(
      color: designVariables.textInput.withFadedAlpha(0.5));

    if (store.realmMandatoryTopics) {
      // Something short and not distracting.
      hintText = zulipLocalizations.composeBoxTopicHintText;
    } else {
      switch (widget.controller.topicInteractionStatus.value) {
        case ComposeTopicInteractionStatus.notEditingNotChosen:
          // Something short and not distracting.
          hintText = zulipLocalizations.composeBoxTopicHintText;
        case ComposeTopicInteractionStatus.isEditing:
          // The user is actively interacting with the input.  Since topics are
          // not mandatory, show a long hint text mentioning that they can be
          // left empty.
          hintText = zulipLocalizations.composeBoxEnterTopicOrSkipHintText(
            emptyTopicsSupported
              ? store.realmEmptyTopicDisplayName
              : kNoTopicTopic);
        case ComposeTopicInteractionStatus.hasChosen:
          // The topic has likely been chosen.  Since topics are not mandatory,
          // show the default topic display name as if the user has entered that
          // when they left the input empty.
          if (emptyTopicsSupported) {
            hintText = store.realmEmptyTopicDisplayName;
            hintStyle = topicTextStyle.copyWith(fontStyle: FontStyle.italic);
          } else {
            hintText = kNoTopicTopic;
            hintStyle = topicTextStyle;
          }
      }
    }

    final decoration = InputDecoration(hintText: hintText, hintStyle: hintStyle);

[PIG208]

Thanks! This looks good to me (removed late from late final String hintText).

[chrisbobbe]

Since the test is about "modern servers", let's use eg.recentZulipFeatureLevel instead of hard-coded 334, bumping (in a prep commit) eg.recentZulipFeatureLevel so that it is actually recent and >=334.

[chrisbobbe]

Relatedly, when I write tests, I think I generally omit "on modern servers" in a test description, leaving that as implied, and I write the description (and the test) so that it won't need changes when we eventually clear out the legacy code. Then the legacy-servers test is the thing that gets a marker in its description, like "on legacy servers", and a TODO(server comment to remove the test.

So here, maybe "TopicAutocompleteView getStreamTopics request"? (And add ..method.equals('GET'), for completeness in matching that description.)

[PIG208]

Yeah. We should be able to skip a lot of changes in the first commit if we bump eg.recentZulipFeatureLevel.

[PIG208]

The description changes make sense to be, but I thought that we usually don't have TODO(server-x) comments on tests (since they ought to fail when we drop legacy server support)?

[chrisbobbe]

Mm, yeah, reasonable to leave them out on the tests I think.

[chrisbobbe]

(same comment about using eg.recentZulipFeatureLevel)

[PIG208]

Thanks for the review! Pushed a new update.