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

[Redesign] Downloads Size Warning + other fixes #1044

Merged

Conversation

Komodo5197
Copy link

Added warning text to downloads dialog for downloads with more than 150 songs. Download dialog will be shown if this triggers, even if no download settings are needed. This necessitates a 1 request delay before the dialog shows, which for me can take quite a while if the artist page is still loading because my server is very slow, but which should hopefully be fairly quick for most people.

Additional fixes:

  • Reworded custom download location warning to mention the issue with the "Music" directory.
  • Padded minutes to two digits for any time over an hour in queue time remaining display.
  • Altered app name of android debug builds to make them less confusing when a release build is installed.

…Pad minutes in timestamps with hours. Alter android app name of debug builds.
@Chaphasilor
Copy link
Collaborator

The wonders of gradle :D

Thanks for the PR and the additional fixes. I'll give it a try now.
How about pro-actively making the needed request in the background, or somehow scraping the needed info from the regular requests that are made to load artist's albums and such?

@Chaphasilor
Copy link
Collaborator

Screenshot_20250211-230643_Finamp_Debug.png

Hmm, track count doesn't match up between artist page and dialog 🤔

@Komodo5197
Copy link
Author

I believe that may be the artist vs album artist thing I was talking about. 205 songs by the artist, but 779 songs in albums with them as an album artist. Dos that seem reasonable in this case?

…ialog. Don't show a size of 0 in dialog for transcoding artist downloads.
@Chaphasilor
Copy link
Collaborator

Yes that could be. Jellyfin sometimes creates nonsensical albums with a bunch of random track in them, and tons of album artists.

Which are we currently downloading?

@Komodo5197
Copy link
Author

We currently download the album artist.

@Chaphasilor
Copy link
Collaborator

Hmm, seems like I wasn't really clear in what I meant. I didn't want to expose the setting yet, just have it be part of the FinampSettings. But I guess exposing it doesn't really hurt. So thanks for that, and sorry for not being clearer.

I was also thinking about reorganizing settings soon, with each settings category having an "advanced" section where settings like these could find a home. Sometimes they're useful for debugging, but regular users shouldn't need to mess with them

@Chaphasilor
Copy link
Collaborator

Oh and if I'm not mistaken the dialog opens quicker now on the artist screen, so using the fetched album list seems to work 👍🏻

@Komodo5197
Copy link
Author

What I basically meant with my comment was that I didn't see any point putting a setting in that we 'might' want to expose, and then having to deal with all the hassle of codegen and maybe migration code if we ever want to change it. Unless it's exposed to the user for change, it was fine as a constant local to the relevant code.

@Komodo5197
Copy link
Author

I've fixed the display bug where the codec was being displayed on 'transcoded' collections with no actual audio files. I tried to fix some issues I noticed where an image download with an outdated download location wouldn't update or show as failed.

@Chaphasilor
Copy link
Collaborator

Is there any unresolved discussion for this, or can we merge it?

@Komodo5197
Copy link
Author

I believe everything has been resolved.

@Chaphasilor
Copy link
Collaborator

Alright, thanks!

@Chaphasilor Chaphasilor merged commit 2aa0092 into jmshrv:redesign Feb 15, 2025
4 checks passed
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