fix: Bitrate display in Media Info#556
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Extract the repeated bitrate formatting logic into a reusable helper method to reduce duplication and improve readability.
- Define the 10_000_000 threshold as a named constant (e.g.,
HIGH_BITRATE_THRESHOLD) to make the switch point self-documenting and easier to tweak. - Add a null check or fallback for
bitRatebefore using!to avoid potential runtime errors if the value is missing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the repeated bitrate formatting logic into a reusable helper method to reduce duplication and improve readability.
- Define the 10_000_000 threshold as a named constant (e.g., `HIGH_BITRATE_THRESHOLD`) to make the switch point self-documenting and easier to tweak.
- Add a null check or fallback for `bitRate` before using `!` to avoid potential runtime errors if the value is missing.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates the InformationModel to correctly convert the API’s bitRate values (in bps) into human-readable kbps or Mbps formats and applies consistent rounding rules for both video and audio streams. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PartyDonut
left a comment
PartyDonut
left a comment
There was a problem hiding this comment.
Thanks for taking the time to fix this. I do have a simple cleanup suggestion. Let me know if it's unclear.
lib/models/information_model.dart
Outdated
| "Interlaced": e.isInterlaced, | ||
| "FrameRate": e.realFrameRate, | ||
| "Bitrate": "${e.bitRate} kbps", | ||
| "Bitrate": e.bitRate! <= 10000000 |
There was a problem hiding this comment.
The bang operator is something best avoided.
While the logic itself is perfectly fine. It would be better to separate this into it's own extension function. That way we can also keep it null safe.
Take a look at the "size_formatting.dart" file for where and how to implement a very similar extension function.
There was a problem hiding this comment.
Good call, thanks for the heads up.
lib/util/bitrate_formatting.dart
Outdated
| } | ||
|
|
||
| String? get videoBitrateFormat { | ||
| const int VIDEO_HIGH_BITRATE_CUTOFF = 10000000; |
There was a problem hiding this comment.
Seems like the AI suggested the naming convention of this, not sure why. But in dart for local constants its normal to use camelCase.
As I'm already nitpicking should the Kb variable in this case not also be lowercase 'kb' same as the returned string?
There was a problem hiding this comment.
haha, the AI suggestion + my background made me go against the linter suggesting camelCase for consts, I'll change that over.
Very good nitpicking eye on the 'kb' name as well, just made both Mb and kb the same on muscle memory 👍
PartyDonut
left a comment
PartyDonut
left a comment
There was a problem hiding this comment.
Very small nitpick on my end, everything else looks good. Thanks for the quick fixes.
PartyDonut
changed the title
2 participants
Pull Request Description
Tiny fix for the bitrate info in the Movie/Episode Info window, and also I have formatted both audio and video to be what I would expect, (for high bitrate video, over 10 Mbps it is
xx.x Mbps, under that it is the value in kbps, and for any audio, the bitrate in kbps), open to opinions, might even be something the user could set for themselves via a toggle.Issue Being Fixed
The Jellyfin API returns the video and audio bitrate in
bps, but previously they were displayed as though they arekbpsvalues.(+ formatting)
Screenshots / Recordings
High bitrate ex:
Lower bitrate ex:
Checklist