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

Fix logic error in display tracking #16560

Merged
merged 1 commit into from
May 17, 2024

Conversation

codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented May 17, 2024

Link to issue number:

None

Summary of the issue:

On my machine (set to 1080p), after a while, NVDA repeatedly says "lanscape". In the Python console, I get:

>>> _displayTracking.getPrimaryDisplayOrientation()
OrientationState(width=1920, height=1080, style=<Orientation.PORTRAIT: 1>)

However, since 1920 is greater than 1080, getPrimaryDisplayOrientation().style should be Orientation.LANDSCAPE.

Description of how this pull request fixes the issue:

Declare parameter names in the call to _getOrientationStyle.

Testing strategy:

Alpha testing

Known issues with pull request:

None known

Change log entry:

None needed

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@codeofdusk codeofdusk requested a review from a team as a code owner May 17, 2024 01:40
@codeofdusk codeofdusk requested a review from gerald-hartig May 17, 2024 01:40
@codeofdusk codeofdusk force-pushed the no-more-spurious-landscape branch 2 times, most recently from a693349 to ba1253d Compare May 17, 2024 01:42
@codeofdusk codeofdusk force-pushed the no-more-spurious-landscape branch from d5b0fea to be7e78a Compare May 17, 2024 01:46
@codeofdusk codeofdusk marked this pull request as draft May 17, 2024 02:13
@codeofdusk
Copy link
Contributor Author

@seanbudd I'm getting several display orientation change notifications per minute on my machine, making NVDA unusable until I restart (at which point they stop for a while). Why do we want to report state changes when heightAndWidthUnchanged is True (in _getNewOrientationStyle)?

@seanbudd
Copy link
Member

@codeofdusk

This event is only meant to fire if your display changes (either in orientation or connecting to a new display).
https://learn.microsoft.com/en-us/windows/win32/gdi/wm-displaychange

As per the code comment if the height and width are the same, it's a screen flip and the orientation state has changed.
Otherwise, it may be a change of display (e.g. monitor disconnected).

Is it possible your device is connected to a display with a faulty connection? I am curious as to what is firing the WM_DISPLAYCHANGE events.

@codeofdusk
Copy link
Contributor Author

@seanbudd I usually have no display connected, but connecting one (even for a few minutes) seems to solve it.

@codeofdusk codeofdusk force-pushed the no-more-spurious-landscape branch from be7e78a to b5e9d89 Compare May 17, 2024 03:24
@codeofdusk
Copy link
Contributor Author

That said, assuming the original behaviour of getPrimaryDisplayOrientation isn't intended, I'll mark this PR as ready for review.

@codeofdusk codeofdusk marked this pull request as ready for review May 17, 2024 03:25
@seanbudd
Copy link
Member

Thanks yes this is a great catch, does it also fix the issue for you without changing _getNewOrientationStyle?

@seanbudd
Copy link
Member

Can you please add a change log entry

@codeofdusk codeofdusk changed the title Suppress spurious display orientation change notifications Fix logic error in display tracking May 17, 2024
@codeofdusk
Copy link
Contributor Author

It doesn't fix the issue for me, but it does fix a logic error, so hopefully it fixes something else somewhere. No what's new entry added since no apparent user impact.

@seanbudd
Copy link
Member

Perhaps we could create a toggle to disable these notifications in general? Might be worth creating an option for power status change notifications too?

@seanbudd seanbudd merged commit c595463 into nvaccess:master May 17, 2024
1 check was pending
@wmhn1872265132
Copy link
Contributor

If this is done, it seems that notifications such as adjusting the volume through the volume keys should not be restricted to the Explorer process.

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.

3 participants