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.

Sign up for GitHub

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

Jump to bottom

recorder: resume logs recorded within 4 hours #3136

Closed
wants to merge 5 commits into from
Closed

recorder: resume logs recorded within 4 hours #3136

wants to merge 5 commits into from

Conversation

bobrippling
Copy link
Collaborator

We now look at the latest log's last timestamp - if it was within four hours, we prompt the user to resume recording to that file.

This change also adds (manually runnable) tests for setRecording()

Fixes #3135

@bobrippling
recorder: resume logs recorded within 4 hours
9b35547 
We now look at the latest log's last timestamp - if it was within
four hours, we prompt the user to resume recording to that file.

Fixes espruino#3135
@bobrippling
recorder: manually runnable tests for setRecording()
2b8399b
@bobrippling bobrippling mentioned this pull request Dec 20, 2023
Open
Mineinjava
Mineinjava reviewed Dec 21, 2023
View reviewed changes
Copy link
Contributor

@Mineinjava Mineinjava left a comment

Choose a reason for hiding this comment

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

Should it be clarified that the file name is in UTC not local time?
This unlikely to affect people not living in GMT+-1

Additionally, I feel like forcing the recorder to append should always append to the latest file, regardless of the time difference

apps/recorder/ChangeLog Outdated
@@ -45,3 +45,4 @@
0.36: When recording with 1 second periods, log time with one decimal.
0.37: 1 second periods + gps log => log when gps event is received, not with
setInterval.
0.38: Resume previous recorder logs, if within four hours - regardless of date
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to clarify that the previous recorder log is only resumed on append

@bobrippling
Copy link
Collaborator Author

Should it be clarified that the file name is in UTC not local time? This unlikely to affect people not living in GMT+-1

Very good point - I think that since we now look at the times within the file, the filename is less important (so long as it still sorts by date), so we could make it that the filename is in localtime (but keep the timestamps themselves in UTC), so when you look at the files they're in your timezone.

Additionally, I feel like forcing the recorder to append should always append to the latest file, regardless of the time difference

That makes sense - how does this sound as a proposed flow? Any tweaks/additions?

on recording start:

if any previous recorder log (containing data):
  if within 4 hours:
    prompt the user: append, new, overwrite
  else:
    prompt the user: append, new

"new" will create a filename based on the current date (in localtime)

(the prompt will also have the existing cancel option)

(I'll leave this PR open here - feel free to add changes on if you like, I'm not sure when I'll next be free to pick it up)

nxdefiant
nxdefiant reviewed Dec 30, 2023
View reviewed changes
apps/recorder/widget.js Outdated Show resolved Hide resolved
gfwilliams
gfwilliams reviewed Mar 14, 2024
View reviewed changes
apps/recorder/widget.js Outdated Show resolved Hide resolved
@gfwilliams
Copy link
Member

I like the idea, but have you actually tested this in practise? Because iterating over the entire file to find the last line could take quite a long time (10+ seconds depending on the track length) and I think that would provide quite a poor experience if you user just wants to start a run.

At minimum it should at least check the filename date and if it's different, skip the check.

... plus this feels like quite a lot of extra complexity? I'm not sure really sure if it's worth it?

@bobrippling
recorder: fix lint
bc5d88b
@bobrippling
Merge branch 'master' into fix/recorder-midnight
3dde5af 
Conflicts:
	apps/recorder/ChangeLog
	apps/recorder/metadata.json
@bobrippling
Copy link
Collaborator Author

bobrippling commented Mar 16, 2024 •
edited
Loading

Good point - @Mineinjava is this an issue you still see?

Either way I'd like to keep the tests (and fix the lint warnings), I'll split out to another PR if necessary (and if you agree the tests are good to have)

@Mineinjava
Copy link
Contributor

Good point - @Mineinjava is this an issue you still see?

Either way I'd like to keep the tests (and fix the lint warnings), I'll split out to another PR if necessary (and if you agree the tests are good to have)

Honestly I think the best solution is to

  • tell the user the name of the last log
  • if they say "append," append regardless of timestamps

@bobrippling
Copy link
Collaborator Author

Yeah that seems reasonable, I'll make some progress towards that as part of this PR

@bobrippling
Copy link
Collaborator Author

This has been subsumed by #3431, closing

@bobrippling bobrippling closed this Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mineinjava Mineinjava Mineinjava left review comments

@nxdefiant nxdefiant nxdefiant left review comments

@gfwilliams gfwilliams gfwilliams left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Recorder] Day in filename is often wrong
4 participants
@bobrippling @gfwilliams @Mineinjava @nxdefiant