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

Add command line parameter for silent installations to disable NVDA at the logon screen and enhance portable-path command line behavior #8623

Merged
merged 7 commits into from Dec 13, 2018

Conversation

Projects
None yet
4 participants
@leonardder
Copy link
Collaborator

leonardder commented Aug 13, 2018

Link to issue number:

Fixes #8574

Summary of the issue:

  1. In the current situation, it is not possible to specify the start at logon behaviour from the command line. This issue is relevant for system administrators who want to install NVDA silently on a multi user system.
  2. When starting NVDA with the --portable-path command line parameter and creating a portable copy from the GUI, the provided portable path isn't shown in the gui as one might expect.

Description of how this pull request fixes the issue:

  1. Added a --enable-start-on-logon command line parameter. It should be specified as --enable-start-on-logon=True or --enable-start-on-logon=False, just --enable-start-on-logon will show an error message. Alternative supported values are yes/no, on/off, 1/0 (i.e. this option behaves as configobj boolean). NVDA's behaviour is to default this to True
  2. When the --portable-path parameter is provided at NVDA start, the provided path is now pre-filled in in the portable copy creation dialog.

Testing performed:

Ran try build.

  1. Just the launcher: in the installation dialog, use NVDA on logon was enabled.
  2. launcher with --enable-start-on-logon=True: in the installation dialog, use NVDA on logon was enabled.
  3. launcher with --enable-start-on-logon=False: in the installation dialog, use NVDA on logon was disabled.
  4. launcher with --install-silent and --enable-start-on-logon=False. IN the copy of NVDA that ran after installation, start on logon was disabled in the general settings.
  5. Launcher with --portable-path: the provided portable path showed up in the create portable copy dialog.

Known issues with pull request:

None

Change log entry:

  • New features

    • Added the --disable-start-on-logon command line parameter to allow silent installations of NVDA that don't run at the logon screen by default. (#8574)
  • Changes

    • When NVDA is started with the --portable-path command line parameter, the provided path is automatically filled in when trying to create a portable copy of NVDA using the NVDA menu. (#8623)

@leonardder leonardder force-pushed the BabbageCom:i8574 branch from 6e4496c to 0cd9dfc Aug 15, 2018

@leonardder leonardder requested a review from feerrenrut Aug 15, 2018

@feerrenrut

This comment has been minimized.

Copy link
Contributor

feerrenrut commented Aug 20, 2018

Hate to question this further at this stage. But, I'm wondering if it might be more future proof to have this option as --enable-start-on-logon [true|false].

This means that the command line option remains useful if we ever change the default. It also allows people to be specific about their expectations on the command line (whether that is to enable or disable), rather than relying on the implicit default behaviour.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Aug 20, 2018

@feerrenrut

This comment has been minimized.

Copy link
Contributor

feerrenrut commented Aug 21, 2018

If we are able to be concrete about it and say the default is enable-start-on-logon=True then I think it would be my preference, but first we have to identify what variations in behaviour exist at the moment (which I think is what you were getting at with your last comment).

Otherwise, it looks like you can use a combination of default and type to ensure a boolean arg with a default of None:

>>> import argparse
>>> p = argparse.ArgumentParser()
>>> p.add_argument('--foo', default=None, type=bool)
_StoreAction(option_strings=['--foo'], dest='foo', nargs=None, const=None, default=None, type=<type 'bool'>, choices=None, help=None, metavar=None)
>>> p.parse_args(['--foo', 'True'])
Namespace(foo=True)
>>> p.parse_args(['--foo', 'False'])
Namespace(foo=True)
>>> p.parse_args([])
Namespace(foo=None)
>>> p.parse_args(['--foo', '"hello"'])
Namespace(foo=True)
>>> p.parse_args(['--foo', '0'])
Namespace(foo=True)
>>> p.parse_args(['--foo', '1'])
Namespace(foo=True)
>>> p.parse_args(['--foo', 0])
Namespace(foo=False)

Note that this appears to rely on casting the argument value to bool. But I don't think this should be a problem.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Aug 21, 2018

@feerrenrut commented on 21 aug. 2018 05:36 CEST:

Otherwise, it looks like you can use a combination of default and type to ensure a boolean arg with a default of None:

This works when throwing False or 0 at parse-args, but when providing 0 or False at the command line, it looks like it will still use the bool value of the string "0" or "False".

bool("False")
True

We can probably fix this with an easy function though:

def stringToBool(s):
    return s.lower() not in ("0", "false", "off")

Then, use stringToBool as the type.

Rename --disable-start-on-logon to --enable-start-on-logon. It now re…
…quires either True/False, Yes/No, On/Off, 1/0 as its value
@derekriemer

This comment has been minimized.

Copy link
Collaborator

derekriemer commented Aug 26, 2018

If you make it optional, with a default of None what happens?

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Aug 27, 2018

@derekriemer commented on 26 aug. 2018 08:28 CEST:

If you make it optional, with a default of None what happens?

I updated the description of the pr to reflect your question.

@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Dec 5, 2018

@feerrenrut: I think I've addressed your concerns. A user can now provide enable-start-on-logon=True or --enable-start-on-logon=False to forcefully enable or disable starting at the logon screen when installing or updating. The default for --enable-start-on-logon is None, in which case the old behaviour applies (i.e. if updating, use the current value, else True)

@feerrenrut
Copy link
Contributor

feerrenrut left a comment

Sorry, I forgot all about this PR. I'm generally happy with this, but have a suggestion to improve the user guide.

Show resolved Hide resolved user_docs/en/userGuide.t2t Outdated

leonardder and others added some commits Dec 13, 2018

Update user_docs/en/userGuide.t2t
Co-Authored-By: leonardder <leonardder@users.noreply.github.com>
@leonardder

This comment has been minimized.

Copy link
Collaborator Author

leonardder commented Dec 13, 2018

Done, thanks for this. I added an additional sentence in the start at logon section.

@feerrenrut feerrenrut merged commit 7274f9b into nvaccess:master Dec 13, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2018.4 milestone Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.