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] Add more BaseItemDtoTypes #749

Merged
merged 3 commits into from
May 26, 2024

Conversation

Komodo5197
Copy link

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

I could try adding an image to a playlist :P

I'm sure that would break something...

@Chaphasilor
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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 Chaphasilor merged commit 54c1e74 into jmshrv:redesign May 26, 2024
4 checks passed
@Chaphasilor
Copy link
Collaborator

Alright. Thanks for handling this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants