Skip to content

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

Merged
feerrenrut merged 6 commits into
nvaccess:masterfrom
accessolutions:i12289-minimalLauncher
May 6, 2021
Merged

Launcher: Honor the --minimal command line parameter (#12289)#12322
feerrenrut merged 6 commits into
nvaccess:masterfrom
accessolutions:i12289-minimalLauncher

Conversation

@JulienCochuyt

@JulienCochuyt JulienCochuyt commented Apr 22, 2021

Copy link
Copy Markdown
Contributor

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
@JulienCochuyt JulienCochuyt requested a review from seanbudd April 22, 2021 07:07
Avoid playing nvda_logo.wav when started with --minimal
@JulienCochuyt JulienCochuyt force-pushed the i12289-minimalLauncher branch from 429980c to 580c6d2 Compare April 22, 2021 07:09
@JulienCochuyt JulienCochuyt changed the title Launcher: Honnor the --minimal command line parameter (#12289) Launcher: Honor the --minimal command line parameter (#12289) Apr 22, 2021
Comment thread launcher/nvdaLauncher.nsi
Comment thread launcher/nvdaLauncher.nsi
Comment thread launcher/nvdaLauncher.nsi Outdated
@JulienCochuyt

Copy link
Copy Markdown
Contributor 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
Copy Markdown
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.

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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.

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

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