Jellyfin audiobook download

lysanntranvouez commented 3 weeks ago

Purpose

Adds a feature "Download from Jellyfin", similar to "Download from URL".

UI to log in to a jellyfin server
Lists all audiobooks found on the server (all "book" libraries, and all folders & audiobooks in those)
Selecting an audiobook starts the download process, hooking into the existing single file download

Related tasks

1205

Approach

For the most part it doesn't touch existing features, only adds new views, view models & controllers
Only exception: Extracted single file download (by URL) into a separate service, so it is reused for the jellyfin download

Things to be aware of / Things to focus on

I haven't programmed Swift or iOS for 8(?) years. I have never done SwiftUI before.
I tried to stay similar to the existing patterns I spotted (presentation flows, use of MVVM), but I'm not super familiar with how things are done on iOS these days, so there's likely room for improvement.

Draft status

This is still very much a draft at this stage:

[x] UX has varying amounts of quality
[x] There is zero error handling
[x] There are no unit tests

Other missing things before I consider this ready for merge:

Must have:
- [x] Option to remember login information + add option to sign out (forget login info) in settings (and probably the jellyfin download view itself?)
- [ ] Document the added 3rd party libs
Should have:
- [ ] A bit of a question - I think it would be good to have some kind of a page before the download, to see the size + a download button. So that the user initiates the download very intentionally. Or else, books and folders need to be distinguished better.
Nice to have:
- [ ] Ability to queue several downloads

Super optional but nice to have:

[ ] Ability to download an entire folder and save the files into a folder with the same name

Screenshots

Simulator recording of an earlier version: https://github.com/user-attachments/assets/1ad1fe56-5d6a-4314-93e8-583229090d4a

lysanntranvouez commented 1 week ago

Sorry for the late reply, I've been quite busy the last couple weeks.

I left a few comments, and I think that if you split off the refactoring of the single download service, we can merge that other PR in quite easily.

Double checking if I understand you correctly. Do you want me to remove the single file download service refactoring from this PR, so that we can merge this PR? Or are you asking for something else?

I'm asking because without the refactoring I'd have to go back to the more basic version of the file download for jellyfin, which also means almost no user feedback. I'd rather not reimplement the user feedback you've already made for the download from URL flow.
But I can fix the issues with my refactoring based on your feedback, like how the service is registered.
Were there any other issues I need to address?

Let me know if I misunderstood anything or if you disagree.

GianniCarlo commented 1 week ago

No worries @lysanntranvouez , I also took my time to go over the PR 😅, regarding the single file download service refactoring, that is something that definitely needs to be included, I just meant that those changes are very straight forward, and could be split from this PR into a base PR that can be merged into develop fairly quickly, as there were other items that you wanted to address (storing login info and sign out), but it's fine to leave it in this PR.

Regarding storing the credentials info, any thoughts or preferences? I was reading that the access token is live until it is revoked by signing out, so it could be stored in the keychain without any problems 🤔, there's a KeychainService class, which is a bit restricted on just storing and retrieving the access token for our servers, but for the Watch app work that I'm doing, I had to open that up a little in another branch, so I was thinking we can update it to store the jellyfin access token too, or also we could just have a new struct that conforms to Codable with properties for the access token, user id and server name, so we transform that into Data and store that in the keychain 🤔

lysanntranvouez commented 1 week ago

Yeah I have some work in progress changes to store the credentials in the keychain.
I kind of shoehorned the data into the available fields... But I like your suggestion of a Codable structure better, so I'll adjust that.

Could you point me at your other branch? I had made another class to store my keychain data. But it would be much better to have one place to interface with the keychain, since its API is a bit... special :D

GianniCarlo commented 1 week ago

Re my branch, I haven't pushed it yet, but I'll split off my changes into a separate branch and push that, will ping back when that's up (at the latest on Monday, not sure if I'll be able to get to it tomorrow)

GianniCarlo commented 1 week ago

@lysanntranvouez I put up my changes for the keychain service here, I'll merge it once the tests run, and the idea would be to just add a new key to the enum, and use the get / set functions with the codable struct, let me know if you have any questions (Re automockable, Sourcery was giving problems because of the overload with the generic so I just removed it)

lysanntranvouez commented 5 days ago

@GianniCarlo

Thank you!

let me know if you have any questions (Re automockable, Sourcery was giving problems because of the overload with the generic so I just removed it)

When the unit tests run, do they use an isolated environment, or can they pick up values stored in the keychain on the target device/simulator (or previous test runs) when we don't use a mock?

GianniCarlo commented 5 days ago

@lysanntranvouez yeah I was worried a bit about that, as the AccountService tests also make use of the keychain service, I honestly don't have any assurances that the keychain is not shared between runs even when parallelization is disabled for the tests (I did try that at some point, but it didn't play nice with the Github actions), but as soon as one of the tests fails, I'll probably end up creating a manual mock for the service

lysanntranvouez commented 5 days ago

I've updated the PR with the ability to save the connection, and to remove the saved connection.

And I've addressed all of your comments, I think.

(But there's still more I want to do before considering this done.)

TortugaPower / BookPlayer