Repository navigation
Conversation
Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Removed duplicate Speech Recognition usage description. Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Ivansss
left a comment
There was a problem hiding this comment.
I gave it a try locally, and it's really nice to see Talk on CarPlay! There are two things that need to be fixed before we can move forward, plus a few ideas to make it even better.
Blockers
- App crashes on launch: the
INPreferences.requestSiriAuthorizationcall makes the app crash because thecom.apple.developer.sirientitlement is missing. Besides that, it would be great if we only asked for Siri permission when it's actually needed. Right now everyone gets the Siri prompt the first time they open the app, even if they never use CarPlay. When tapping the "Ask Siri to make a call" button, it already asks for permission if they are not already granted, so we can leave the asking for permission just there. - Localization CI check: the "Check for localizable changes" job is failing because the new strings ("Speed Dial", "History", "No recent calls", …) aren't in
Localizable.stringsyet. Running./generate-localizable-strings-file.shand committing the result should make it happy 🙂
Things to improve
- Avatars: it would be really nice to show the conversation avatars instead of the generic person icon.
AvatarManager.shared.getAvatar(for:with:completionBlock:)already handles all the cases, andCPListItem.setImage(_:)lets you update the row once the image is loaded. - In-call view: after starting a call, either from a conversation in the Rooms tab or from the Calls tab, the Talk CarPlay app stays in the view with the call button or in the list. The call can only be ended by switching to the CallKit call screen. It would be great to have an in-call view inside the Talk app where the call can be ended directly.
- Single view instead of tabs: the two tabs overlap quite a bit. What do you think about a single list with the Siri cell on top, followed by ongoing calls, favorites, and then the other conversations?
- Plurals:
"%ld unread messages"should be a plural string so it reads nicely for a single message too. The existing"%ld conversations with unread messages"entry inen.lproj/Localizable.stringsdictmakes a good template.
Thanks a lot for your work on this, and let me know if anything's unclear or if I can help! 🙏
Removed unnecessary Siri authorization request from AppDelegate. Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Add avatar loading functionality for conversation items. Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Refactor CarPlayManager to simplify template management and remove unused variables. Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Added completion handler to pushTemplate for error handling. Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Added handling for user activity in CarPlay scene to support start call intent. Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
Signed-off-by: alexm8002 <a.m.a.martinez@icloud.com>
|
@Ivansss, thanks for the detailed feedback — all of the requested changes have now been addressed. Blockers
Improvements
I also adjusted the CarPlay Siri handling so |
Ivansss
left a comment
There was a problem hiding this comment.
Thanks for the updates, this is a big step forward! There are still two blockers, plus some smaller things.
Blockers
- Localization CI check: still failing because some new strings are missing in
Localizable.strings. Please run./generate-localizable-strings-file.shevery time strings are added or changed, and commit the result. - Joining ongoing calls:
room.hasCallmeans there's a call in the conversation, not that the user is in it. Tapping a conversation in "Ongoing calls" shows an "End call" button and there's no way to join.
Things to improve
- App name: please use
talkAppNamefromNCAppBrandingfor the title of the main list instead of the hardcoded "Talk", so branded apps show their own name. The "Nextcloud Talk" subtitle in the conversation view can be removed, or replaced with the call state (e.g. "Call in progress"). - In-call view: instead of pushing a separate view, update the conversation view itself.
CPContactTemplate.contactcan be reassigned, so the same view can show "Call", "Join call" (when a call is already going on), or a red "End call" button (e.g. aCPButtonwithphone.down.fill) once the user is in the call. This fixes the blocker above, and also the back button issue:activeCallTemplateisn't reset when leaving the view, so it can't be shown again until the call ends. - Avatars: avatars flicker on every rooms update, since all items are recreated with the placeholder and the avatar is set asynchronously via
DispatchQueue.main.async. Cached avatars are returned synchronously, so callingitem.setImage(image)directly inloadAvatarsets them before the item is displayed. I tested this locally and the flickering is gone. Also, it would be nice to show the avatars rounded, and in the conversation details view as well. - Cleanup:
callHistory()is unused and can be removed.
Great work so far, really looking forward to having this in! If you have any questions, just ping me :)
Signed-off-by: alexm8002 a.m.a.martinez@icloud.com
Summary
Adds Apple CarPlay support to Nextcloud Talk, allowing users to browse Talk conversations and start native Talk calls directly from CarPlay.
Changes
INStartCallIntentsupportScope
This PR is limited to CarPlay and Siri integration.
SIP/PSTN dial-out support, phone number dialing and related telephony features are intentionally excluded and will be submitted separately.