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

Always deal with MediaItem at the LayoutMedia layer #2780

Draft
wants to merge 1 commit into
base: livekit
Choose a base branch
from

Conversation

hughns
Copy link
Member

@hughns hughns commented Nov 14, 2024

That way we only convert to the view model when we do LayoutMedia => Layout

That way we only convert to the view model when we do LayoutMedia => Layout
Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

I see how this simplifies things. If I understand correctly one can phrase the PR as shifting the transition from Media to MediaViewModel into a lower level.
Passing the full MediaItem instead of just the ViewModel increases the access however. I think the reason for creating it like this is to have as limited access to the MediaItem as possible in the CallViewModel.

The vm should represents this "smallest set of data".

What is the win we get by passing the whole MediaItem?
If there is too much redundancy I think we might need to evaluate if MediaItem is already a View Model. And we should copy all UserMediaViewModel code into UserMedia.

Is there any data from the MediaItem that we need in the Layout classes?

@hughns hughns marked this pull request as draft November 14, 2024 13:35
auto-merge was automatically disabled November 14, 2024 13:35

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants