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

Launcher: Honor the --minimal command line parameter (#12289) #12322

Merged
merged 6 commits into from
May 6, 2021

Conversation

JulienCochuyt
Copy link
Collaborator

@JulienCochuyt JulienCochuyt commented Apr 22, 2021

Link to issue number:

Fixes #12289

Summary of the issue:

The nvda_logo.wav sound is played even if the launcher is started with the --minimal command line parameter.

Description of how this pull request fixes the issue:

Avoid playing the nvda_logo.wav sound if the launcher is started with the --minimal command line parameter.

Testing strategy:

Started with different combinations of command line parameters.
Ensure they are still interpreted as expected.

Known issues with pull request:

Change log entries:

Bug fixes
The NVDA installer now also honors the --minimal command line parameter and does not play the start-up sound, following the same documented behavior as an installed or portable copy NVDA executable.

Code Review Checklist:

  • Pull Request description is up to date.
  • Unit tests.
  • System (end to end) tests.
  • Manual tests.
  • User Documentation.
  • Change log entry.
  • Context sensitive help for GUI changes.

@JulienCochuyt JulienCochuyt requested a review from a team as a code owner April 22, 2021 07:07
Avoid playing nvda_logo.wav when started with --minimal
@JulienCochuyt JulienCochuyt changed the title Launcher: Honnor the --minimal command line parameter (#12289) Launcher: Honor the --minimal command line parameter (#12289) Apr 22, 2021
launcher/nvdaLauncher.nsi Show resolved Hide resolved
launcher/nvdaLauncher.nsi Show resolved Hide resolved
launcher/nvdaLauncher.nsi Outdated Show resolved Hide resolved
@JulienCochuyt
Copy link
Collaborator Author

@feerrenrut,
While at commenting and refactoring to increase readability, this NSIS file might also benefit from a little linting, such as indenting blocks and removing useless double quotes in !include directives.
I've tried to adhere to the current style for consistency, but I can restyle the whole thing if you so wish.

@feerrenrut
Copy link
Contributor

I've tried to adhere to the current style for consistency, but I can restyle the whole thing if you so wish.

I'd prefer that a restyle was done in a separate PR. If you are willing to do the testing, I'll happily take it.

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

This looks good now, thanks for doing that. I've just tested this locally and can confirm that the startup sound is not played.

However, NVDA still speaks through the process. I think the main use case for this argument is for mass unattended installations. Would you be willing to extend this to also tell the temporary copy to be set to no-speech?

@lukaszgo1
Copy link
Contributor

However, NVDA still speaks through the process. I think the main use case for this argument is for mass unattended installations. Would you be willing to extend this to also tell the temporary copy to be set to no-speech?

Currently minimal is also automatically set when NVDA starts on secure screens (see nvda.pyw around line 234). Wouldn't it be better to keep minimal as is so just suppress startup sound of the launcher and modify installSilent to suppress startup sound of the launcher and set NVDA to no speech??

@JulienCochuyt
Copy link
Collaborator Author

@lukaszgo1 wrote:

Wouldn't it be better to keep minimal as is so just suppress startup sound of the launcher and modify installSilent to suppress startup sound of the launcher and set NVDA to no speech??

I agree.

@feerrenrut wrote:

I'd prefer that a restyle was done in a separate PR. If you are willing to do the testing, I'll happily take it.

I'll prepare the two PRs (--install-silent and restyle) as based upon the result of merging this one.

@feerrenrut
Copy link
Contributor

Currently the userguide has the following explanations:

--minimal	No sounds, no interface, no start message, etc.
--install	Installs NVDA (starting the newly installed copy)
--install-silent	Silently installs NVDA (does not start the newly installed copy)

I'm sure you are both aware of this already, but for completeness. From https://techterms.com/definition/silent_install

A silent install is the installation of a software program that requires no user interaction. It is a convenient way to streamline the installation process of a desktop application.

Since our docs are not very specific, and most people will expect "install-silent" to produce no UI at all, graphical, speech, or otherwise. I'd make that the option that controls this.

On the other hand the docs for --minimal specifically mentions "no interface", speech is a user interface. Perhaps it is enough to update the docs to be more clear. But it leaves us with an odd spread of options.

The main use cases I am aware of are:

  • Sys-admin who wants to automatically install NVDA to many machines without the installation causing any disruption. For this NVDA should produce no sound, display no GUI, complete without interaction, and exit when installation is complete.
  • A user who wants to install NVDA on a computer, but continue using their old version once installation is complete. For this NVDA should display a GUI, should provide speech, should provide launcher startup sound, but should exit when installation is complete.

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

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.

Command line switch --minimal isn't silent
4 participants