-
Notifications
You must be signed in to change notification settings - Fork 127
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
Fix message completion trigger to work anywhere in the message #3696
Fix message completion trigger to work anywhere in the message #3696
Conversation
Can you try testing out this new behavior within the |
I've run the testing on |
As in write a brand new one just for this special case and assert that the suggestions show up |
Oh! Do you mean I need to write a new unit test for this? *I apologize If my English is unclear. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## develop #3696 +/- ##
===========================================
- Coverage 78.71% 78.68% -0.03%
===========================================
Files 792 792
Lines 67839 67863 +24
===========================================
- Hits 53399 53398 -1
- Misses 14440 14465 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Why did the CI/CD fail, And how can I solve the problems? |
Only the enterprise tests failed which is to be expected as you don't have access to that code, it's private. |
I got it! thank you for your reply |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @vickcoo, thanks for opening your first contribution 🎉
I've just tested it out, but I think we probably need to refine it a bit more. If you type @
into the composer but don't want to mention someone it now always shows a suggestion and if you subsequently try to @mention
someone it uses that first @
as the trigger so your filtering doesn't work. If we're going to allow mentioning from anywhere in the message, we really need to use the caret position as the trigger rather than iterating through all the words. See an example of the unexpected behaviour here:
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-27.at.10.12.40.mp4
Hi @pixlwave! thank you for your feedback, here are a few ideas. Solution 1
Solution 2 |
Solution 1 sounds super to me if it's possible! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. I just tried this out and it is really nice! 🤩
I've added a couple of comments about the new algorithm inline, but one other thing I noticed was that if you accept a suggestion in the middle, the cursor jumps to the end of the message which might be a bit annoying:
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-02-03.at.11.39.14.mp4
ElementX/Sources/Screens/RoomScreen/ComposerToolbar/CompletionSuggestionService.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/Screens/RoomScreen/ComposerToolbar/CompletionSuggestionService.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/Screens/RoomScreen/ComposerToolbar/CompletionSuggestionService.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/Screens/RoomScreen/ComposerToolbar/ComposerToolbarViewModel.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/Screens/RoomScreen/ComposerToolbar/CompletionSuggestionService.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks and works great! There's just a small compile error to fix on the unit tests and then this will be ready to merge 👏
Edit: It's coming from SwiftGen. Make sure to swift run tools setup-project
to ensure you have all the homebrew dependencies set up and then if you build the project you should see a change to the GeneratedMocks.swift
that also needs to be committed.
ElementX/Sources/Screens/RoomScreen/ComposerToolbar/CompletionSuggestionService.swift
Outdated
Show resolved
Hide resolved
…SuggestionService.swift Co-authored-by: Doug <[email protected]>
|
Thank you very much for your contribution @vickcoo. The tests passed (but appear to fail as the coverage upload fails) so I'm going to merge this PR now 🎉 |
You're welcome! I've just been using this on todays Nightly build and it rocks 🙌 |
This PR resolves issue #3522
Pull Request Checklist
UI changes have been tested with: