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 an appModule for zoom, fixes #7754 #8491

Merged
merged 2 commits into from Jul 19, 2018
Merged

Conversation

derekriemer
Copy link
Collaborator

@derekriemer derekriemer commented Jul 9, 2018

Link to issue number:

Fixes #7754

Summary of the issue:

Make zoom's notifications via alerts accessible.

Description of how this pull request fixes the issue:

In zoom, alerts are filtered out because zoom uses a window which is not a child of the foreground window for zoom. This pull request requests events for the window class the notifications use, even in the background.

Testing performed:

Tested to see if alerts worked while in zoom.

Known issues with pull request:

None

Change log entry:

Section: Bug fixes

@josephsl
Copy link
Collaborator

josephsl commented Jul 9, 2018

Hi,

Hmmm, if what's new entry is added, this could set a precedent for others. @LeonarddeR and others, any comments on this?

Thanks.b

@PratikP1
Copy link

PratikP1 commented Jul 9, 2018 via email

@josephsl
Copy link
Collaborator

josephsl commented Jul 9, 2018 via email

@PratikP1
Copy link

PratikP1 commented Jul 9, 2018 via email

LeonarddeR
LeonarddeR previously approved these changes Jul 10, 2018
Copy link
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

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

Note that I honestly haven't used this software, however I assume you've tested your implementation.

@@ -2394,4 +2395,3 @@ Major highlights of this release include support for 64 bit editions of Windows;
- NVDA now asks if it should save configuration and restart if the user has just changed the language in the User Interface Settings Dialog. NVDA must be restarted for the language change to fully take effect.
- If a synthesizer can not be loaded, when choosing it from the synthesizer dialog, a message box alerts the user to the fact.
- When loading a synthesizer for the first time, NVDA lets the synthesizer choose the most suitable voice, rate and pitch parameters, rather than forcing it to defaults it thinks are ok. This fixes a problem where Eloquence and Viavoice sapi4 synths start speaking way too fast for the first time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this line got accidentally removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentional?

@josephsl
Copy link
Collaborator

josephsl commented Jul 10, 2018 via email

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.

Also, I will have to rely on your testing. Can you get the original reporter to confirm this addresses the issue for them with a try/pr build?

@@ -2394,4 +2395,3 @@ Major highlights of this release include support for 64 bit editions of Windows;
- NVDA now asks if it should save configuration and restart if the user has just changed the language in the User Interface Settings Dialog. NVDA must be restarted for the language change to fully take effect.
- If a synthesizer can not be loaded, when choosing it from the synthesizer dialog, a message box alerts the user to the fact.
- When loading a synthesizer for the first time, NVDA lets the synthesizer choose the most suitable voice, rate and pitch parameters, rather than forcing it to defaults it thinks are ok. This fixes a problem where Eloquence and Viavoice sapi4 synths start speaking way too fast for the first time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentional?

@derekriemer
Copy link
Collaborator Author

Copyright headers are there. Yes, will do.

@derekriemer
Copy link
Collaborator Author

I rebased this on master.

@derekriemer
Copy link
Collaborator Author

I didn't mean to remove that line, sorry, I was messing with the online github file editor. This poses the question, why is there two lines at the end of that file though?

@derekriemer
Copy link
Collaborator Author

@feerrenrut multiple reports on the parent ticket now confirm my try build works as expected.

feerrenrut
feerrenrut previously approved these changes Jul 18, 2018
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.

Zoom Does Not work As Expected with NVDA
6 participants