Move MessageWindow from core to winAPI module by seanbudd · Pull Request #14035 · nvaccess/nvda

seanbudd · 2022-08-19T04:08:19Z

Link to issue number:

None

Summary of the issue:

core.main is too long of a function.
Work was performed in the message window section of core.main.
In efforts to reduce the length of core.main, the MessageWindow should be factored out to winAPI.

In doing this, a bug was discovered that orientation status was not incorrectly reported when the state had not changed.
The previous orientation status was not being correctly cached.

Description of user facing changes

A bug was fixed where orientation changes were incorrectly reported when the orientation has not changed.

When the power status changes, the amount of battery time remaining is now reported to be consistent with script_say_battery_status.

Description of development approach

Code has been migrated and reorganised to purpose driven modules.

Testing strategy:

Manually tested:

power state changes (disconnect and connecting power to laptop, performing script_say_battery_status)
session state changes (locking and unlocking device)
display state changes (rotating hybrid tablet device)

Known issues with pull request:

None

Change log entries:

refer to PR diff

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.

AppVeyorBot · 2022-08-19T05:39:41Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
PASS: Lint check.
PASS: System tests.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/3f4aa05ahk4s6c40/artifacts/output/nvda_snapshot_pr14035-26241,981b655d.exe

See test results for failed build of commit 981b655de7

AppVeyorBot · 2022-08-19T09:49:33Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/o99fbdsip0h5lcov/artifacts/output/nvda_snapshot_pr14035-26245,7f96082d.exe

See test results for failed build of commit 7f96082d17

seanbudd · 2022-08-19T10:23:44Z

-			self.orientationCoordsCache = (width,height)
-			if width > height:
-				# If the height and width are the same, it's actually a screen flip, and we do want to alert of those!
-				if self.orientationStateCache == self.ORIENTATION_LANDSCAPE and self.orientationCoordsCache != (width,height):
-					return
-				#Translators: The screen is oriented so that it is wider than it is tall.
-				ui.message(_("Landscape" ))
-				self.orientationStateCache = self.ORIENTATION_LANDSCAPE
-			else:
-				if self.orientationStateCache == self.ORIENTATION_PORTRAIT and self.orientationCoordsCache != (width,height):


setting the cache on line 636 means line 639 and line 645 will never be true, so non-changes of orientation are incorrectly reported.

AppVeyorBot · 2022-08-22T09:24:39Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit 0a7ac9dc44

feerrenrut

This is definitely an improvement.
Let's look for opportunites to add unit testing while at it, to make future modifications in the area easier.

AppVeyorBot · 2022-08-25T01:13:19Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
Build execution time has reached the maximum allowed time for your plan (60 minutes).

See test results for failed build of commit f4ac14906b

AppVeyorBot · 2022-08-25T02:54:37Z

FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/y53en67q8w3xmm3r/artifacts/output/nvda_snapshot_pr14035-26316,91f7de59.exe

See test results for failed build of commit 91f7de5982

feerrenrut

Overall looks good.

feerrenrut · 2022-08-29T09:04:01Z

@@ -137,6 +137,14 @@ def windowProc(self, hwnd: HWNDValT, msg: int, wParam: int, lParam: int) -> None
 		@param lParam: Additional message information.
 		"""
 		post_windowMessageReceipt.notify(msg=msg, wParam=wParam, lParam=lParam)


post_ should mean "after it has been handled", however given the naming of this extension point it is a bit of a fuzzy concept. Can you confirm that this is the original order of extension point vs handleWindowsMessage?

The reason to have consistency here is if the extension point listener wants to check state after it has been modified by NVDA.

yes, this is keeping the original order.

In this case, I believe post_ means "after the message has been received".

It probably should have been named pre/post_handleWindowMessage

This can be easily renamed in this PR, the old symbol has been deprecated/moved anyway.
I'll change this to pre_handleWindowMessage?

feerrenrut · 2022-08-30T03:40:10Z

 # See the file COPYING for more details.

+"""
+When working on this file, consider moving to winAPI.


Let's leave these comments out for now, it isn't clear where the line is for what is "generic Windows abstraction" vs core NVDA behavior. Many files probably have these two concepts coupled, and it will require a careful approach to extract the generic Windows abstraction part into winAPI.

feerrenrut · 2022-08-30T03:40:33Z

 Used to provide input gestures for touchscreens, touch modes and other support facilities.
 In order to use touch features, NVDA must be installed on a touchscreen computer running Windows 8 and later.
+
+When working on this file, consider moving to winAPI.


Similarly here.

feerrenrut

I'm a bit worried that the scope of this refactor is growing. This increases the changes of introducing regressions.

feerrenrut · 2022-08-30T03:43:31Z

+_orientationState: Optional[OrientationState] = None
+
+
+def initialize():


Can this module be excluded from the add-on API? Given this is new code, and we don't know there is a demand for it within the API, keeping it excluded will let us iterate safely on the design.

This also goes for the other new modules that don't need to be exposed.

feerrenrut · 2022-08-30T03:53:22Z

+class SystemMetrics(IntEnum):
+	"""
+	GetSystemMetrics constants
+	https://docs.microsoft.com/en-gb/windows/win32/api/winuser/nf-winuser-getsystemmetrics


Since these are winuser constants, the values living in winUser makes makes them easier to find.
The options I see right now:

Keep these in winUser, moving that to winAPI later.

Add a new winUser to winAPI (containing only these constants) now, accepting that it will be confusing to have two identically named modules.

Rename this to winUserConstants.

As discussed, this has been moved to winAPI.winUser.constants

AppVeyorBot · 2022-08-30T07:15:12Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/0niaqy7o9rtt9pyw/artifacts/output/nvda_snapshot_pr14035-26415,0ae423b9.exe

See test results for failed build of commit 0ae423b959

feerrenrut

Looks good.

AppVeyorBot · 2022-08-31T04:33:23Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests. See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/96mgd6qdg3u4ckpq/artifacts/output/nvda_snapshot_pr14035-26419,82d6a274.exe

See test results for failed build of commit 82d6a27496

jage9 · 2022-09-13T20:14:27Z

Previously, the battery status command read the percent followed by the charge state. This now seems to be reversed, which to me is less than ideal. Is there a reason to flip this information around? The numeric to me is the more important bit of information.

seanbudd · 2022-09-14T00:49:08Z

@jage9 - could you open a new issue or GitHub discussion for this?
Discussions on closed pull requests are easily lost and without more feedback it is hard to know what the preferred behaviour is for most people.

In the issue, please say why this information is more important to you in that order.
For example, I imagine most people know if their AC is disconnected or connected, so the % is more important.

seanbudd · 2022-12-22T05:21:59Z

The API changes for this issue have been announced here: https://groups.google.com/a/nvaccess.org/g/nvda-api/c/7-8ddyZzloQ/m/6L_Jrpy3BQAJ

This was merged before the mailing list was announced. However, we have announced API changes in 2022.3.3 and 2023.1, so it may be misleading to skip the API changes in 2022.4 (when the mailing list was created).

Move code from core to winAPI

0a97788

seanbudd self-assigned this Aug 19, 2022

seanbudd added 2 commits August 19, 2022 17:22

fix ckeckPot

4dda07e

fix announcing charge status

a271d6f

seanbudd changed the title ~~Move code from core to winAPI module~~ Move MessageWindow from core to winAPI module Aug 19, 2022

seanbudd added the merge-early Merge Early in a developer cycle label Aug 19, 2022

seanbudd added 2 commits August 22, 2022 14:01

Merge branch 'master' into moveCodeToWinAPI

9a9888a

fixup reporting unknown status

233e19f

seanbudd marked this pull request as ready for review August 22, 2022 04:10

seanbudd requested a review from a team as a code owner August 22, 2022 04:10

seanbudd requested a review from feerrenrut August 22, 2022 04:10

seanbudd commented Aug 22, 2022

View reviewed changes

initialize batterystate as unknown

3e589b7

feerrenrut suggested changes Aug 24, 2022

View reviewed changes

seanbudd added 5 commits August 24, 2022 19:41

Merge branch 'master' into moveCodeToWinAPI

59f1543

used named constants for seconds

54ae3d6

add notes to move to winAPI module

73d0d07

Create HWNDVal Type

927680b

simplify window message handling

7e7bd3e

seanbudd added 2 commits August 25, 2022 11:39

Make display tracking modular and unit testable

0cac074

Make power tracking modular and unit testable

5158be2

seanbudd marked this pull request as draft August 25, 2022 01:52

add unit testing

191652a

seanbudd marked this pull request as ready for review August 25, 2022 02:56

seanbudd requested a review from feerrenrut August 25, 2022 02:57

feerrenrut suggested changes Aug 29, 2022

View reviewed changes

seanbudd added 2 commits August 30, 2022 11:09

rename to _getNewOrientationStyle

91c7b96

change to assertEqual

822440a

seanbudd marked this pull request as draft August 30, 2022 01:14

initialize orientation state'

d543615

seanbudd marked this pull request as ready for review August 30, 2022 02:47

seanbudd requested a review from feerrenrut August 30, 2022 02:47

remove usagees of deprecated code

1122dcf

feerrenrut reviewed Aug 30, 2022

View reviewed changes

seanbudd added 2 commits August 30, 2022 13:43

Merge branch 'master' into moveCodeToWinAPI

ec77e83

address review feedback:

2381c14

feerrenrut suggested changes Aug 30, 2022

View reviewed changes

seanbudd marked this pull request as draft August 30, 2022 04:07

seanbudd added 4 commits August 30, 2022 14:58

Merge branch 'master' into moveCodeToWinAPI

43f72fe

rename modules, mark as private

6fcad2f

move SystemMetrics enum

f731680

revert changes to winAPI.constants

07b56ef

seanbudd marked this pull request as ready for review August 30, 2022 06:30

seanbudd requested a review from feerrenrut August 30, 2022 06:31

improve deprecation message

2048b83

feerrenrut approved these changes Aug 31, 2022

View reviewed changes

Comment thread source/core.py

seanbudd merged commit 2392d13 into master Aug 31, 2022

seanbudd deleted the moveCodeToWinAPI branch August 31, 2022 05:55

nvaccessAuto added this to the 2022.4 milestone Aug 31, 2022

		_orientationState: Optional[OrientationState] = None


		def initialize():

Uh oh!

Conversation

seanbudd commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Aug 19, 2022

Uh oh!

AppVeyorBot commented Aug 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Aug 22, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Aug 25, 2022

Uh oh!

AppVeyorBot commented Aug 25, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Aug 30, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AppVeyorBot commented Aug 31, 2022

Uh oh!

jage9 commented Sep 13, 2022

Uh oh!

seanbudd commented Sep 14, 2022

Uh oh!

seanbudd commented Dec 22, 2022

seanbudd commented Aug 19, 2022 •

edited

Loading