autocomplete: Put best matches near input field.

[apoorvapendse]

This commit reverses the list that was originally
presented to the user while showing the typeahead
menu.

This makes sense since on mobile its easier to click on options closer to the input box, i.e. where your fingers are currently present, instead of pressing arrow keys to navigate through the options on a keyboard which is convenient on a desktop setup.

Hence we place the best matching options not at the top of the typeahead menu, but instead at the bottom for better reachability and convenience of the user.

CZO

Fixes #1121.

Here is a demo:
Screencast from 12-11-2024 12:38:19 PM.webm

[PIG208]

Thanks for the PR @apoorvapendse! Left some comment.

This doesn't fix #1123 because the emoji picker (added in #1103) is a feature separate from autocompletion, so please update the PR comment and the commit message to reflect that.

[apoorvapendse]

Hey @PIG208, thanks for reviewing the PR.
I've updated it and hope it meets your expectations.
Could you please elaborate on what kind of comment is expected as per this comment
Thanks.

[apoorvapendse]

@PIG208 thank you for clarifying ,
I don't think this is affected by #226, since the default behavior on CZO also requires you to enter :<some text> in order to trigger the emoji typehead.

Open to your feedback.
Thank you.

[apoorvapendse]

Hey @PIG208,
I am sorry, did you mean to mention #226 instead of #227 in this comment.

I added a comment mentioning that dependency here in the latest commit.

Is that what was expected?

[PIG208]

Thanks for the update! Left some more comments. I read the first test and skimmed the second one. It looks like the current approach of getting scroll offset is not working properly.

For the next revision, please do a round of self-review addressing the style issues mentioned in the comments, and apply it to other places as well.

[apoorvapendse]

Change Summary for @PIG208

• Replaced pumpAndSettle with pump
• Addressed your concern regarding the removal of scrolling in the opposite direction.
• Addressed your concern regarding the usefullness of scrolling.
  Replaced comparing listViewFinder.dy with comparing the y position of the first emoji & mention in case of negative y offset.
  See CZO topic I created for more details.
• Removed ListViewFinder render checks as suggested.
• Moved ListViewFinder initialization before initialScrollOffset as suggested.
• Wrapped the end parens as mentioned.
• Broke the check statements into multiple lines as per this input.
• Removed the redundant check(because:'Each user option should be rendered (index: $i)',finder).findsOne(); as suggested.
• Addressed other long line check statements.
• Removed expectedUserSequence and used users instead.
• Fixed other spacing issues.

Thank you for being patient.
I believe this PR is ready for another review.

[apoorvapendse]

Sorry for responding so late @PIG208 .
GitHub didn't send me any emails about your replies.

I've fixed the for loop formatting here, here and here.

[PIG208]

Thanks for the update! I revised the @-mentions test as an example of what we usually expect for testing. Please read it and try to apply the same approach to the similar test for emojis.

[apoorvapendse]

Hi, @PIG208.
Kindly TAL when you find the time.
I've written a summary for my changes.
source.

[PIG208]

Thanks for the update. I think the test for emojis needs to be more concise. Maybe try to follow the other one closer as they are testing the same aspect of autocompletion but in different contexts.

[PIG208]

Let's remove the because for this check. findsOne should be evident that we expect the option to be rendered.

[PIG208]

There is still the spacing issue — there should be a space here because: "..."

[PIG208]

Usually we don't start lines with ,. Format list of arguments using trailing commas instead.

[PIG208]

We only need the position of the first element, so this map can be simplified, similar to the other test case added.

[apoorvapendse]

Change Summary for @PIG208

Updates

1. Replaced mapping over emoji seq to generate positions list with inital position of the first emoji that satisfies the suggestions set by this and this condition and is similar to the test you wrote and follows this suggestion of yours.

2. Used trailing comma here as pointed here.

3. Fixed spacing issue here as suggested here.

Blockers

No blockers, thanks to your detailed feedback.

Review request

I have addressed all the 4 comments made in the recent review.
And would be happy to get another review when you find it convenient.

[PIG208]

Thanks for the update! Left some more comments.

[PIG208]

true is already the default value, so we can omit warnIfMissed: true here.

[PIG208]

I see that you removed the emojiSequence.take(8).map altogether. We do still need the check for (icon) => find.text(icon).findsOne(), (and additionally a find.text(emojiSequence.last)).findsNothing(), moving the check from line 320 to here should work).

Having all these check help the reader understand that only the first few options are initially visible, so it makes sense to check again later after scrolling the list. See the other test for reference, as they are quite similar.

[PIG208]

nit: indentation

This find is a part of the check and should be indented.

[PIG208]

nit: similar indentation issue here too

[PIG208]

nit: remove extra newline

[apoorvapendse]

I've done the changes mentioned @PIG208.
Please take a look at your convenience.

Thank you for being patient.

[PIG208]

Commit message nit:

Tests have been added to verify the
emoji and mention render behavior.

Normally we include tests whenever there is behavior change, so we can remove this paragraph.

Co-authored-by: Zixuan James Li <[email protected]>
Fixes: https://github.com/zulip/zulip-flutter/issues/1121

should be:

Co-authored-by: Zixuan James Li <[email protected]>
Fixes: #1121

The lines started with Something: are conventionally placed at the end as an agreed upon metadata structure for commits; there is no strict rules on what types of metadata you can have, but usually they are in this form.

Left some comments, mostly nits.

[PIG208]

Let's just remove the because: part as it is evident from the code what the check is for.

[PIG208]

nit: formatting: (icon) => check(...)

[PIG208]

Once we rename emojiSequence to emojiNames, we can rename icon to emojiName.

[PIG208]

nit: this comment does not seem quite useful, as it just repeats the code below it

[apoorvapendse]

I am unable to understand how you see the resolved link here.

This is the commit message I see on git log:

commit 64f4b77bf4812f97294ba10076a2cf36c6886ef7 (HEAD -> put-best-matches-near-fingers)
Author: Apoorva Pendse <[email protected]>
Date:   Wed Dec 11 19:35:54 2024 +0530

    autocomplete: Put best matches near input field.
    
    This commit reverses the list that was originally
    presented to the user while showing the typeahead menu.
    
    This makes sense since on mobile its easier to click
    on options closer to the input box, i.e.
    where your fingers are currently present,
    instead of pressing arrow keys on a keyboard which is
    true on a desktop setup.
    
    Hence we place the best matching options
    not at the top of the typeahead menu, but
    instead put them at the bottom for better
    reachability and convenience of the user.
    
    Tests have been added to verify the
    emoji and mention render behavior.
    
    Also mentions dependencies on #226 where
    required.
    
    Co-authored-by: Zixuan James Li <[email protected]>
    Fixes #1121.

[apoorvapendse]

Changelog @PIG208

1. Rename emojiSequence to emojiNames here and icon to emojiName here and here as per suggestion1 and suggestion2
2. Removed the redundant check as suggested.
3. Removed the redundant comment as suggested.
4. Removed because as per this.
5. Updated the formatting here as pointed out here.
6. Removed this from the commit message.
7. Incorporate the change in Fix failing tests for RawAutocomplete #1190 for both the mention and emoji tests.

I think I've addressed all your suggestions and this PR is ready for another review.

[PIG208]

I am unable to understand how you see the resolved link #1131 (review).

Oh right. That link (#1121) was converted into the full link when I drafted that message.

Let's change

Co-authored-by: Zixuan James Li <[email protected]>
Fixes #1121.

to

Co-authored-by: Zixuan James Li <[email protected]>
Fixes: #1121

[PIG208]

All my remaining comments are small. Marking this for Greg's review.

[apoorvapendse]

Thanks a lot @PIG208 for your patient reviewing.
I’m planning to compile all the formatting suggestions you’ve provided here into a document, so I can use it as a checklist for future reviews and save you the trouble next time.

I've fixed the trailing commas and changed the "biohazard" wording to "last" as advised in the recent round.
And the commit message as suggested

[gnprice]

Thanks @apoorvapendse and @PIG208 for all your work on this!

There's still an open question here of whether we want to make this change in the UX. That was Alya's feedback on the thread (in #mobile at the time because #mobile-design didn't exist yet; I've just moved it there) last month:
https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/Put.20best.20autocomplete.20matches.20at.20bottom.20of.20list.20.23F1121/near/1999854

Given that state, I think it won't make sense to invest more effort into the implementation before further design discussion.