Skip to content

Report battery information first when fetching current status#14215

Merged
seanbudd merged 3 commits into
branchFor2022.4from
fixup-batteryReportStatus
Oct 10, 2022
Merged

Report battery information first when fetching current status#14215
seanbudd merged 3 commits into
branchFor2022.4from
fixup-batteryReportStatus

Conversation

@seanbudd

@seanbudd seanbudd commented Oct 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Link to issue number:

Fixes #14214
See also #14213

Summary of the issue:

The current battery status message when pressing NVDA+shift+b is too verbose, because it reports the AC status before the battery level.

This is useful for an AC state change (e.g. when plugging in a charger)
But is not ideal for when pressing NVDA+shift+b to fetch the battery status.

Description of user facing changes

Reverts to previous ordering in 2022.3.

When the AC status changes, the AC status is still reported first: "charging" or "AC disconnected".
When NVDA+shift+b is pressed to fetch the battery status, the battery status is reported first: "X percentage, Z hours and Y minutes remaining"

Description of development approach

Change internal parameter onlyReportIfStatusChanged to ReportContext.
A ReportContext is used to determine order of speech.

When the context is an AC status change, this reports the current AC status first.
When the context is a user fetching the current battery status, this reports the remaining battery life first.

Testing strategy:

Manually test disconnecting and connecting AC and using NVDA+shift+b

Existing unit tests have been updated

Known issues with pull request:

None

Change log entries:

None, fixes unreleased regression

Code Review Checklist:

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

@seanbudd seanbudd requested a review from a team as a code owner October 5, 2022 01:38
@seanbudd seanbudd requested a review from feerrenrut October 5, 2022 01:38
@seanbudd seanbudd added this to the 2022.4 milestone Oct 5, 2022
@seanbudd seanbudd force-pushed the fixup-batteryReportStatus branch from b125d76 to 3583808 Compare October 5, 2022 01:54
@seanbudd seanbudd changed the base branch from master to branchFor2022.4 October 5, 2022 01:54
@AppVeyorBot

AppVeyorBot commented Oct 5, 2022

Copy link
Copy Markdown

See test results for failed build of commit 18051fcb70

@Mohamed00

Mohamed00 commented Oct 5, 2022

Copy link
Copy Markdown

I feel like the AC disconnected message should be removed when the battery life time estimate from Windows becomes available. It makes sense to have the AC disconnected message announced at first when the time estimate is initially unavailable, but after that it feels unnecessary, because if Windows is returning an estimate of how much time is left from the battery, a user could infer on their own that a charger is disconnected.

@Brian1Gaff

Brian1Gaff commented Oct 5, 2022 via email

Copy link
Copy Markdown

@seanbudd

seanbudd commented Oct 5, 2022

Copy link
Copy Markdown
Member Author

I feel like the AC disconnected message should be removed when the battery life time estimate from Windows becomes available. It makes sense to have the AC disconnected message announced at first when the time estimate is initially unavailable, but after that it feels unnecessary, because if Windows is returning an estimate of how much time is left from the battery, a user could infer on their own that a charger is disconnected.

I think it's better to be explicit.
It may be possible that the remaining life estimate is reported even if the AC is connected. For example, how much time you would have if you disconnected the AC now.

@Mohamed00

Mohamed00 commented Oct 6, 2022

Copy link
Copy Markdown

I'm not quite sure what you mean here. DO you mean that the remaining battery life could be reported even if a charger is supplying power?

@k-kolev1985

k-kolev1985 commented Oct 6, 2022

Copy link
Copy Markdown
Contributor

No, as far as I know, the system does not report remaining battery time when the battery is charging. The system reports remaining time during the battery charging, but that time is about how much time remains until the battery is fully charged.

@seanbudd

seanbudd commented Oct 7, 2022

Copy link
Copy Markdown
Member Author

No, as far as I know, the system does not report remaining battery time when the battery is charging. The system reports remaining time during the battery charging, but that time is about how much time remains until the battery is fully charged.

Turns out you are right:

The number of seconds of battery life remaining, or –1 if remaining seconds are unknown or if the device is connected to AC power.
https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-system_power_status

I still think it is better to explicitly state the new power status first when reporting the status change event (disconnect/connect).

@Mohamed00

Mohamed00 commented Oct 7, 2022 •
edited
Loading

Copy link
Copy Markdown

DO you mean when a charger is plugged in, the AC status should be announced first? If so, yes, I agree with you there, that announcement can stay. However I'm not sure what exactly the point is in hearing AC disconnected after an announcement like 93 percent 2 hours and 40 minutes remaining, as it is currently.

@seanbudd seanbudd force-pushed the fixup-batteryReportStatus branch from 2e563a3 to ad3b7fe Compare October 10, 2022 00:08
@seanbudd seanbudd added the release/blocking this issue blocks the milestone release label Oct 10, 2022
seanbudd
seanbudd commented Oct 10, 2022
View reviewed changes
Comment thread tests/unit/test_winAPI/test_powerTracking.py
feerrenrut
feerrenrut reviewed Oct 10, 2022
View reviewed changes
Comment thread source/winAPI/_powerTracking.py Outdated
Comment thread tests/unit/test_winAPI/test_powerTracking.py
Comment thread tests/unit/test_winAPI/test_powerTracking.py
@seanbudd seanbudd requested a review from feerrenrut October 10, 2022 03:46
@seanbudd seanbudd merged commit 03a8e08 into branchFor2022.4 Oct 10, 2022
@seanbudd seanbudd deleted the fixup-batteryReportStatus branch October 10, 2022 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release/blocking this issue blocks the milestone release

Projects

None yet

Milestone

2022.4

Development

Successfully merging this pull request may close these issues.

New NVDA+Shift+b message feels too verbose

6 participants

@seanbudd @AppVeyorBot @Mohamed00 @Brian1Gaff @k-kolev1985 @feerrenrut