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

Remove audio logo from NVDA launcher #17507

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

SaschaCowley
Copy link
Member

@SaschaCowley SaschaCowley commented Dec 12, 2024

Link to issue number:

Closes #14068
Closes #15110

Summary of the issue:

The NVDA launcher currently plays an audio logo when launched.
This creates a number of problems:

  • Playback of this file uses winmm, which is a deprecated API.
  • Playback of this file may delay execution of the launcher, if it takes longer than unpacking the launcher does. (Use shorter startup sound for NVDA launcher #14068)
    • The NVDA audio logo is 10 seconds long, whereas it typically takes around 6 seconds for the launcher to start the temporary NVDA on my machine.
  • Some users may want to silence playback of this sound altogether, which is currently not possible. (15110)

Description of user facing changes

The NVDA launcher is now silent until the temporary copy of NVDA it includes is started.

Description of development approach

Removed all code related to playing the audio logo from launcher/launcher.nsis.

Testing strategy:

Built the launcher and ran it from explorer.
Also ran NVDA from source patched to check for updates, and swapped the downloaded launcher for the one without the sound logo.

Known issues with pull request:

There is now an appreciable period of silence between NVDA quitting to perform an update, and the temporary NVDA speaking to read out the update progress.

Code Review Checklist:

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

@coderabbitai summary

@SaschaCowley SaschaCowley marked this pull request as ready for review December 12, 2024 03:03
@SaschaCowley SaschaCowley requested a review from a team as a code owner December 12, 2024 03:03
@cary-rowen
Copy link
Contributor

There is now an appreciable period of silence between NVDA quitting to perform an update, and the temporary NVDA speaking to read out the update progress.

Can we improve this? Is it possible to use a shorter sound? In order to let users know that the update has started, rather than being stuck for some reason.

@@ -11,7 +11,6 @@ CRCCheck on

ReserveFile "${NSISDIR}\Plugins\x86-unicode\system.dll"
ReserveFile "${NSISDIR}\Plugins\x86-unicode\banner.dll"
ReserveFile "..\miscDeps\launcher\nvda_logo.wav"
Copy link
Member

Choose a reason for hiding this comment

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

can we also delete that file from miscDeps in a separate PR?

Copy link
Member Author

@SaschaCowley SaschaCowley Dec 12, 2024

Choose a reason for hiding this comment

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

Created nvaccess/nvda-misc-deps#35. Do we want to merge that and update the submodule on this branch?

@SaschaCowley
Copy link
Member Author

@cary-rowen

There is now an appreciable period of silence between NVDA quitting to perform an update, and the temporary NVDA speaking to read out the update progress.

Can we improve this? Is it possible to use a shorter sound? In order to let users know that the update has started, rather than being stuck for some reason.

Unfortunately PlaySound and friends are the only easy ways of natively playing a sound file that I'm aware of, and they all use winmm. As we're running under NSIS, our options here are rather limited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants