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

Add fps #635

Merged
merged 6 commits into from
Feb 3, 2021
Merged

Add fps #635

merged 6 commits into from
Feb 3, 2021

Conversation

jleckron
Copy link
Contributor

@jleckron jleckron commented Feb 2, 2021

Hi Boris,

I added functionality to see the real frame rate for each video in detailed mode 😉

I also added functionality to use the sidebar to sort by fps. In doing so, I added a couple new key-value pairs in en.json. According to i18n/README.md, hopefully it is simple to expand my changes for all other supported languages.

Please let me know of any changes you would like to see.

Thank you for the support!

Issue #623

Copy link
Owner

@whyboris whyboris left a comment

Choose a reason for hiding this comment

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

Awesome work!

Thank you for the comment about the real vs average FPS 👍

I'm requesting just a minor change: moving the "fps" text to the template 👍

@@ -39,6 +39,7 @@ export interface ImageElement {
stars: StarRating; // star rating 0 = n/a, otherwise 1, 2, 3
timesPlayed: number; // number of times the file has been launched by VHA
width: number; // width of the video (px)
frameRate: number; // base frame rate of the video in fps
Copy link
Owner

Choose a reason for hiding this comment

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

I don't have a strong preference, but perhaps fps is better - as it's shorter and is already a well-known acronym 🤔

interfaces/final-object.interface.ts Outdated Show resolved Hide resolved
* Generate frame rate display formatted as XXfps
* @param frameRate
*/
function getFrameRateDisplay(frameRate: number): string {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is better done in the template (.html file) 👍

The reason some other values are stored in the ImageElement object rather than live-computed is because their computation is much more computationally heavy, while this one is just appending text 👍

@@ -34,6 +34,8 @@

| {{ video.fileSizeDisplay }}

| {{ video.frameRateDisplay }}
Copy link
Owner

Choose a reason for hiding this comment

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

Here is where we can add the "fps" text 🤝

@whyboris
Copy link
Owner

whyboris commented Feb 3, 2021

Thank you @jleckron -- updated code looks great! I'll test it before merging this weekend 🤝

@whyboris whyboris merged commit b88d728 into whyboris:main Feb 3, 2021
@whyboris
Copy link
Owner

whyboris commented Feb 3, 2021

I tested -- works beautifully. I'll update the translations for other languages and add the button to the "Sorting options" list ✅

Excellent work - thank you very much 🙇 🤝 🎖️

@whyboris
Copy link
Owner

whyboris commented Feb 3, 2021

I made a few quick changes -- making the FPS search toggleable in the settings 🤝
#636 🙇

c3cris pushed a commit to c3cris/Video-Hub-App that referenced this pull request Feb 20, 2021
* Added fps display

* fps display works on test cases

* Added sortby fps in sidebar

* Fixed formatting (removed whitespace)

* Switched fps ordering to better match with other ordering patterns

* Changed template to fps, removed unnecessary functions, shortened variable names
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.

None yet

2 participants