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 deletion of albums and songs #331

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Conversation

Y0ngg4n
Copy link
Contributor

@Y0ngg4n Y0ngg4n commented Sep 22, 2022

This PR Adds simple deletion of already downloaded albums and songs

@jmshrv
Copy link
Owner

jmshrv commented Sep 22, 2022

Thanks for this :)

I've just added a push that makes items actually disappear when deleting, but there are a few issues before this is merged:

  • The rest of the downloads screen doesn't update (such as the overview at the top, file sizes etc
  • When deleting an album in offline mode, the album still shows in the album screen, which causes an error when opened as the album no longer exists

Both could be solved by doing some state management with Riverpod. For the 2nd one, this would mean hooking up the album list to use a central source of albums from Riverpod, which would be a pretty big task. I don't mind doing this, as it would also help with other issues like deleting albums in offline mode from the album screen (which I've disabled for the same reason).

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Sep 22, 2022

Hi thanks for your review. Sure would be nice if i could get some help from you and learn fron your project 🙂

Yes i have already seen that the list does not update caused by statelesswidget. But do not know how you handle states via hive. Quick side question wouldnt it be better to use isar?

@jmshrv
Copy link
Owner

jmshrv commented Oct 9, 2022

Finamp gets the list of albums with the JellyfinApiHelper class, which simply returns once without any sort of live updating. Currently, downloaded songs are stored in Hive databases, as at the time Isar was still in development. #213 will replace this janky system of Hive databases with an Isar database which should make stuff much easier. For now, we can either:

  • Add a provider that handles getting the albums and allows different bits of the app to modify the list
  • Rework JellyfinApiHelper to return a listener on the database. I'd like to more cleanly separate online and offline mode, so maybe I'll consider this then.

@jmshrv
Copy link
Owner

jmshrv commented Oct 9, 2022

Fixes #160

@jmshrv jmshrv linked an issue Oct 9, 2022 that may be closed by this pull request
@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Oct 9, 2022

Ok thank you for the explanation. So what do we have to do with this PR now?

@jmshrv
Copy link
Owner

jmshrv commented Oct 27, 2022

I'll probably go with the stream option as it would be much easier than using providers, although I might change my mind mid-implementation as it would cause a lot of useless StreamBuilders in the code.

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Oct 27, 2022

Ok sounds good

@arthurlutz
Copy link

Looking forward to this feature. Do you need help testing it out ?

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Nov 22, 2022

No i have to first implement the missing parts. But i am currently short on time.

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Dec 19, 2022

@jmshrv i have tested it now and it seems to work just fine. anything we need to do here or can we just merge?

@jmshrv
Copy link
Owner

jmshrv commented Dec 19, 2022

I'll merge this for now, the other improvements can come later

@jmshrv jmshrv merged commit ccc90e4 into jmshrv:main Dec 19, 2022
@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Dec 20, 2022

@jmshrv thank you :)

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Dec 24, 2022

@jmshrv could we push out a new release?

@jmshrv
Copy link
Owner

jmshrv commented Dec 24, 2022

Yeah, I'll make a new one as discussed in #371

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.

Allow deleting local files from "Downloads" view
3 participants