jmshrv / finamp

A Jellyfin music client for mobile
Mozilla Public License 2.0
1.67k stars 117 forks source link

[Redesign] Add more BaseItemDtoTypes #749

Closed Komodo5197 closed 1 month ago

Komodo5197 commented 1 month ago

Adds several more types which will be treated like songs by download system, like Movie or Video. They may or may not download correctly, but hopefully they shouldn't throw errors?

Changed default action when parsing a BaseItemDto of unknown type from throwing an error to creating a collection which will not have any children when syncing. Hopefully this keeps the download system from ever breaking the UI.

Komodo5197 commented 1 month ago

I've also figured out why the themed menu's weren't repainting. They ended up using toString on their size to detect size changes, but it seems like in release mode this just prints 'Instance of Size' for everything. I've switched to properly using hashCode, and also moved back to screenSize instead of object size because it seems like this is actually the core bug that made me move away from that.

Chaphasilor commented 1 month ago

I can confirm that the visual bug in the menus and with landscape mode has been fixed with this PR.
I currently don't have any other media types on my test server, and don't feel like testing any weird setups either. Users will have other setups anyway, so I guess we'll just see how it works for them after the next update. If that's okay for you?
Otherwise I could also try to add some other media to test...

Komodo5197 commented 1 month ago

I created a movie library to test that, and it seemed to work fine when I put one in a playlist. I haven't tested with an unknown collection type, but at least it can't be any worse than it was, and I don't know how we'd end up with one of those right now anyway.

Chaphasilor commented 1 month ago

I could try adding an image to a playlist :P

I'm sure that would break something...

Chaphasilor commented 1 month ago

Surprisingly, adding an image to a playlist didn't cause an issue, but I think it just didn't get added server-side. The regular menu didn't even have the option to add to playlist, just the multi-select menu. Similar for eBooks, although those had the menu option Downloading a TV show episode worked in principle, but the downloaded item didn't have any audio. Direct-playing the episode before downloading it also didn't work, but transcoded streaming had audio.
Transcoded downloads didn't work at all, causing the server to return a 500 error. finamp-logs_transcoded-episode-download.txt

I'm also not sure if the bitrate is correct, but it could be:
image

Komodo5197 commented 1 month ago

I messed around some more, and both direct playback and transcoded downloads worked for my item, so it's probably just an issue with certain formats. I don't really think I can do much about that. I also noticed that the audiostream for my item didn't actually have a bitrate returned from the server, so the displayed bitrate was ~15000 kbps, but there isn't really anything we can do about that. Your item might actually have one though, because even though 384 seems high, it seems very low if it was including video.

Chaphasilor commented 1 month ago

Okay. Do you think we should simply not download known "bad" types, like movies or episodes? If some of them won't be able to play, I think there's no point in downloading them. Not sure how hard it would be to exclude them though.

If it's not feasible, I think we can merge this, since it's definitely much better than the old behavior :)

Komodo5197 commented 1 month ago

I'm thinking it's a filetype specific issue, and we don't even do anything for actual song types related to that, much less video types. I'd just merge as-is, and not worry too much about the full extent of video comparability.

Chaphasilor commented 1 month ago

Alright. Thanks for handling this!