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

Edit-message (2/n): Implement variant of content input without a typing notifier #1431

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

chrisbobbe
Copy link
Collaborator

The edit-message compose box (#126) shouldn't send typing notifications like the regular compose box does.

(There's actually a different kind of typing notification that we'll want to implement eventually; that's #1350. But that's not for us to do now.)

This PR implements _ContentInput.noTypingNotifier, as prep for implementing the edit-message compose box.

Related: #126

@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Mar 21, 2025
@chrisbobbe chrisbobbe requested a review from PIG208 March 21, 2025 21:50
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 refactor! This will be helpful for implementing saved snippets as well; left a comment bringing that into the discussion.

// ignore: unused_element
factory _ContentInput.noTypingNotifier({
required Narrow narrow,
required SendableNarrow destination,
Copy link
Member

@PIG208 PIG208 Mar 24, 2025

Choose a reason for hiding this comment

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

I think ideally destination shouldn't be needed for this variant, but I can't think of a better alternative without making destination nullable in the base class _ContentInput and having null-checks in the mixin. (I think this—having a default constructor that allows all combinations, and factories with set values and assertions—is how some Flutter upstream widgets (e.g.: SwitchListTile) handle this.)

For editing messages, providing a reasonable value for destination might still be convenient for the caller (and will become a requirement when we get to supporting typing notifications for editing).

Saved snippets might need a variant that does not have typing notifier or autocompletion; in that case neither narrow nor destination should be needed from the caller, as there are no reasonable values other than null for them anyway:

  factory _ContentInput.savedSnippet({
    required ComposeBoxController controller,
    required String hintText,
  }) => _ContentInputNoTypingNotifier._(
    narrow: null,
    destination: null,
    controller: controller,
    hintText: hintText,
  );

Copy link
Member

@PIG208 PIG208 Mar 24, 2025

Choose a reason for hiding this comment

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

Another approach would be dropping the mixin, moving the typing notifier logic to _ContentInputStateWithTypingNotifier, and moving the destination field to _ContentInputWithTypingNotifier. However, that won't help if we need the mixin in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another approach would be dropping the mixin, moving the typing notifier logic to _ContentInputStateWithTypingNotifier, and moving the destination field to _ContentInputWithTypingNotifier. However, that won't help if we need the mixin in the future.

Interesting! I'll push some wip commits trying this out.

@chrisbobbe chrisbobbe marked this pull request as draft March 24, 2025 23:41
@chrisbobbe
Copy link
Collaborator Author

Thanks for the review! I've pushed some NOMERGE commits with a way to have _ContentInputNoTypingNotifier without destination, following the direction you mentioned in #1431 (comment). It preserves the property that the build method is shared among the with- and without-typing-notifier variants.

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! This change LGTM. I guess destination might need dartdoc highlighting its relationship with typing notifications.

@@ -486,6 +533,10 @@ class _ContentInputState extends State<_ContentInput> with WidgetsBindingObserve
}
}


Copy link
Member

Choose a reason for hiding this comment

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

nit: remove extra newline

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.

2 participants