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 bulk download option for Seasons and MusicAlbums #299

Closed
wants to merge 1 commit into from

Conversation

CarlosOlivo
Copy link
Contributor

Now that #276 has been merged, this makes more sense, at least until the native counterpart exists.

Closes #290

  • Music Albums
    JRz9cWWTy1

  • TV Shows > Season
    cSqkHvudgb

@nielsvanvelzen
Copy link
Member

I'm not a fan of the JavaScript part; we should add support for bulk downloads in jf-web and communicate via nativeshell. Your injection can easily break with each web release. The app should support multiple Jellyfin versions (10.6->10.7 is the exception because of the ES6 migration).

@CarlosOlivo
Copy link
Contributor Author

Your injection can easily break with each web release.

Maybe... If

  1. Someone decides to refactor jellyfin-web to ES6² - Electric Boogaloo and change the routes again
  2. Someone decides to introduce breaking changes in jellyfin-apiclient-*

The app should support multiple Jellyfin versions (10.6->10.7 is the exception because of the ES6 migration).

From jellyfin-web

From jellyfin-apiclient-javascript

So this code can be compatible from version v10.4.0 adding the route selector itemdetails.html or from version v10.6.0 without making changes

we should add support for bulk downloads in jf-web and communicate via nativeshell.

You're free to do so ;)

@nielsvanvelzen
Copy link
Member

Next release definitely doesn't work with 10.6<=. And sure, those classes might not have changed much but what if we change those buttons to Vue components (something that is on the roadmap) and the classes change? What if we decide change the url? Sure both might not have happened in the past but nobody says it won't happen tomorrow.

We should avoid injecting stuff as much as possible. This PR adds a lot of magic and although it adds a much requested feature, I;m against merging it unless properly implemented. To do that changes should be made in both the nativeshell and jf-web, which likely will be 10.8 >= if it happens.

@CarlosOlivo
Copy link
Contributor Author

And sure, those classes might not have changed much but what if we change those buttons to Vue components (something that is on the roadmap) and the classes change? What if we decide change the url?

When the switch to jellyfin-vue happens I'm pretty sure that also in this case the injection code will break, which is what happened in the change from v10.6.0 to v10.7.0, after all jellyfin-android in its current state is just a web browser with userscripts (Eg chrome.cast.js - unstable behavior, NavigationPlugin.js - doesn't work with dialogs) + some native code (ExoPlayer + Android Auto integration)

Next release definitely doesn't work with 10.6<=
To do that changes should be made in both the nativeshell and jf-web, which likely will be 10.8 >= if it happens.

Then your statement

The app should support multiple Jellyfin versions (10.6->10.7 is the exception because of the ES6 migration).

doesn't make any sense. Probably the next time gonna be:
The app should support multiple Jellyfin versions (XY.A->XY.B is the exception because of the jf-web Vue rewrite).

This PR adds a lot of magic and although it adds a much requested feature, I;m against merging it unless properly implemented.

  • This isn't magic, the API it's documented here and here.
  • This is magic and will probably break on the next jellyfin-web change like before. Following your way of thinking the Chromecast integration should be removed as it has not been properly implemented natively.

I know about the existence of jellyfin-vue and jellyfin-android compose branch but to be honest they are both in their barebones, so they are not an "imminent risk".

Either way, you are free to do your proper implementation. Or to hold this, QualityOptions and every new feature until v10.8 ;)

@Maxr1998
Copy link
Member

And sure, those classes might not have changed much but what if we change those buttons to Vue components (something that is on the roadmap) and the classes change? What if we decide change the url?

When the switch to jellyfin-vue happens I'm pretty sure that also in this case the injection code will break, which is what happened in the change from v10.6.0 to v10.7.0, after all jellyfin-android in its current state is just a web browser with userscripts (Eg chrome.cast.js - unstable behavior, NavigationPlugin.js - doesn't work with dialogs) + some native code (ExoPlayer + Android Auto integration)

There are, however, no definite plans that jf-vue is going to replace jf-web. For now, it's developed alongside as an alternative web-based client. Even if it's ever decided that jf-vue replaces jf-web, it'll still be quite a while until that happens - we would need to reach feature parity first, and afaik it isn't even decided whether that'll be a target for jf-vue.
The main difference between the injected mentioned code above and your PR is that the former is necessary for compatibility and device integration. Some APIs in jf-web were specifically designed for that, while other code lives in this repo because it wouldn't fit into jf-web. The download code in this PR could just as well be added to jf-web, and then supported with much less hassle in here.

This PR adds a lot of magic and although it adds a much requested feature, I;m against merging it unless properly implemented.

  • This isn't magic, the API it's documented here and here.
  • This is magic and will probably break on the next jellyfin-web change like before. Following your way of thinking the Chromecast integration should be removed as it has not been properly implemented natively.

While I agree that calling it 'magic' maybe isn't the best choice for it, I share Niels' sentiment that the added code is out-of-scope for this app, as already outlined above. And you're right with the Chromecast part, if the code hadn't existed yet and wasn't already part of the old app, I would never have added it in the current form to the rewritten app. It's a mess that I feel bad about having in here and I'm looking forward to the day when I can drop it.

I know about the existence of jellyfin-vue and jellyfin-android compose branch but to be honest they are both in their barebones, so they are not an "imminent risk".

Right, compose is still early, because we were waiting for stability the rewritten java-apiclient (which depends on 10.7 being fully released), and I don't have much time to work on it right now anyway. You're free to chime in and start working on it if you don't want to wait of course 😉

Either way, you are free to do your proper implementation. Or to hold this, QualityOptions and every new feature until v10.8 ;)

I don't really understand the hostility in your comment. Niels made a topical comment suggesting an alternative approach, there's no need to reply with snarky remarks.
Back to topic:

  • You could just as well port the code over to jf-web yourself
  • It's not about holding back features, but about scope and the correct location for changes
  • 10.8 was mostly mentioned because we can't just publish versions as we please for jf-web, but are bound to a version release cycle that is decoupled from jf-android
  • Your quality options PR is completely unrelated to this and the delays are mostly caused by me just being busy and the sheer size of that PR

Regardless, I think everything is said for this discussion. I'll close this PR for now, but I'm happy to re-open it if you can suggest a different approach for this change. Preferably, if you decide to do your changes in jf-web, you can ping me there so that I can review your changes.
Closing thoughts: this project is maintained by volunteers in their free time, contributions are welcome but there's no guarantee they'll always be accepted. If they aren't, it doesn't imply any ill intent.

@Maxr1998 Maxr1998 closed this Feb 10, 2021
@Morgan-6Freedom
Copy link

Hello. This is too bad that the implementation hasn't been merged. It could have been a solution for the most wanted feature. If we could be able to download an album or better a collection or a playlist, we could be able to use Jellyfin in offline mode.
If someone has a new idea of how to implement it, it woud be great !
Thank you everybody for your hard work !

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.

Download a whole season at a time
4 participants