-
-
Notifications
You must be signed in to change notification settings - Fork 285
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
Start work on offline download support #1065
base: main
Are you sure you want to change the base?
Conversation
I'm glad that this is able to pick up starting from my latest work, but is there a reason why we didn't ask on the previous draft PR to continue the work elsewhere? |
I'm sorry I don't quite understand what you mean by:
|
There is already a pre-existing PR for working on downloads. While it has been a while since work was done on it, and that's okay, now we have 2 PRs for the same feature. I'm going to have to ask you to communicate with @JacobKingDev about taking over the feature, or at the very least it would have been nice ask if there was interest to continue the previous work. Even if after a few days there isn't an answer, then another PR would have been fine. |
@LePips I have asked in the previous PR, but I'm yet to get a response. In a mean time, I've completed a very rough version of my idea of the downloads section. It can be enabled in the experiments section, and I'd love some feedback on how you think it looks and feels. The code for the UI is largely a verbatim copy of the ItemView group (there is a lot of duplicate code that can be removed) the reason I did it like this is I wanted to match the current design as much as possible. If you have any feedback on the rest of the code or the way I've decided to handle the downloads, please let me know. What currently works:
What doesn't work (yet) / what I haven't got around to:
|
It will take me a while to look at this as I work on the video player again. Due to that, I see that some things will probably have to be rewritten, but I don't anticipate it to be too much. I will also have to think a lot about the overall architecture that my initial work had and whether it needs to be redone. |
With two forks already and multiple folks wanting to pitch in (myself included), I think it would be helpful to have these PRs put into a feature branch until it's ready for |
@chickdan I would like that. If you want to make a feature branch, I would be happy to contribute. |
I'd also like to contribute if a feature branch could be created. I am unsure how ios will handle transcoded downloads and if it will have the same issue as the ExoPlayer on android (transcoded downloads have malformed data from ffmpeg.) More details can be had here: jellyfin/jellyfin-meta#66 |
Seems like BenStokamns repo has gone stale but I would love to help if he is still working on it. |
Hi everyone, good to see so much interest for this feature. Currently, I am on holiday until late August. The way the offline feature is currently written doesn't really sit well with me and would need a significant refactor to make it work nicely with the existing code base. If you still wish to contribute you can make pull requests to the downloads branch in my repo which I will still check and merge. |
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.
I finally did a quick glance at this and see that a lot of code was copied and just renamed with the prefix Offline_
. Those views are redundant and we need to find a solution that uses current views since they can be created with a BaseItemDto
which should be available when downloaded.
At the same time, don't worry about implementing much for the video player as I am making changes in #1203 to make playing across the board easier.
Hi Ethan thanks for your response it reminded me that I should make some time to look at this again sometime soon. To address your comments, yes, as I mentioned a lot is copied and is redundant, this was because I wanted a working version quick and dirty before a holiday. That being said, yes views can be created based on BaseItemDto's but any functionality derived from that class automatically attempts to retrieve data from the server which is of course not available when offline. So we would have to find a solution for that that covers both online and offline usage. What that would look like I do not know. If you have any suggestions I would love to hear them. |
Also small side note. I plan to discard almost all changes I made as I have since realized that I made a huge mess. |
That sounds good to me, especially since there were many changes since the beginning of this PR and even more especially with app navigation/server connection expectations. If you would like, you can wait until after #1203 so I can assist with how to use the video player. |
This PR was inspired by @JacobKingDev. It improves upon the current framework for media downloading provided by @LePips. It is far from finished, and I intended to improve on it in the coming weeks. I would love any feedback, as this is the first time I'm working with Swift, so my code is probably riddled with nonsensical rubbish. That being said here is a list of the changes I've implemented.