-
Notifications
You must be signed in to change notification settings - Fork 459
feat(ui): Add active call information to the current notification item #6668
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
Merged
+888
−25
Merged
Changes from 9 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
a49c4cd
feat(ui): Add active call information to the current notification item
BillCarsonFr e780afc
fixup: test compilation
BillCarsonFr 7303a60
fixup: remove unwanted println!
BillCarsonFr 2daa931
fix test, now rtc notification is updated in two steps
BillCarsonFr e3cfa0e
update changelog entries
BillCarsonFr d0edb2e
test: Use track_caller for extrated assert util
BillCarsonFr 87d1bbb
Merge branch 'main' into valere/rtc/active_call
BillCarsonFr cff75f4
post merge fix, timeline::new is async
BillCarsonFr 412b5df
post merge: fix too many args in constructor
BillCarsonFr 720a66b
review: proper use of the event builder framework
BillCarsonFr d49cc94
fixup: example not compiling
BillCarsonFr f0ef719
fixup style
BillCarsonFr e893aeb
review: add back new line
BillCarsonFr 857d1d3
Merge branch 'main' into valere/rtc/active_call
BillCarsonFr 6794d29
fixup: post merge fix conflicting import
BillCarsonFr ecda844
review: Avoid unnecessary clones and bad unreachable!
BillCarsonFr dfaee95
review: remove stale comment
BillCarsonFr 56c6fe4
fixup clippy
BillCarsonFr b0f3fd6
review: clarify comments
BillCarsonFr e49eecc
fixup clippy
BillCarsonFr 1dd6701
Merge branch 'main' into valere/rtc/active_call
BillCarsonFr 9b56dfe
review: rename `lk_focus` to `set_livekit_focus`
BillCarsonFr efd443d
Merge branch 'main' into valere/rtc/active_call
BillCarsonFr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| The `TimelineItemContent::RtcNotification` now contains additional fields for when the notification is related to | ||
| and active call. The new fields are `active_members` (if not empty then the call is active), `call_start_ts_millis`, `is_joined`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| The `TimelineItemContent::RtcNotification` now contains an optional `ActiveCallInfo` struct, which provides information | ||
| about the active call associated with the notification. | ||
| This allows to render in the timeline more detailed context about the active call (members, join state, etc...) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What does this mean? What are
active_members, the thing in theActiveCallInfo?Is this the event ID which activated a call and contains the list of members which are participating in the call?
If so, isn't
ActiveCallInfothe right place for this event ID?Can we have
SomeRTC event ID, butNonefor theactive_callfield? I think the answer is yes, but not really sure why this would be so.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.
It was initially like that but I did a refactor because it didn't fell like it belongs to it.
Currently there is nothing tying an active call to a notification, the trick for now is to use the last notification event in the timeline as the anchor.
When the notification_event_id was part of the
ActiveCallInfo, it was initially created with None. Then in the timeline controller when we have the last notification event we had to mutate theActiveCallInfoto set the event_id now that we know it.And also at every room_info stream update we were getting a new
ActiveCallInfoupdate with None event_id, so had to mutate it again before saving.The
call_started_ts_millisalso suffers from the same problem :/, I was close to remove it from the ActiveCallInfoThere 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.
I clarified the comments b0f3fd6