Fix functionality to report remaining time on an audio file over one day long by saurabhanand03 · Pull Request #14451 · nvaccess/nvda

saurabhanand03 · 2022-12-16T05:36:42Z

Link to issue number:

Summary of the issue:

When using the foobar2000 audio player, the ctrl + shift + R command should report the remaining time in the audio file in the SS, MM:SS, HH:MM:SS, or D HH:MM:SS format depending on the remaining time length.
However, using the command on audio files over a day long produces an error as the code attempts to read the time string in the HH:MM:SS format instead of the D HH:MM:SS format.

Description of user facing changes

Time calculation is now processed correctly for all audio file lengths. Shortcuts for total time, elapsed time, and remaining time all report accurate values rather than returning errors.

Description of development approach

The elapsedTime and totalTime variables are read from the foobar2000 application directly, then the remainingTime variable is calculated based on the difference between totalTime and elapsedTime. However, since the totalTime and elapsedTime strings were not being parsed into "time" objects properly, the remainingTime calculation was returning an error.

In order to fix this, the getParsingFormat() and getOutputFormat() functions were merged into the parseIntervalToTimestamp() and script_reportRemainingTime() functions, respectively, since the two former functions were only called once in their respective latter functions.
The two former functions served to determine a given time string's format, so that said time string could be properly parsed in the two latter functions. However, it was also necessary to know the time string format so that the "remaining time" calculation could be done properly.

In addition to fixing the time calculations, the spoken UI message was also amended to say "[time] total", "[time] elapsed", or "[time] remaining", rather than just "[time]" so as to be more understandable for the listener.

Testing strategy:

Open foobar2000
Open audio file over 24 hours long
Skip to multiple time points throughout audio file to test out following steps 4-6:
Type ctrl + shift + e and confirm that correct elapsed time is reported
Type ctrl + shift + r and confirm that correct remaining time is reported
Type ctrl + shift + t and confirm that correct total time is reported
Test out commands with audio files under 24 hours long as well

Known issues with pull request:

If time is in MM:SS or SS format, reported message will not be spoken in time format. For example, if the remaining time is 5:34, the reported message will be "five colon thirty-four remaining" rather than "five minutes and thirty-four seconds remaining". I believe this issue has to do with how the UI message is read rather than the foobar2000 application, so it was not in the domain of this pull request.

Change log entries:

Bug fixes

Remaining time is now reported correctly (Foobar2000: Reporting the remaining time (Control Shift R) on a file that is over 1 day long produces an error. #14127)

New features

Speaker now says "elapsed", "remaining", or "total" after time is reported for more clarity.

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
Security precautions taken.

…day long

AppVeyorBot · 2022-12-16T06:34:25Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/8yx8a52x99fpr983/artifacts/output/nvda_snapshot_pr14451-27310,57910c3c.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 23.8,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 22.4,
FINISH_END 0.1

See test results for failed build of commit 57910c3caa

seanbudd · 2022-12-21T00:46:43Z

-	"""Attempts to find a suitable parsing format string for a HH:MM:SS, MM:SS or SS -style time interval."""
-	timeParts = len(interval.split(":"))
+
+def parseIntervalToTimestamp(interval):


please add type hints

Suggested change

def parseIntervalToTimestamp(interval):

def parseIntervalToTimestamp(interval: str): -> Optional[int]

seanbudd · 2022-12-21T00:49:31Z

 		elapsedTime = self.getElapsedAndTotalIfPlaying()[0]
 		if elapsedTime is not None:
-			ui.message(elapsedTime)
+			ui.message(elapsedTime + " elapsed")


This needs to be a translation string, otherwise elapsed won't be translated

Suggested change

ui.message(elapsedTime + " elapsed")

# Translators: The time elapsed time of a currently playing track in Foobar 2000.

# %s is replaced with the time elapsed.

ui.message(_("%s elapsed" % elapsedTime)

seanbudd · 2022-12-21T00:49:54Z

 		totalTime = self.getElapsedAndTotalIfPlaying()[1]
 		if totalTime is not None:
-			ui.message(totalTime)
+			ui.message(totalTime + " total")


same here with respect to translations

seanbudd · 2022-12-21T00:51:19Z

 # A named tuple for holding the elapsed and total playing times from Foobar2000's status bar
 statusBarTimes = collections.namedtuple('StatusBarTimes', ['elapsed', 'total'])

-def getParsingFormat(interval):


removing this function breaks the add-on API. Please follow steps outlined in devDocs/deprecations.md to deprecate the function.

alternatively, keep the function

seanbudd · 2022-12-21T00:52:02Z

 	else:
 		return None

-def getOutputFormat(seconds):


same here with respect to add-on API compatibility

seanbudd · 2022-12-21T00:52:26Z

-			remainingTime = parsedTotalTime - parsedElapsedTime
-			msg = time.strftime(getOutputFormat(remainingTime), time.gmtime(remainingTime))
+			parsedElapsedTime = parseIntervalToTimestamp(elapsedTime)
+			msg = getRemainingTime(parsedElapsedTime, parsedTotalTime) + " remaining"


same here with regards to translations for "remaining"

seanbudd · 2022-12-21T00:53:07Z

-	elif seconds < 3600:
-		return "%M:%S"
+
+def getRemainingTime(parsedElapsedTime, parsedTotalTime):


same here in respect to typing hints

seanbudd · 2022-12-21T00:55:33Z

Thanks for fixing this @saurabhanand03, just a few minor issues remaining.

seanbudd · 2023-01-16T06:38:23Z

+		return time.strftime("%#d day %H:%M:%S", time.gmtime(remainingTime - 86400))
 	else:
-		return "%H:%M:%S"
+		return time.strftime("%#d days %H:%M:%S", time.gmtime(remainingTime - 86400))


"day"/"days" should also be translatable. the time format in general would be good to be translatable in general if possible.
I wonder if we have existing code in NVDA to make a localised timestamp announcement

seanbudd · 2023-01-19T05:22:48Z

Closing in favour of #14127

Fixes #14127 Supersedes #14451 Summary of the issue: When using the foobar2000 audio player, the ctrl+shift+r command should report the remaining time in the audio file in the SS, MM:SS, HH:MM:SS, or D HH:MM:SS format depending on the remaining time length. However, using the command on audio files over a day long produces an error as the code attempts to read the time string in the HH:MM:SS format instead of the D HH:MM:SS format. Description of user facing changes Time calculation is now processed correctly for all audio length up to 31 days. After this point, it is uncertain how foobar reports time. In addition to fixing the time calculations, the spoken UI message was also amended to say "[time] total", "[time] elapsed", or "[time] remaining", rather than just "[time]" so as to be more understandable for the listener. Description of development approach Created a collection of translatable time string output formats. Created a generic way to parse timedeltas to localised formatted strings. Created internal functions for foobar appModule to parse foobar time strings to localised formatted strings. --------- Co-authored-by: saurabhanand03 <asaurabh2003@gmail.com>

Fix functionality to report remaining time on an audio file over one …

5eb87f4

…day long

saurabhanand03 requested a review from a team as a code owner December 16, 2022 05:36

saurabhanand03 requested a review from michaelDCurran December 16, 2022 05:36

seanbudd reviewed Dec 21, 2022

View reviewed changes

seanbudd marked this pull request as draft December 21, 2022 00:54

seanbudd reviewed Jan 16, 2023

View reviewed changes

seanbudd mentioned this pull request Jan 19, 2023

Fix reporting time in foobar2000 over 24 hours #14570

Merged

9 tasks

seanbudd closed this Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix functionality to report remaining time on an audio file over one day long#14451

Fix functionality to report remaining time on an audio file over one day long#14451
saurabhanand03 wants to merge 1 commit into
nvaccess:masterfrom
saurabhanand03:foobar2000-time-reporting-error

saurabhanand03 commented Dec 16, 2022

Uh oh!

AppVeyorBot commented Dec 16, 2022

Uh oh!

seanbudd Dec 21, 2022

Uh oh!

seanbudd Dec 21, 2022

Uh oh!

seanbudd Dec 21, 2022

Uh oh!

seanbudd Dec 21, 2022

Uh oh!

seanbudd Dec 21, 2022

Uh oh!

seanbudd Dec 21, 2022

Uh oh!

seanbudd Dec 21, 2022

Uh oh!

seanbudd Dec 21, 2022

Uh oh!

seanbudd commented Dec 21, 2022

Uh oh!

seanbudd Jan 16, 2023

Uh oh!

seanbudd commented Jan 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	def parseIntervalToTimestamp(interval):
	def parseIntervalToTimestamp(interval: str): -> Optional[int]

-			ui.message(elapsedTime + " elapsed")
+			# Translators: The time elapsed time of a currently playing track in Foobar 2000.
+			# %s is replaced with the time elapsed.
+			ui.message(_("%s elapsed" % elapsedTime)

Uh oh!

Conversation

saurabhanand03 commented Dec 16, 2022

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Dec 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Dec 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Jan 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants