Skip to content

msglist: When initial message fetch comes up empty, auto-focus compose box #1544

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chrisbobbe
Copy link
Collaborator

This is part of our plan to streamline the new-DM UI: when you start a new DM conversation with no history, we should auto-focus the content input in the compose box.

Fixes: #1543

…e box

This is part of our plan to streamline the new-DM UI: when you start
a new DM conversation with no history, we should auto-focus the
content input in the compose box.

Fixes: zulip#1543
@chrisbobbe chrisbobbe requested a review from rajveermalviya June 3, 2025 16:58
@chrisbobbe chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Jun 3, 2025
@chrisbobbe
Copy link
Collaborator Author

(The CI failure is #1545; I've sent #1546 for that.)

Copy link
Member

@rajveermalviya rajveermalviya left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@rajveermalviya rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jun 4, 2025
@rajveermalviya rajveermalviya requested a review from gnprice June 4, 2025 11:49
Comment on lines +1621 to +1628
@override void requestFocusIfUnfocused() {
if (topicFocusNode.hasFocus || contentFocusNode.hasFocus) return;
if (topicInteractionStatus.value == ComposeTopicInteractionStatus.notEditingNotChosen) {
topicFocusNode.requestFocus();
} else {
contentFocusNode.requestFocus();
}
}
Copy link
Collaborator Author

@chrisbobbe chrisbobbe Jun 6, 2025

Choose a reason for hiding this comment

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

This function could also be useful for #1448.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good; some small comments below.

Comment on lines +1551 to +1552
/// A convenience method to encapsulate choosing the topic or content input
/// when both exist (see [StreamComposeBoxController.requestFocusIfUnfocused]).
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd think of a "convenience method" as doing something the caller could have perfectly reasonably done for itself, but which just makes it a bit easier or shorter at the caller. Because this is encapsulating meaningful logic of its own (at least in the subclass implementation), I wouldn't call it a "convenience method".

Suggested change
/// A convenience method to encapsulate choosing the topic or content input
/// when both exist (see [StreamComposeBoxController.requestFocusIfUnfocused]).
/// This encapsulates choosing the topic or content input
/// when both exist (see [StreamComposeBoxController.requestFocusIfUnfocused]).

Comment on lines +1623 to +1626
if (topicInteractionStatus.value == ComposeTopicInteractionStatus.notEditingNotChosen) {
topicFocusNode.requestFocus();
} else {
contentFocusNode.requestFocus();
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a switch — seems like one wants to explicitly think through each of the values of ComposeTopicInteractionStatus in order to understand this logic.

Comment on lines +158 to +159
check(controller).isA<StreamComposeBoxController>()
.topicFocusNode.hasFocus.isFalse();
Copy link
Member

Choose a reason for hiding this comment

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

Can make this check a bit stronger, verifying focus doesn't go to the content input either:

Suggested change
check(controller).isA<StreamComposeBoxController>()
.topicFocusNode.hasFocus.isFalse();
check(controller).isA<StreamComposeBoxController>()
..topicFocusNode.hasFocus.isFalse()
..contentFocusNode.hasFocus.isFalse();

Comment on lines +192 to +194
await prepareComposeBox(tester,
narrow: DmNarrow.withUser(user.userId, selfUserId: store.selfUserId),
messages: [eg.dmMessage(from: user, to: [store.selfUser])]);
Copy link
Member

Choose a reason for hiding this comment

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

store is set up by prepareComposeBox itself, so these end up using the state left behind by the previous test case. You can see the effect by running the case in isolation:

$ flutter test test/widgets/compose_box_test.dart --name DmNarrow
00:06 +0: auto focus DmNarrow, non-empty fetch                                   
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following LateError was thrown running a test:
LateInitializationError: Local 'store' has not been initialized.

When the exception was thrown, this was the stack:
#0      LateError._throwLocalNotInitialized (dart:_internal-patch/internal_patch.dart:214:5)
#1      main.<anonymous closure>.<anonymous closure> (file:///home/greg/z/flutterz/test/widgets/compose_box_test.dart)
[…]

Looks like prepareComposeBox takes a selfUser argument, so that can be used to control this.

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

Successfully merging this pull request may close these issues.

msglist: When the initial message fetch comes up empty, auto-focus the compose box
3 participants