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

Jellyfin audiobook download #1208

Merged

Conversation

lysanntranvouez
Copy link

@lysanntranvouez lysanntranvouez commented Oct 29, 2024

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

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:

  • UX has varying amounts of quality
  • There is zero error handling
  • There are no unit tests

Other missing things before I consider this ready for merge:

  • Must have:
    • 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.

The below would be nice to have, but I think we can skip them for the initial PR. It's quite chonky already.

  • Nice to have:
    • Ability to queue several downloads
    • Search function in the jellyfin library

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/39b15e1e-301f-4073-a2bd-0e7e22486b21

Lysann Tranvouez added 30 commits October 25, 2024 15:05
Lysann Tranvouez added 15 commits November 23, 2024 14:10
Also adds MockCoordinatorPresentationFlow to help testing a flow getting dismissed:

MockNavigationController didn't work here.
The JellyfinCoordinator usually uses a Modal flow. When it is supposed to be hidden, we call flow.finishPresentation.
However, when used with pushFlow and MockNavigationController for the unit tests, this will only popToRootViewController, leaving us with a controller on the horizontal stack.

I also cannot switch to modalFlow, because then I do not have a MockNavigationController for checking the pushed view controllers.

An alternative would be extending the modalFlow to accept a factory for the navigation controller (which could then me a MockNavigationController).
But that's pretty much what MockCoordinatorPresentationFlow is.
Hitches were obeserved only in debug builds, but there was no quality difference between 16 and 32 pixels, and the complexity is O(n^2).
* auto focus fields to bring keyboard up immediattely
* use return key to go to next field or submit
Tiny size changes could happen when a scroll bounce happened.
This resulted in the blurhash returning for a second while waiting for the new image (the URL contains the image size).
Fixed by using an Equatable view wrapper which ignores size.

Larger size changes might still happen when e.g. rotating the screen or changing multitasking window sizes on iPad?
I think these cases will be negligible though.
nicer pdding for library

corner radius of image view is relative to its size
selecting an audiobook shows details about the book with an explicit download button, to avoid accidentally stsarting a download
@lysanntranvouez
Copy link
Author

lysanntranvouez commented Nov 28, 2024

So I would say this is now a MVP. The important features are there, there are some open nice to haves on my list above still, but I don't think it's the best idea to include them into this PR.

Updated demo video (simulator recording): https://github.com/user-attachments/assets/39b15e1e-301f-4073-a2bd-0e7e22486b21


I am aware of one bug, and I need some help with that: Sometimes when I hit download, the app appears to do some kind of a tactical restart. The main UI seems to reload. I am not sure what's happening, or how to find out what's happening. Is this some kind of error handling in the app? Or am I somehow dismissing the root view controller or something?

Attaching another video to demonstrate what I mean: https://github.com/user-attachments/assets/923a3c49-1e33-4626-ac4c-aa9260479c7e

@GianniCarlo
Copy link
Collaborator

Hi @lysanntranvouez ! sorry I've been a bit out this week, I'm almost done with the watch version (streaming is working, only need to add the download option), and I got tunnel vision.

Regarding the issue, you mentioned, the problem is here:

Screenshot 2024-11-29 at 12 15 15 PM

There are instances where the self.flow.finishPresentation(animated: true) is being called more than once, so the first time the Jellyfin screens are dismissed, the second time, the dismiss function dismisses the main navigation controller since it's not presenting anything (this is just how UIKit handles it under the hood). You just have to make sure that a coordinator only finishes once

I can put it up on TestFlight so it's easier for anyone who wants to test this feature, let me know if you would rather I wait until you put in the fix, or if I should go ahead and put it up on TestFlight

Re reviewing the PR, I want to finish up the work on the Apple watch (I can almost feel it 😅) and I'll circle back to this PR

@lysanntranvouez
Copy link
Author

Don't worry, I wasn't done anyway, and it's a lot. I understand that this takes time.

Plus, that help was quick help, thank you so much! I just pushed a fix for that.

TestFlight would be great! (Can I have an invite to that?)

@GianniCarlo
Copy link
Collaborator

Sure @lysanntranvouez ! I just put it up here: https://testflight.apple.com/join/ccKCfquZ (the link is open to anyone that wants to join an test it), I created a separate test group on TestFlight, so right now that build has only what is included in your PR

@GianniCarlo
Copy link
Collaborator

@lysanntranvouez I'm finally done with my PR, I went over this one, and besides translations, I think it's in a pretty good shape just to merge in, and get feedback from Jellyfin users on the next release. Let me know how things are looking, as I mean to submit the new release by the end of this week

Lysann Tranvouez added 2 commits December 11, 2024 20:20
because often the end might mention "book 1" if you're looking at a folder representing a series
@lysanntranvouez
Copy link
Author

Hey there, sorry for the late reply - I was a bit under the weather.

After doing some proper on device testing I just pushed two tiny tweaks.

But, it's ready to go from my point of view :)

@GianniCarlo GianniCarlo marked this pull request as ready for review December 11, 2024 23:19
@GianniCarlo GianniCarlo merged commit 613071d into TortugaPower:develop Dec 12, 2024
1 check passed
@GianniCarlo
Copy link
Collaborator

@lysanntranvouez no worries! I went ahead and merged the PR, I did some tests and there was a small retain cycle with the JellyfinConnectionViewModel (I fixed this in here), the thing is that under the current logic, the idea is that the Coordinator is kept in memory while the screen is in the navigation stack, and that's accomplished with the onTransition block from the ViewModel that keeps a reference to self from the coordinator, the problem was that on that transition block, the view model was being passed inside also, and the view model was keeping a reference to itself and leaking after the screen is dismissed.

I'll add the translations later today, and put up the final beta on all the three groups currently running betas, before submitting this weekend. I really appreciate your contribution with this PR 🙌, this really was a complex task to tackle

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