View messages sent by a user from their profile - #2356
Conversation
|
(Would appreciate some early feedback on the overall approach and the UI/UX!) |
96d097c to
e26c586
Compare
ce42e9a to
8006da1
Compare
8006da1 to
8aa3c0f
Compare
|
Thanks for the working on this @sm-sayedi! Could you please rebase this PR to latest main, and resolve conflicts. |
8aa3c0f to
3f09a82
Compare
|
@rajveermalviya Conflicts resolved. |
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks for working on this @sm-sayedi! Comments below, I haven't yet closely looked at the UI code in the final commit, but implementation otherwise seems to be working fine in manual testing.
| TextField(decoration: InputDecoration(hintText: 'Search')) | ||
| => true, |
There was a problem hiding this comment.
nit: Can occupy the line
| TextField(decoration: InputDecoration(hintText: 'Search')) | |
| => true, | |
| TextField(decoration: InputDecoration(hintText: 'Search')) => true, |
|
|
||
| if (narrow case SearchNarrow(keyword: '')) { | ||
| // The server would reject an empty keyword search; skip the request. | ||
| if (narrow case SearchNarrow(filters: [])) { |
There was a problem hiding this comment.
nit: Prefer const empty list literals.
| if (narrow case SearchNarrow(filters: [])) { | |
| if (narrow case SearchNarrow(filters: const [])) { |
There was a problem hiding this comment.
This particular case seems to be breaking some tests! It checks if filters == const [], which is not true, for example, when an empty search query is submitted or when the "X" icon is tapped! In these cases, filters == [].
|
|
||
| @override | ||
| int get hashCode => Object.hash('SearchNarrow', keyword); | ||
| int get hashCode => Object.hash('SearchNarrow', filters); |
There was a problem hiding this comment.
Hash calculation of list should be done using Object.hashAll.
| int get hashCode => Object.hash('SearchNarrow', filters); | |
| int get hashCode => Object.hash('SearchNarrow', Object.hashAll(filters)); |
And if we need order-independency then use Object.hashAllUnordered and const UnorderedIterableEquality<ApiNarrowElement>().equals(other.filters, filters) for equals above.
| @override | ||
| bool operator ==(Object other) { | ||
| if (other is! ApiNarrowElement) return false; | ||
| return (operator, operand, negated) == (other.operator, other.operand, other.negated); |
There was a problem hiding this comment.
The operator getter will throw (in debug mode) for ApiNarrowChannel and ApiNarrowDm, because of asserts in their getter implementation.
There was a problem hiding this comment.
The new revision removes ApiNarrowEelement.== and instead relies on a string _key for equality, a similar pattern used in DmNarrow.
| onPressed: () => Navigator.push(context, | ||
| MessageListPage.buildRoute(context: context, | ||
| narrow: SearchNarrow('')))), | ||
| narrow: SearchNarrow(filters: [])))), |
There was a problem hiding this comment.
nit: Prefer const empty list literals, here and below.
| super.initState(); | ||
| } | ||
|
|
||
| void _udpateSearch() { |
|
|
||
| return Container( | ||
| // Uses the constraint value of a similar search field in new_dm_sheet.dart: | ||
| // https://github.com/zulip/zulip-flutter/blob/01907d833/lib/widgets/new_dm_sheet.dart#L265 |
There was a problem hiding this comment.
Not sure we've ever linked to our repo's code like this (https://github.com/zulip/zulip-flutter/blob/…), should be fine to drop this comment.
There was a problem hiding this comment.
What about replacing it with a short comment like the following:
// Same max height as the search field in [_NewDmSearchBar].
That height value doesn't match the Figma design, but to stay consistent, it uses the value from _NewDmSearchBar. So I think a small hint about where the value comes from would be helpful for future readers.
| "@starredMessagesPageTitle": { | ||
| "description": "Page title for the 'Starred messages' message view." | ||
| }, | ||
| "searchPageTitle": "Search", |
There was a problem hiding this comment.
This already exists, see searchMessagesPageTitle.
| MessageListPage.buildRoute(context: context, | ||
| narrow: SearchNarrow(filters: [ApiNarrowSender(userId)]))), | ||
| icon: const Icon(ZulipIcons.message_square), | ||
| label: Text(zulipLocalizations.profileButtonViewMessagesSent)), |
There was a problem hiding this comment.
We'll want a different button label for self-user profile.
There was a problem hiding this comment.
It seems like the Web uses the same View messages sent label for all profiles! 🙂 Please let me know if you have something else in mind.
|
|
||
| @override | ||
| ApiNarrow apiEncode() => [ApiNarrowSearch(keyword)]; | ||
| ApiNarrow apiEncode() => filters; |
There was a problem hiding this comment.
Let's return either a copy (.toList()) or a List.unmodifiable(filters).
304fefa to
472b7e1
Compare
|
Thanks for the review @rajveermalviya! Revision pushed with a few replies to the thread above; PTAL. |
472b7e1 to
aa250ac
Compare
When the basic keyword search was first implemented (zulip#1662), tests were left out so the feature could be quickly shipped to the users. Writing the tests was tracked as zulip#1667, but was later deferred until the next time we'd make changes to this part of the code. Now, it is that time.
So far, we've used this narrow only for keyword searches. Zulip supports a rich set of search filters in addition to the keyword filter. This rename is to prepare for supporting search in general, not just by keyword.
This is in preparation for supporting other search filters, not just the existing keyword filter. The reason for representing the SearchNarrow by a list of `ApiNarrowElement`s (and not by passing the filter values separately, e.g. as in TopicNarrow) is that the search narrow can get fairly complex with an arbitrary number of filters in arbitrary orders. This way, we can keep the filters original order, for example, when parsing a search narrow URL or when displaying the search filters in the UI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…inkFragment This matches the order of the cases to the order of the corresponding class declarations. The one exception is ApiNarrowDm, which precedes its subclasses in their declarations. In the switch, however, its subclasses must come first; otherwise they'll never match.
The search bar that the previous Figma link pointed to seems to be the only instance where the font height is 26. In all other instances, the font height is 28. This new size also seems to be working well with other UI elements in the search bar, such as the search pills. Look at the font height of different search bars in the following link: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=10806-61281&t=BCnHanef27fpPXMk-0
…ge list To prepare for supporting more search filters in addition to the existing keyword search. Almost all of the search filters will be displayed as pills inside the search bar. Search bar being in the app bar cannot provide us with the flexible vertical space we need for accommodating those search pills. For that, we need to move the search bar out of the app bar. Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=11362-5093&t=BCnHanef27fpPXMk-0
Search pills will need to appear alongside the TextField, which requires building the layout manually so that the TextField can later be placed inside a Wrap with the search pills. This change uses the Row widget for placing the search and remove icons to the sides of the TextField.
…fault In the next commits, which will support several types of search filters (sender and keyword search in this PR), we will want to have different placeholders for the empty message-list page, based on the filters applied. emptyMessageListSearch will be the default placeholder header when we don't have a specific one, thus the rename.
With a sender filter applied, the search page now has its search bar pre-filled with a pill representing the sender and its message list showing all the messages the sender has sent and that the self-user has access to. The sender's messages can be further filtered by submitting a search keyword. Tapping the pill removes it, without immediately updating the search results. The search results can be updated by submitting (a new) search query. As of this commit, there is no path in the UI to reach the search page with the sender filter applied; soon that'll be possible when "View messages sent" button is added to the profile page. Also, there is currently no way to choose a different sender using the search bar; that's zulip#1660. Figma design for the search page with the pill(s): https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=10904-101132&t=BCnHanef27fpPXMk-0
Fixes zulip#679. Tapping the button leads to the search page showing all the messages the profile user has sent and that the self-user has access to.
aa250ac to
5457afb
Compare
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks for the revision! All LGTM, modulo some nits below.
Moving over to Chris' review.
| void _clearInput() { | ||
| _controller.clear(); | ||
| _handleSubmitted(''); | ||
| _searchFilters.clear(); |
There was a problem hiding this comment.
This should be wrapped in setState.
| setState(() { | ||
| _searchFilters.remove(senderFilter); | ||
| }); | ||
| _focusNode.requestFocus(); |
There was a problem hiding this comment.
msglist: Support sender filter in the search page
…
Tapping the pill removes it, without immediately updating the search results.
The search results can be updated by submitting (a new) search query.
…
Hmm, is there a reason for this preference, we could update the search too by adding a _updateSearch() here, right?
| ApiNarrowSearch(this.operand, {super.negated}) | ||
| : assert(operand.trim() == operand), | ||
| assert(operand.isNotEmpty); |
There was a problem hiding this comment.
I think generally avoid asserts in API types, so we'll need to move these validation to somewhere else.
| // TODO(#1660): handle more cases | ||
| _ => null, |
There was a problem hiding this comment.
nit:
| // TODO(#1660): handle more cases | |
| _ => null, | |
| // The keyword filter appears in the text field itself, not as a pill. | |
| ApiNarrowSearch() => null, | |
| // TODO(#1660) show pills for these too | |
| ApiNarrowChannel() || ApiNarrowTopic() || ApiNarrowDm() | |
| || ApiNarrowIs() || ApiNarrowWith() || ApiNarrowMessageId() => null, |
| fragment.write(_encodeHashComponent(element.operand)); | ||
| case ApiNarrowSender(): | ||
| final senderId = element.operand; | ||
| final name = store.getUser(senderId)?.fullName ?? 'unknown'; |
There was a problem hiding this comment.
I see, yeah that seems right.
|
|
||
| return Container( | ||
| // Uses the constraint value of a similar search field in new_dm_sheet.dart: | ||
| // https://github.com/zulip/zulip-flutter/blob/01907d833/lib/widgets/new_dm_sheet.dart#L265 |
Fixes #679.
This PR adds a button ("View messages sent") to the profile screen, tapping which opens the messages list showing all the messages sent by the user (that the self-user can access). The message list is actually a search view with the search bar pre-filled with a pill representing the user.
This PR also changes the location of the search bar from its previous location in the app bar to the top of the message list. This change seemed necessary as the search bar can now contain a search pill for the sender that can take one line and push the actual text field to the second line. In #1660, we will have several types of search pills that need to be accommodated inside the search bar. The app bar doesn't seem to provide us with enough flexible height, so we have to take the search bar out of it.
Figma links:
Screenshots
Screen recording
Screen.Recording.2026-07-07.at.2.50.58.AM.mov