autocomplete: Implement new design for @-mention autocomplete items

[fombalang]

Hello everyone,

This PR implements the updated design for the @-mention autocomplete items as detailed in this Figma File https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3859-3131&m=dev

I added new contextMenuItemLabel and contextMenuItemMeta color variables to the designVariables class from the design. I also had to manually set the line height to match the design exactly.

As Greg described here, the autocomplete item only shows the user's real email address when the user has allowed their email to be visible, if not it doesn't show anything. I used the _getDisplayEmailFor() method from the profile page to get the user's real email. I am not sure if it is a good idea to have a copy of the same method here, maybe we should find a way to not have multiple copies of the same method.

I updated autocomplete tests to also check if the email(metaData) is also shown as the updated design now shows extra metadata.

I attempted to add another test to verify if the email is not shown when the user's emailAddressVisibility != emailAddressVisibility.everyone, while doing so I learned a bit more about how the app manages users and data, but to be honest I couldn't get it to work properly, and I did not want to massively change up the current test file. Still trying to wrap my head around that. This also leads me to a bug I think, which I discovered in the example_data.dart initialSnapshot method where some of the arguments from the intialSnapshot method aren't being passed down to the method, I think someone missed that when setting up the method.

Please review when you can, thank you

Fixes: #913

[gnprice]

This also leads me to a bug I think, which I discovered in the example_data.dart initialSnapshot method where some of the arguments from the intialSnapshot method aren't being passed down to the method, I think someone missed that when setting up the method.

Sure, this would be good to fix — please go ahead and add a commit with that fix if you think you already see how to do it, or start a thread in #mobile-team and we can discuss in more detail.

[fombalang]

Okay, thank you for the review, I will make the changes. For the changes such as the bug and the refactor of the email logic, should I add those into this PR, or should it be in a different PR?

[gnprice]

If it's needed for the other work you're doing in this PR, then a commit in this PR is a good way to do it.

If it's not related to what's in this PR, then a separate PR is best.

[fombalang]

If it's needed for the other work you're doing in this PR, then a commit in this PR is a good way to do it.

If it's not related to what's in this PR, then a separate PR is best.

Okay

[fombalang]

• Our designer doesn't want the "ink splash" effect on buttons. We should remove the InkWell widget and implement the on-pressed appearance specified here:

About the splash, I kept the inkwell widget, because a gesture detector doesn't show splashes, but I removed the sparkle effect, and left just the color change on tap. I also added a border radius in the splash as shown on the design, it's a bit difficult to see here but it is there. Would this work according to the design?

https://github.com/user-attachments/assets/9c190f0c-c373-4cbb-85f9-7def3b56611a

[fombalang]

I also did the updates for the case where the user has a really long name or email.

Before

After

[chrisbobbe]

Would this work according to the design?

If your code causes any noticeable differences from the design, please identify them and say why you think they are OK. This will shorten the path to deciding if the code will work. :)

It looks like you haven't pushed your latest revision to GitHub. When it's ready for another review, please do that, and comment saying that it's ready for review.

[fombalang]

It looks like you haven't pushed your latest revision to GitHub. When it's ready for another review, please do that, and comment saying that it's ready for review.

No, I haven't yet.

That is because I have been facing some problems with the ComposeAutocomplete test, I found that changing the dimensions of the avatar widget from 32 to 36 causes the test to fail. Not only that I found that there was a threshold of sizes for which the test passed, 32 -33 are good, and from 34 upward the test fails.

And I have looked at the test, and tried to do some searching as to why it might be failing, and I haven't succeeded. I did not see any obvious place where the avatar widget size was checked in the test. I also thought it might be because of an outdated golden test snapshot, but I could not find any references to a snapshot file. So I have been confused and stumped.

I was just about to comment on that before seeing your comment, wondering if anyone can help point me in the right direction.

[gnprice]

If you push the code here, and then start a conversation in #mobile-dev-help with the test failure you're seeing, that will probably be the most efficient way to get help investigating it.

[fombalang]

If you push the code here, and then start a conversation in #mobile-dev-help with the test failure you're seeing, that will probably be the most efficient way to get help investigating it.

Okay, I will do that

[fombalang]

It looks like you haven't pushed your latest revision to GitHub. When it's ready for another review, please do that, and comment saying that it's ready for review.

I have pushed my changes, please review

[fombalang]

I noticed that some other tests failed on the pipeline, some database tests to be specific.

❌ /home/runner/work/zulip-flutter/zulip-flutter/test/model/database_test.dart: non-migration tests create account (failed)
  Invalid argument(s): Failed to load dynamic library 'libsqlite3.so': libsqlite3.so: cannot open shared object file: No such file or directory
  dart:ffi                                                        new DynamicLibrary.open
  package:sqlite3/src/ffi/load_library.dart 52:27                 _defaultOpen
  package:sqlite3/src/ffi/load_library.dart 127:12                OpenDynamicLibrary.openSqlite
  package:sqlite3/src/ffi/api.dart 13:39                          sqlite3
  package:drift/native.dart 313:12                                _NativeDelegate.openDatabase
  package:drift/src/sqlite3/database.dart 79:19                   Sqlite3Delegate.open
  package:drift/src/runtime/executor/helpers/engines.dart 431:22  DelegatedDatabase.ensureOpen.<fn>
❌ /home/runner/work/zulip-flutter/zulip-flutter/test/model/database_test.dart: non-migration tests create account with same realm and userId  (failed)
❌ /home/runner/work/zulip-flutter/zulip-flutter/test/model/database_test.dart: non-migration tests create account with same realm and email (failed)
❌ /home/runner/work/zulip-flutter/zulip-flutter/test/model/database_test.dart: migrations upgrade to v2, empty (failed)
❌ /home/runner/work/zulip-flutter/zulip-flutter/test/model/database_test.dart: migrations upgrade to v2, with data (failed)

These tests didn't fail before, and they also passed locally.

I think it is related to the migration to Ubuntu 24.04 which apparently seems to have been causing a lot of issues and is now being rolled back. https://github.com/actions/runner-images/issues/10636

[chrisbobbe]

Thanks for pointing out those unrelated test failures. I've run CI again and those failures are gone; see https://github.com/zulip/zulip-flutter/pull/999#issuecomment-2417351945

Some failures remain and are related to the changes in this PR; I see a discussion in #mobile-dev-help about that.

[chrisbobbe]

If you push the code here, and then start a conversation in #mobile-dev-help with the test failure you're seeing, that will probably be the most efficient way to get help investigating it.

Thanks for starting that conversation. Here's a link, so people reading this will know the conversation is happening and where to find it: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/ComposeAutocomplete.20test.20failure.20help/near/1963058

[fombalang]

If it's needed for the other work you're doing in this PR, then a commit in this PR is a good way to do it.

If it's not related to what's in this PR, then a separate PR is best.

Hello I have created a PR for this bug, please review when you can. https://github.com/zulip/zulip-flutter/pull/1011

[chrisbobbe]

1011

Thanks; I'll add this to my review queue. 🙂

I see Greg has replied in that discussion about the test failures. Please comment here when those failures are resolved and this PR is ready for another review. 🙂

[fombalang]

I see Greg has replied in that discussion about the test failures. Please comment here when those failures are resolved and this PR is ready for another review. 🙂

Hello I have resolved the issues with the test and pushed an update here. Please review.

I did an interactive rebase and merged the test fix to the previous commit before the latest one, I think it caused it to not run the pipeline since the latest commit is still the same one from before, maybe we might have to trigger it manually.

[gnprice]

It's puzzling that GitHub didn't run the CI checks on the latest revision.

It's not because the commits are the same; they're different. Even though the last commit has the same changes in it as the last commit of the previous revision, it's still a different commit as far as Git is concerned because it has a different history. You can see here:

fombalang force-pushed the autocomplete-results-list branch from e107e03 to 5b85e89 1 hour ago

that the latest revision's tip commit is 5b85e89 and the revision before that was e107e03.

Anyway, we can run the tests locally, and as long as they pass when run locally then that confirms that they pass.

[gnprice]

I tried the "dismiss review" button on Chris's previous "request changes" review, just in case that was related to why CI wasn't running. Doesn't seem like it had an effect — still not running.

I don't even have a button to tell checks to run. Instead the "Checks" tab just looks like this:

Workflow runs completed with no jobs

It's as if there weren't any files under .github/workflows/ at all. But in reality there is; your branch doesn't touch that directory, so it has the same workflow file in it as it has in main.

:shrug: We'll just rely on running checks locally, then. If this odd GitHub behavior starts happening repeatedly, we'll have to investigate further.

[fombalang]

that the latest revision's tip commit is 5b85e89 and the revision before that was e107e03.

Okay, I see. which means it should have triggered again.

[fombalang]

🤷 We'll just rely on running checks locally, then. If this odd GitHub behavior starts happening repeatedly, we'll have to investigate further.

Okay then. Everything passes locally on my end.

[chrisbobbe]

The GitHub UI is showing that there are some rebase conflicts:

Could you please resolve those and comment here when that's done? If you need help, please ask in #git help in the development community.

[fombalang]

development community

I can just resolve using the GitHub UI right? or would that add an unwanted commit?

[fombalang]

The GitHub UI is showing that there are some rebase conflicts:

I have resolved the conflicts. I rebased my changes on top of the latest changes from main and fixed the conflicts. Please review when you can.

[fombalang]

Thanks! A few small comments from a light review, but more substantively:

The first commit—

autocomplete: Implement new design for @-mention autocomplete items

—doesn't pass tools/check. Please fix the branch so that all commits pass all tests.

Okay. I am confused though, the check runs against the latest commit, right? How do I then ensure this commit would pass the check independently? Also, I squashed the fix that I did for the test into this commit.

[fombalang]

I have made the changes, please review them when available.

[chrisbobbe]

How do I then ensure this commit would pass the check independently?

By running tools/check at that commit. 🙂 See the README on tests: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#tests

[fombalang]

I have made the latest changes. Please review when you can.

[fombalang]

Thanks, this is getting closer!

In #995 (comment), by "prep" commit I meant a commit that comes before the commit that needs it, to prepare for it. 🙂 In the branch as it is now, the helper function gets rewritten one way, but then that way is thrown out and replaced with the one we want. Instead of changing it twice, we should just change it once. That change should happen in a commit that comes before the commit that redesigns the UI, since that's the commit that needs the change.

Oh, I see now.

Also, do I have to include "prep" in the commit message?

[chrisbobbe]

No, we don't have a rule about that. :)

[fombalang]

I have made the updates and fixed the conflicts and reordered the prep commit. Also added a fix for the contextMenuItemText color variables as an NFC commit. I am a bit confused if a change like this would be considered an NFC commit? 🤔

[chrisbobbe]

tools/check fails at this commit:

autocomplete_test [nfc]: Rename findNetworkImage to findAvatarImage

It looks like that commit is doing more than it says in the commit message. It's also changing some tests so that they expect an email address to appear in an autocomplete item. That's something we want to do, but not at this commit, before the app code has actually been changed to show the email address. We should add that expectation in the same commit that satisfies it:

autocomplete: Implement new design for @-mention autocomplete items

[chrisbobbe]

Also added a fix for the contextMenuItemText color variables as an NFC commit. I am a bit confused if a change like this would be considered an NFC commit? 🤔

Thank you!

Looks like it does make a functional change, although a small one. The lerp method is used to fade the colors smoothly over 200 milliseconds when transitioning between light mode and dark mode. So I imagine the bug you've fixed here could be reproduced on main this way:

1. Open the app on Android on main
2. In system settings, switch to light mode and schedule the app to switch to dark mode a minute or so after the current time (or vice versa)
3. Open the message list
4. Long-press a message to bring up the action sheet
5. While watching the text in the action sheet, wait for the scheduled switch from light to dark (or vice versa) that you set up in step 2
6. Notice that the text's color goes in the wrong direction over the 200ms animation, then suddenly snaps to the correct color when the animation is finished

I haven't tried this myself. This is quite a small bug, but one we're glad to fix! No need to spend time reproducing it; all you need to do is change your commit message to remove [nfc]. 🙂

[fombalang]

That's something we want to do, but not at this commit, before the app code has actually been changed to show the email address. We should add that expectation in the same commit that satisfies it:

Yeah. I think during my rebase I must have messed it up somehow, when I was reordering the commit structure.

[fombalang]

I have updated the commits, removed the NFC label from the color fix, and adjusted the other commits.

I ran tools check on each commit using git checkout <commit-hash> -- . && tools/check && git checkout -- . Not sure if this is the right command to use, but all the commits passed.

[chrisbobbe]

The command I use, from a feature branch that tracks main, is:

git rebase -i --exec 'tools/check --diff @~'

Are you familiar with interactive rebase? This sets up an interactive rebase with an "exec" step that runs the command tools/check --diff @~ at each commit. (The --diff @~ part makes tools/check run only on files that differ from the previous commit.)

[fombalang]

The command I use, from a feature branch that tracks main, is:

git rebase -i --exec 'tools/check --diff @~'

Are you familiar with interactive rebase? This sets up an interactive rebase with an "exec" step that runs the command tools/check --diff @~ at each commit. (The --diff @~ part makes tools/check run only on files that differ from the previous commit.)

Okay, I will try this and see. Yes, I am familiar with interactive rebase, although I am mainly used to working in the Android Studio UI and not the terminal.

[fombalang]

Thanks! LGTM except for one tiny nit below, and I'll mark this for Greg's review.

Alright, great! Thank you very much for your help reviewing!

[fombalang]

Generally this all looks good. Various small comments below.

Alright, thank you for the corrections. I will implement these changes.

[fombalang]

This should also get tests. No need to check things like that the font size or the colors etc. match the design — that sort of test wouldn't really add value beyond duplicating what's in the implementation. But there is a bit of logic here that would be good to test:

Yeah, I originally intended to add some test cases similar to the ones you describe, which is what initially led me to find the small bug with the emailAdressVisbility. But, I couldn't figure out exactly how the account stores work to properly override the emailAdressVisbility and create the test cases. But I will get back to it and see how to add the test cases.

[gnprice]

Cool, sounds good.

But, I couldn't figure out exactly how the account stores work to properly override the emailAdressVisbility and create the test cases. But I will get back to it and see how to add the test cases.

Yeah, if you browse through the other tests in test/widgets/, you'll see examples of using that eg.initialSnapshot function you sent a fix for, and that should help you get going. If you have questions after giving that another try, please go ahead and ask in #mobile-dev-help on chat.zulip.org.

[fombalang]

Cool, sounds good.

But, I couldn't figure out exactly how the account stores work to properly override the emailAdressVisbility and create the test cases. But I will get back to it and see how to add the test cases.

Yeah, if you browse through the other tests in test/widgets/, you'll see examples of using that eg.initialSnapshot function you sent a fix for, and that should help you get going. If you have questions after giving that another try, please go ahead and ask in #mobile-dev-help on chat.zulip.org.

Hello. I continued working on this. I have completed all the other issues you commented on. I made a thread on the #mobile-dev-help with some of my blockers. Here is the link. https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/ComposeAutocomplete.20test.20emailAddressVisbility.20help/near/1985612

[gnprice]

I've just merged #1069, which touched some of this same code. @fombalang when you push your next revision, don't worry about rebasing it forward past those changes — just push it without rebasing, and I can take care of rebasing it on top of those changes and resolving the conflicts.

(Ordinarily we expect contributors to handle resolving rebase conflicts; it's an important Git skill. But these conflicts are unusually complex, and have more to do with my #1069 than with your PR or with the routine need for resolving conflicts.)

If you use git rebase -i for revising your branch (which you probably will want to do), you can say git rebase -i --keep-base to tell it not to rebase forward, and to instead focus on what rebase -i specifically is good at.

Separately, even if you're still working on the tests, I'd encourage you to go ahead and push the revision you have that addresses all the other review comments. That way I can re-review those changes while you're still working on the tests.