Skip to content

Release lock on mouse keys when exiting.#13529

Merged
seanbudd merged 2 commits into
nvaccess:masterfrom
mwhapples:releaseMouseLockOnExit
Mar 30, 2022
Merged

Release lock on mouse keys when exiting.#13529
seanbudd merged 2 commits into
nvaccess:masterfrom
mwhapples:releaseMouseLockOnExit

Conversation

@mwhapples

Copy link
Copy Markdown
Contributor

Link to issue number:

#13410

Summary of the issue:

If the user locks a mouse button and then exits NVDA, the mouse button remains locked.

Description of how this pull request fixes the issue:

The mouse button states are checked in terminate() of the mouseHandler and unlocks the mouse buttons as required.

Testing strategy:

Manually tested by locking the mouse button and then restarting NVDA.

Known issues with pull request:

None known.

Change log entries:

New features
Changes
Any locked mouse keys will be unlocked when NVDA exits, previously the mouse button would remain locked.
Bug fixes
For Developers

Code Review Checklist:

  • Pull Request description:
    • description is up to date Done
    • change log entries Done
  • Testing:
    • Unit tests Not sure how to create unit tests to test mouse button states.
    • System (end to end) tests Same as unit tests, need help knowing how to test mouse buttons.
    • Manual testing Done
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation Not requested in issue.
    • Developer / Technical Documentation N/A
    • Context sensitive help for GUI changes N/A
  • UX of all users considered:
    • Speech N/A
    • Braille N/A
    • Low Vision N/A
    • Different web browsers N/A
    • Localization in other languages / culture than English N/A

@mwhapples mwhapples marked this pull request as ready for review March 23, 2022 14:43
@mwhapples mwhapples requested a review from a team as a code owner March 23, 2022 14:43
@mwhapples mwhapples requested a review from seanbudd March 23, 2022 14:43

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As mentioned in #13410 these should be announced.

I think the best way to do this is to move the code from script_toggleLeftMouseButton and script_toggleRightMouseButton into helper functions in mouseHandler.

For example, for the left button mouseHandler.isLeftButtonLocked(), mouseHandler.unlockLeftButton(), mouseHandler.lockLeftButton().

Make the scripts and mouseHandler.terminate call these functions.

Comment thread source/mouseHandler.py Outdated

def terminate():
global scrBmpObj, _shapeTimer
if winUser.getKeyState(winUser.VK_LBUTTON) & 32768:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be great to replace this magic number 32768 with a named constant such as MOUSE_BUTTON_DOWN.
Alternatively, we could create an IntEnum in keyboardHandler and import it.

class KeyState(IntEnum):
	TOGGLED = 1
	DOWN = 32768

Replacing other usages of these constants can be done in a separate issue and pull request

@seanbudd seanbudd Mar 24, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually the enum should probably use a bitshift or 0bx binary format, as MS docs defines these as

  • If the high-order bit is 1, the key is down; otherwise, it is up.
  • If the low-order bit is 1, the key is toggled. A key, such as the CAPS LOCK key, is toggled if it is turned on. The key is off and untoggled if the low-order bit is 0. A toggle key's indicator light (if any) on the keyboard will be on when the key is toggled, and off when the key is untoggled.
class KeyState(IntFlag):
	"""
	Mirrors the return values in GetKeyState (also used in GetAsyncKeyState)
	https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getkeystate#return-value
	"""
	TOGGLED = 1
	"""If the low-order bit is 1, the key is toggled.
	The key is off and untoggled if the low-order bit is 0.

	A key, such as the CAPS LOCK key, is toggled if it is turned on.
	A toggle key's indicator light (if any) on the keyboard will be on when the key is toggled,
	and off when the key is untoggled.
	"""
	DOWN = 1 << 15  # = 32768
	"""If the high-order bit (of a 16 bit int) is 1, the key is down; otherwise, it is up."""

@seanbudd seanbudd marked this pull request as draft March 24, 2022 02:18
@mwhapples mwhapples marked this pull request as ready for review March 24, 2022 09:31
@seanbudd

Copy link
Copy Markdown
Member

One problem I have noticed is that NVDA exits before the ui messages are spoken.
I'm not sure if it's easily possible to block the exit until the messages are announced, and I don't know if it's a good idea to do so either.

@feerrenrut feerrenrut added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 28, 2022
@mwhapples

Copy link
Copy Markdown
Contributor Author

I too don't know whether it would be a good thing to halt exit until the messages have been spoken, although it would seem strange to announce something only to have it cut off. I though tend to make my screenreaders fairly low verbosity so I personally would not really choose to have these messages spoken in the first place. I could do with guidance on how this should be made to work and then I will look into making the code work that way.

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Providing messages during the exit of NVDA is a general pattern we haven't solved yet.
I think this can be merged as-is until we find an appropriate UX pattern to solve this problem.

@seanbudd

Copy link
Copy Markdown
Member

I'm unable to commit the update to changes.t2t to update your repository fork.
Could you set this up if you haven't yet? Otherwise, feel free to push the changes updates.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@mwhapples mwhapples requested a review from a team as a code owner March 29, 2022 10:17
@mwhapples mwhapples requested a review from Qchristensen March 29, 2022 10:17
@mwhapples

Copy link
Copy Markdown
Contributor Author

Well I am not too familiar with github and all its options. I did a rebase from master in git command line to bring this up to date with master. Not sure whether that deals with the issue of merging. The referenced github page also mentions having a "Allow edits by maintainers" checkbox checked, which it is on this pull request. If more is needed I will need greater explanation of what I need to do.

@seanbudd

Copy link
Copy Markdown
Member

Something very strange has happened with this rebase - would you be able to undo it?

@mwhapples mwhapples force-pushed the releaseMouseLockOnExit branch from b9ad238 to 286ca26 Compare March 30, 2022 19:51
@mwhapples

Copy link
Copy Markdown
Contributor Author

I have done git reset --hard nvaccess/master and then reapplied my changes. Hopefully that sorts things out.

@seanbudd

Copy link
Copy Markdown
Member

Thanks, I realised pushing changes was a problem on my end. I'll merge this when the build finished.

@seanbudd seanbudd merged commit eec2568 into nvaccess:master Mar 30, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.2 milestone Mar 30, 2022
@seanbudd seanbudd linked an issue Mar 31, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unlock the mouse buttons when exiting NVDA

4 participants