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

Feature: Added audio bitrate to the Details Pane #15304

Merged
merged 5 commits into from
May 5, 2024

Conversation

Artoryn
Copy link
Contributor

@Artoryn Artoryn commented May 5, 2024

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Feature: Add audio file bitrate to details pane #15282

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
      • Small consolidation of formatting of the bitrate display string. This was done as the same formatting was going to be added in a third location so centralizing it and using the DisplayFunctions already present would make more sense.
      • Change the bitrate from 1024 for the conversion to 1000 (bytes to bits), as this would fix the "Sidenote" that the issue mentioned. This was done to make sure the bitrate is calculated off bits and not bytes better matching file explorer's display of this value.
  • Are there any other steps that were used to validate these changes?
  1. Open app (Audio validation)
  2. Navigated to directory with a .mp3, .mp4, and .wav audio file
  3. Verified that the Bitrate showed on the Preview Pane Detail window
  4. Opened individual file properties window
  5. Navigated to the Details section
  6. Verified that the Bitrate still showed properly formatted under the properties window

  1. Open app (Video validation)
  2. Navigated to a directory with a .mp4 video file
  3. Verified that the Bitrate showed on the Preview Pane Detail window
    a. Added string title text of "Audio Encoding Bitrate" as it seemed unclear for video files which in properties has 2 "Encoding Bitrate" tags under Audio and Video
  4. Opened individual file properties window
  5. Navigated to the Details section
  6. Verified that the Bitrate for Audio and Video still showed properly formatted under the properties window

  1. Open app (Combined file validation different bitrates)
  2. Navigated to a directory with multiple video/audio files
  3. Verified that the Bitrate showed on the Preview Pane Detail window for individual files
  4. Selected multiple files (with different bitrates)
  5. Opened properties window
  6. Navigated to the Details section
  7. Verified that "(multiple values)" displays under bitrate

  1. Open app (Combined file validation same bitrates)
  2. Navigated to a directory with video/audio file
  3. Verified that the Bitrate showed on the Preview Pane Detail window for individual file
  4. Create a copy of a file for testing
  5. Selected both files (original and copy of file)
  6. Opened properties window
  7. Navigated to the Details section
  8. Verified that the expected Bitrate displays under bitrate

Screenshots (optional)
image

@0x5bfa 0x5bfa added needs - testing Pull request requires testing before being merged and removed needs - code review labels May 5, 2024
@hishitetsu
Copy link
Member

@Artoryn is this still a work in progress?

@yaira2 yaira2 added needs - code review and removed needs - testing Pull request requires testing before being merged labels May 5, 2024
@Artoryn
Copy link
Contributor Author

Artoryn commented May 5, 2024

@Artoryn is this still a work in progress?

No, as long as the few refactors to existing functionality are fine, it is good to go. I marked it Ready for Review.

@Artoryn Artoryn marked this pull request as ready for review May 5, 2024 15:27
Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the last stale review.

The code is pretty looking great. LGTM.

@hishitetsu
Copy link
Member

@yaira2 what about also adding video encoding bitrate to video files?

@yaira2
Copy link
Member

yaira2 commented May 5, 2024

@yaira2 what about also adding video encoding bitrate to video files?

Good idea, I'm on board with this.

@Artoryn
Copy link
Contributor Author

Artoryn commented May 5, 2024

This was the original reason I added the new text for specifying Audio over just the Encoding Bitrate as well.
image

Will get these changes pushed.

Co-authored-by: hishitetsu <[email protected]>
Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@yaira2 yaira2 changed the title Feature: Add audio file bitrate to Preview Pane Details Feature: Added audio file bitrate to the Details Pane May 5, 2024
@yaira2 yaira2 changed the title Feature: Added audio file bitrate to the Details Pane Feature: Added audio bitrate to the Details Pane May 5, 2024
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels May 5, 2024
@yaira2
Copy link
Member

yaira2 commented May 5, 2024

@Artoryn thank you for contributing to Files.

@yaira2 yaira2 merged commit d2652a4 into files-community:main May 5, 2024
6 checks passed
@Artoryn Artoryn deleted the dev/PreviewAudioBitrate branch May 6, 2024 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Add audio file bitrate to details pane
4 participants