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

Adding clip duration to Events View #4133

Merged
merged 4 commits into from
Nov 2, 2022
Merged

Adding clip duration to Events View #4133

merged 4 commits into from
Nov 2, 2022

Conversation

banthungprong
Copy link
Contributor

@banthungprong banthungprong commented Oct 20, 2022

Issue #3813
(I'm not sure if this is the correct workflow, but I try...:
In the GitHub Docs it should be possible to manually link an issue to this pull request via the Development in the sidebar. Is not possible here).

Simply added the difference between end and start time as a span-Block.
And in the next div after the Start-Time the End-Time.
frigate Events with Length

Copy link
Collaborator

@NickM-27 NickM-27 left a comment

Choose a reason for hiding this comment

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

Happy to hear what others think, but my thoughts are:

  • having the end and start time as well as duration in seconds is redundant.
  • Maybe an option in the config could do one or the other so users have a choice
  • Just doing seconds is fine for short events but when you have events for a parked car 6372s is not super intuitive, might be nice if it was broken into a separate function and formatted as h m s

@banthungprong
Copy link
Contributor Author

Happy to hear what others think, but my thoughts are:

  • having the end and start time as well as duration in seconds is redundant.
  • Maybe an option in the config could do one or the other so users have a choice
  • Just doing seconds is fine for short events but when you have events for a parked car 6372s is not super intuitive, might be nice if it was broken into a separate function and formatted as h m s

Thanks for you fast reply!

  • end/start time is redundant. Will remove it.
  • will add an option in the config (because I did this already with an option to enable/disable a camera in the config (Support for enabling/disabling a camera in config #4109). Default will be disabled
  • formatting is natural (but I thought that the events never would reach these values). Will change the output.

@blakeblackshear
Copy link
Owner

I would rather not end up with a bunch of config options that modify the UI. Let's just show the start time and the duration. That should cover the vast majority of needs.

@NickM-27
Copy link
Collaborator

Sounds good to me, @banthungprong in that case can also move the duration down to the other time fields since there won't be an end time 👍

@banthungprong
Copy link
Contributor Author

I formatted it like this (hours/minutes only shown if not 0), using a new function clipLength():
image

Comment on lines 47 to 51
if (hours == 0 && minutes == 0) {
length=`${seconds} Seconds`
} else {
length=`${minutes} Minutes ${seconds} Seconds`
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we used abbreviations instead?

Suggested change
if (hours == 0 && minutes == 0) {
length=`${seconds} Seconds`
} else {
length=`${minutes} Minutes ${seconds} Seconds`
}
if (hours == 0 && minutes == 0) {
length=`${seconds}s`
} else {
length=`${minutes}m ${seconds}s`
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that but I'm not 100% sure what that means for translations... Is m and s really international (thinking about Asian languages, etc)?

Copy link
Contributor Author

@banthungprong banthungprong Oct 24, 2022

Choose a reason for hiding this comment

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

In the final commit "Minutes/Seconds" are used (identical to the existing solution in RecordingPlaylist.jsx).
For Translation see #947/#3718 (that's what I found regarding internationalization/language resources).

@@ -459,7 +474,7 @@ export default function Events({ path, ...props }) {
</div>
<div className="text-sm">
{new Date(event.start_time * 1000).toLocaleDateString()}{' '}
{new Date(event.start_time * 1000).toLocaleTimeString()}
{new Date(event.start_time * 1000).toLocaleTimeString()} <span className="text-red-300">({clipLength(event.end_time - event.start_time)})</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to color the text here, the red doesn't mean anything but it could imply an error or some other meaning which isn't important as it is just the duration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • red: correct. Simply took the color from the original request. Will remove it.
  • Abbreviations are ok for me here. But: In the Recordings View "Minutes"/"Seconds" are used (RecordingPlaylist.jsx). I assumed it would be better to use the same formatting in different views.
  • in RecordingPlaylist.jsx functions from "date-fns" are used. So I will do the same here - stupid to implement something new if you get it for free...
  • When a clip is new, the end time and duration is still "in progress". So I got negative Integer as results... The solution was already in the RecordingPlaylist.jsc.

@netlify
Copy link

netlify bot commented Nov 2, 2022

Deploy Preview for frigate-docs ready!

Name Link
🔨 Latest commit e46a105
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/636211573e6ac000096e1fa3
😎 Deploy Preview https://deploy-preview-4133--frigate-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@banthungprong
Copy link
Contributor Author

I found a "stupid" mistake. Switched start/end time in the function call. Found this when using a docker image based on this branch.
Renamed function to clipDuration because a lenght would be Bytes a duration is a time range.

@banthungprong banthungprong changed the title Adding clip length in s to Events View Adding clip duration to Events View Nov 2, 2022
@blakeblackshear blakeblackshear merged commit 552638d into blakeblackshear:dev Nov 2, 2022
herostrat pushed a commit to herostrat/frigate that referenced this pull request Nov 24, 2022
* Adding clip length in s to Events View

* added function returning human readable length

* switched to date-fns functions for formatting

* fixed switched start/end time, changed length to duration
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.

4 participants