Skip to content
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

Add Static Location and Live Location Support #3531

Open
wants to merge 61 commits into
base: develop
Choose a base branch
from

Conversation

nuno-vieira
Copy link
Member

@nuno-vieira nuno-vieira commented Dec 13, 2024

🔗 Issue Links

Resolves https://linear.app/stream/issue/IOS-578/location-attachment

🎯 Goal

Adds support for static and live location attachments in the Low-Level Client SDK.

The UI has been implemented in the Demo App to demonstrate how to use the new location APIs.
A guide should be written in the stream documentation on how to use the location APIs.

📝 Summary

New APIs:

  • ChatChannelController
    • sendStaticLocation() - Sends a static location message to the channel.
    • startLiveLocationSharing() - Starts a live location-sharing message in the channel.
    • stopLiveLocationSharing() - Stops sharing the live location message in the channel.
  • ChatMessageController
    • updateMessage() - Updates the message partially. (It was missing from the SDK)
    • updateLiveLocation() - Updates the message's live location attachment if it has one.
    • stopLiveLocationSharing() - Stops sharing the live location attachment if it has one.
  • CurrentChatUserController
    • updateLiveLocation() - Updates the location of all active live location messages for the current user. Internally, it uses the Message Controller to update each message that has a live location attachment.
  • CurrentChatUserControllerDelegate
    • didStartSharingLiveLocation() - Notifies whenever the current user is sharing a live location.
    • didStopSharingLiveLocation() - Notifies whenever the current user stops sharing a live location.
    • didChangeActiveLiveLocationMessages() - Notifies whenever the live location messages update.
  • Throttler
    • The Throttler was part of the UI SDK somehow, so it was moved to the LLC and made public like the Debouncer.
  • ChatMessage
    • staticLocationAttachments - Returns the attachments of .staticLocation type.
    • liveLocationAttachments - Returns the attachments of .liveLocation type.
  • AttachmentType
    • .staticLocation
    • .liveLocation
  • ChatMessageLiveLocationAttachment & LiveLocationAttachmentPayload
  • ChatMessageStaticLocationAttachment & StaticLocationAttachmentPayload

Out of Scope

  • End Time: The backend does not support this out of the box yet, and there's no contract yet for this. But we should add this as soon as possible since all of the common apps have this feature.
  • Multi-Device: At the moment, there is no protection against the user sharing the location between multiple devices. The location will be shared from both devices since, at the moment, we don't have a way to attach the device ID or the device model to an attachment. Since this is an edge case, it can be left for future improvement.

🛠 Implementation

TODO

🎨 Showcase

Demo:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-03.at.17.38.38.mp4

🧪 Manual Testing Notes

TODO

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change should be manually QAed
  • Changelog is updated with client-facing changes
  • Changelog is updated with new localization keys
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

@nuno-vieira nuno-vieira added 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK ✅ Feature An issue or PR related to a feature labels Dec 18, 2024
Copy link
Contributor

@martinmitrevski martinmitrevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't test a lot, since a had constant crashes (shared details on Slack). Let's figure that out first, then I will do another round.

DemoApp/Screens/DemoAppTabBarController.swift Outdated Show resolved Hide resolved
userName: String
) {
if let existingAnnotation = userAnnotation {
UIView.animate(withDuration: 5) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to do 5 second long animation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly to have a smooth animation. It simulates the pin constantly being moved since the animation is always running. It looks quite good this way. But it is also possible to have an hard animation like WhatsApp, but for now this is quite smooth

client?.registerAttachment(LocationAttachmentPayload.self)

// Custom Attachments
if AppConfig.shared.demoAppConfig.isLocationAttachmentsEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be always on, since it's the demo app. Our config screen becomes very complex at this point.

Copy link
Member Author

@nuno-vieira nuno-vieira Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was disabled by default before, but since we now support locations out of the box, I can enable it by default. That said, I prefer not to do so because it involves sensitive data. Customers could easily misuse it and potentially spam our server.

@nuno-vieira nuno-vieira marked this pull request as ready for review January 3, 2025 17:40
@nuno-vieira nuno-vieira requested a review from a team as a code owner January 3, 2025 17:40
@nuno-vieira nuno-vieira changed the title [WIP] Add Static Location and Live Location Support Add Static Location and Live Location Support Jan 3, 2025
…when the user should start and stop location sharing
@nuno-vieira nuno-vieira force-pushed the add/location-attachments branch from 5e5a9ab to 70218c2 Compare January 3, 2025 22:01
Copy link

github-actions bot commented Jan 3, 2025

1 Warning
⚠️ Big PR
1 Message
📖 There seems to be app changes but CHANGELOG wasn't modified.
Please include an entry if the PR includes user-facing changes.
You can find it at CHANGELOG.md.

Generated by 🚫 Danger

@Stream-SDK-Bot
Copy link
Collaborator

Stream-SDK-Bot commented Jan 3, 2025

SDK Size

title develop branch diff status
StreamChat 6.98 MB 7.07 MB +93 KB 🟢
StreamChatUI 4.77 MB 4.77 MB -1 KB 🚀

Comment on lines +898 to +905
/// In order to
///
/// - Parameters:
/// - location: The location information.
/// - text: The text of the message.
/// - extraData: Additional extra data of the message object.
/// - completion: Called when saving the message to the local DB finishes,
/// not when the message reaches the server.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a formatting issue

Comment on lines +844 to +851
public func sendStaticLocation(
_ location: LocationAttachmentInfo,
text: String? = nil,
messageId: MessageId? = nil,
quotedMessageId: MessageId? = nil,
extraData: [String: RawJSON] = [:],
completion: ((Result<MessageId, Error>) -> Void)? = nil
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Planning to add similar functions to Chat?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move most of the logic in these action methods to separate handler/sender for making them reusable for Chat?
TypingEventsSender, ReadStateHandler?

/// - attachments: The attachments to be updated.
/// - extraData: The additional data to be updated.
/// - completion: Called when the server updates the message.
public func updateMessage(
Copy link
Contributor

@laevandus laevandus Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partialUpdateMessage/partialMessageUpdate for matching to the common naming pattern?

Copy link
Member Author

@nuno-vieira nuno-vieira Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I did not put this name is because editMesasge is kinda already a partial update as well. All the existing data will remain the same. At least the end result will be the same, even though internally we provide the non-changed data to the server.

The only real diff between the editMessage, and updateMessage, is that one is scheduled, and the other one is not 🤔

final class ActiveLiveLocationAlreadyExists: ClientError {
let messageId: MessageId

public init(messageId: MessageId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal?

@@ -1766,7 +1955,7 @@ private extension ChatChannelController {

// MARK: - Errors

extension ClientError {
public extension ClientError {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping it internal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Feature An issue or PR related to a feature 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants