Fix LibraryItem deep links #51
Conversation
| optional string meta_details_videos = 1; | ||
| optional string meta_details_streams = 2; | ||
| optional string player = 3; | ||
| optional ExternalPlayerLink external_player = 4; |
There was a problem hiding this comment.
I don't see a use case where this would be needed, so not sure if we need a separate struct just for a variable that will not be used.
There was a problem hiding this comment.
It was for simplicity and clarity, to match what is implemented in core so we don't have to come back to add things and overall just to not be confused
There was a problem hiding this comment.
But since the structure is the same, what's the point of having a separate object for it? Having the same one just makes it easier to deal with deeplinks, since in many places you have either MetaItem or LibraryItem. And deelinks property having the same type makes it easier to work with in strongly typed languages, rather than each time checking the type to access fields.
There was a problem hiding this comment.
The structure are not the same, LibraryItemDeepLinks has external_player MetaItemDeepLinks doesn't
I don't see how it makes it easier, for me it's just more confusing because it does not match core
There was a problem hiding this comment.
Hmm, you've removed it and then added again external_player, which like I mentioned doesn't have any use case in android (I'd even say that this is variable is misplaced and is breaking SRP, since it most likely should be used from a Stream rather than from library item).
Having unused variables just increases serialization/deserialization burden on low end devices when it needs to pass from rust to kotlin, and it is especially relevant as that's why protobuf was used to reduce the amount of data and objects travelling through rust-kotlin. But even with protobuf we should be vary of passing too much data (ie, this library item change, when fetching multiple pages of library will need to convert hundreds of items on each field event)
There was a problem hiding this comment.
I feel like you are arguing for the sake of it and i don't follow your logic so i will close this and make a new PR with only the fix for the player deeplink of the library item
There was a problem hiding this comment.
Well it's not arguing. Just trying to provide a review and insighs on what to be wary of when adding something to the structures.
Like I mentioned LibraryItem and MetaItemPreview are very sensitive structures, since we have to deserialize a ton of them on each event, so it's best to keep them as lean as possible and always be vary of adding new things to it's structure, as from experience it affects the performance quite a bit. So it's best to always think what variables will be used and needed for them (ie. we don't pass all the data from LibraryItemState, only what's needed and used)
tymmesyde
force-pushed
the
fix/library-item-deep-links
branch
from
39f88f5 to
c53612d
Compare
LibraryItem was using MetaItemDeepLinks instead of LibraryItemDeepLinks
The trait implementation did not passed the StreamsItem to the deeplink used for the player field
Moved the deeplinks to a designated file