Skip to content

Launcher: Default to replacing an already running NVDA (#8320)#9827

Merged
michaelDCurran merged 1 commit into
nvaccess:masterfrom
accessolutions:i8320
Sep 9, 2019
Merged

Launcher: Default to replacing an already running NVDA (#8320)#9827
michaelDCurran merged 1 commit into
nvaccess:masterfrom
accessolutions:i8320

Conversation

@JulienCochuyt

@JulienCochuyt JulienCochuyt commented Jun 27, 2019

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #8320

Summary of the issue:

It can be difficult at times for a non-initiated user to pass the -r|--replace command line parameter, even though it is the most expected behavior when running the executable of a portable copy.

Description of how this pull request fixes the issue:

  • Removed -r|--replace altogether. It is still accepted - yet ignored - as a command line parameter
  • An already running NVDA is always terminated, unless -k|--check-running
  • -q|--quit and -k|--check-running are now mutually exclusive
  • The desktop shortcut is still created with -r, in order to support downgrading an installed version

Testing performed:

  • Behavior of -q and -k with and without a running NVDA.
  • Launching a newly created portable copy

Known issues with pull request:

I have updated the user documentation in the command line arguments section.
I did not thoroughly read it through to check if a mention to the replace mechanism is present somewhere else.

Change log entry:

Section: New features, Changes, Bug fixes

Changes: Running nvda.exe now defaults to replace an already running copy of NVDA. The -r|--replace command line parameter is still accepted, but ignored.

@LeonarddeR LeonarddeR requested a review from feerrenrut June 27, 2019 09:26
LeonarddeR
LeonarddeR reviewed Jun 27, 2019
View reviewed changes

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In the user docs, it might be handy to note that the default behavior of opening the executable is that the running copy is terminated.

@JulienCochuyt

JulienCochuyt commented Jun 27, 2019

Copy link
Copy Markdown
Contributor Author

In the user docs, it might be handy to note that the default behavior of opening the executable is that the running copy is terminated.

I have just updated the "Launching NVDA" section, mentioning the new behavior for both installed and portable versions. Thanks for pointing that out.
Do you have any other places in mind where it might be relevant to refer to this change?

@LeonarddeR

LeonarddeR commented Jun 27, 2019 via email

Copy link
Copy Markdown
Collaborator

@feerrenrut

feerrenrut commented Jul 1, 2019

Copy link
Copy Markdown
Contributor

I think this PR should wait until the next release, there isn't much time for us to work through any issues that may crop up, and given that it is related to how NVDA starts I'm a bit hesitant to merge it right now.

@LeonarddeR

LeonarddeR commented Jul 1, 2019

Copy link
Copy Markdown
Collaborator

Mmm, this is filed against master, not beta. I guess 2019.3 won't be released any time soon? 2019.2 isn't even out and this pr won't hurt that.

@JulienCochuyt

JulienCochuyt commented Jul 15, 2019

Copy link
Copy Markdown
Contributor Author

120bc43: Rebased onto latest master.

@josephsl

josephsl commented Jul 16, 2019

Copy link
Copy Markdown
Contributor

Hi,

Reef was right - I get conflicts when merging Threshold branch to a private version of this PR, mostly having to do with argparse and installer. I advise holding off until this PR is properly rebased on master branch powered by Threshold/Python 3.

thanks.

@JulienCochuyt
Launcher: Default to replacing an already running NVDA (nvaccess#8320)
3708c8c 
 * An already running NVDA is always terminated, unless -k|--check-running
 * -q|--quit and -k|--check-running are now mutually exclusive
 * Updated User Guide
    * Launching NVDA
    * Command Line Options
@JulienCochuyt

JulienCochuyt commented Aug 13, 2019

Copy link
Copy Markdown
Contributor Author

Rebased onto latest master and linted.

The original PR is preserved in the branch accessolutions/nvda/i8320-py2 if you ever consider merging it into an eventual 2019.2.1

@michaelDCurran michaelDCurran merged commit 4e8073d into nvaccess:master Sep 9, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Sep 9, 2019
michaelDCurran added a commit that referenced this pull request Sep 9, 2019
@michaelDCurran
Update changes file for pr #9827.
ac5f4fb
@JulienCochuyt JulienCochuyt deleted the i8320 branch September 10, 2019 00:17
@JulienCochuyt

JulienCochuyt commented Sep 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@michaelDCurran, thanks for merging.
Do you wish I also file a PR against beta for the preserved original Python 2 version?
This could add some convenience to the future 2019.2.1, as the last Python 2 version is expected to live quite long before corporate environment switch to threshold versions.

EDIT:
The Python 2 version now also includes the fix for #10179.

@josephsl

josephsl commented Sep 10, 2019

Copy link
Copy Markdown
Contributor

Hi,

Found a rather serious (and very critical) bug: if you try to run an app with admin privileges, you'll see UAC come up. After the secure copy of NVDA exits after you interact with secure screens, normal copy of NVDA comes up, but it'll restart.

Thanks.

@josephsl josephsl mentioned this pull request Sep 10, 2019
Closed
JulienCochuyt added a commit to accessolutions/nvda that referenced this pull request Sep 10, 2019
@JulienCochuyt
Fix up of: Launcher: Default to replacing an already running NVDA (nv…
755acf8 
…access#8320, nvaccess#9827, nvaccess#10179)
@JulienCochuyt JulienCochuyt mentioned this pull request Sep 10, 2019
Merged
@JulienCochuyt

JulienCochuyt commented Sep 10, 2019

Copy link
Copy Markdown
Contributor Author

Found a rather serious (and very critical) bug: if you try to run an app with admin privileges, you'll see UAC come up. After the secure copy of NVDA exits after you interact with secure screens, normal copy of NVDA comes up, but it'll restart.

I did test the installer, but missed the special case of the secure desktop.
Thanks for this report.

JulienCochuyt added a commit to accessolutions/nvda that referenced this pull request Sep 10, 2019
@JulienCochuyt
Fix up of: Launcher: Default to replacing an already running NVDA (nv…
4795bcf 
…access#8320, nvaccess#9827, nvaccess#10179)
michaelDCurran pushed a commit that referenced this pull request Sep 10, 2019
@JulienCochuyt @michaelDCurran
Fix up of: Launcher: Default to replacing an already running NVDA (#8320
abb9f55 
, #9827, #10179) (#10183)
JulienCochuyt added a commit to accessolutions/nvda that referenced this pull request Sep 16, 2019
@JulienCochuyt
Launcher: Default to replacing an already running NVDA (nvaccess#8320)
f38307d 
 * Backport of nvaccess#9827
 * An already running NVDA is always terminated, unless -k|--check-running or --ease-of-access (nvaccess#10179)
 * -q|--quit and -k|--check-running are now mutually exclusive
 * Updated User Guide
    * Launching NVDA
    * Command Line Options
@JulienCochuyt JulienCochuyt mentioned this pull request Sep 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelDCurran michaelDCurran michaelDCurran approved these changes

@feerrenrut feerrenrut Awaiting requested review from feerrenrut

+1 more reviewer

@LeonarddeR LeonarddeR LeonarddeR approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2019.3

Development

Successfully merging this pull request may close these issues.

Launching a new copy of NVDA should exit running version and start launched version.

6 participants

@JulienCochuyt @LeonarddeR @feerrenrut @josephsl @michaelDCurran @nvaccessAuto