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

New handy tech display models #9691

Merged

Conversation

FelixGruetzmacher
Copy link
Contributor

@FelixGruetzmacher FelixGruetzmacher commented Jun 7, 2019

Link to issue number:

Does not apply.

Summary of the issue:

When vendor-specific applications need direct access to a Handy Tech Braille display, they indicate this by sending a window message to a driver-created invisible window. Up until now, the native NVDA driver for Handy Tech devices does not create this window.

Description of how this pull request fixes the issue:

The driver now creates the window upon instantiation. It also responds to the sleep and wake messages appropriately.

Testing performed:

Tested with Actilino by sending a file to it via HTCom while driver was running. File was successfully sent, and afterwards the driver resumed its operation normally, i.e. the device restarted to braille.

Known issues with pull request:

None known.

Change log entry:

Section: Bug fixes

  • HTCom can now be used with a Handy Tech Braille display in combination with NVDA.
    Section: New features
  • The Handy Tech Basic Braille Plus family of displays is now natively supported by NVDA.

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 8, 2019

Unfortunately I found an issue after opening this. Currently, when the driver is instantiated, it creates a hidden window to take messages from our proprietary apps, and when the driver is terminated or its constructor fails, that window is destroyed. However, I failed to take into account that instantiation of the driver may take place in a thread which is not in a position to handle window messages, and which is also different from the thread which will eventually attempt to destroy the window. In this scenario, which happens for automatic detection, we'll be left with a message window that not only won't handle messages but also won't go away when asked. And in fact this is what I'm seeing.
Would it be acceptable to create the hidden window at module level rather than as part of a driver instance? It would mean that window would exist for every running NVDA on every system as driver modules are always imported. Strictly speaking I don't like such artifacts but right now I can see no other simple workaround. Could I get away with it?

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jun 8, 2019

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 8, 2019

@leonardder thanks, that helped a lot. The problem went away for me, and I committed the change.

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jun 8, 2019

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 8, 2019

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jun 8, 2019

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 8, 2019

@bramd
Copy link
Contributor

@bramd bramd commented Jun 8, 2019

@FelixGruetzmacher I think NVDA stores just LF line endings in Git, since Git converts CRLF by default to LF on commit. This can be controlled in Gits config, see https://stackoverflow.com/questions/10418975/how-to-change-line-ending-settings for example.

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 8, 2019

className = u"Handy_Tech_Server"
def __init__(self, driver):
super(InvisibleDriverWindow, self).__init__(u"Handy Tech Server")
self.driver = driver
Copy link
Collaborator

@leonardder leonardder Jun 10, 2019

Choose a reason for hiding this comment

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

Please make this a weak reference. You can see how this works on the Model class.

There is also a possibility to provide a callback to the reference that is called when the reference dies, i.e. when the driver instance dies. We really need to make sure that the window is destroyed in that case. Could you also test this, e.g. by forcefully disconnecting a device and checking whether the window is destroyed correctly? I assume it will if terminate is called when the driver loses its connection.

self.driver = driver
def windowProc(self, hwnd, msg, wParam, lParam):
if msg == window_message:
if wParam == 100 and lParam == 1:
Copy link
Collaborator

@leonardder leonardder Jun 10, 2019

Choose a reason for hiding this comment

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

Were do 100 and 1 stand for? Could you make these constants? Note that constants are capitalised.

self.driver.go_to_sleep()
elif wParam == 100 and lParam == 0:
self.driver.wake_up()
return 0
Copy link
Collaborator

@leonardder leonardder Jun 10, 2019

Choose a reason for hiding this comment

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

What does 0 mean in this context?

self.driver.wake_up()
return 0

window_message=windll.user32.RegisterWindowMessageW(u"Handy_Tech_Server")
Copy link
Collaborator

@leonardder leonardder Jun 10, 2019

Choose a reason for hiding this comment

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

This is now global to the driver module and therefore to NVDA as a whole. It would be nice if we could avoid this by registering the window message in create_message_window and unregister it in self.destroy_message_window

source/brailleDisplayDrivers/handyTech.py Show resolved Hide resolved
source/brailleDisplayDrivers/handyTech.py Show resolved Hide resolved
source/brailleDisplayDrivers/handyTech.py Show resolved Hide resolved
source/brailleDisplayDrivers/handyTech.py Show resolved Hide resolved
try:
self._messageWindow.destroy()
except:
log.debugWarning("", exc_info=True)
Copy link
Collaborator

@leonardder leonardder Jun 10, 2019

Choose a reason for hiding this comment

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

Make sure that if you move window message registering to the driver, that it is properly unregistered.

source/brailleDisplayDrivers/handyTech.py Show resolved Hide resolved
@feerrenrut feerrenrut self-requested a review Jun 11, 2019
Copy link
Collaborator

@leonardder leonardder left a comment

Almost there. Thanks for the great work until now.

source/brailleDisplayDrivers/handyTech.py Show resolved Hide resolved
time.sleep(self.timeout)
self._dev.close()
self._dev = None
time.sleep(self.timeout) #
Copy link
Collaborator

@leonardder leonardder Jun 11, 2019

Choose a reason for hiding this comment

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

There's an unnecessary hash after this line.

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 11, 2019

Copy link
Collaborator

@leonardder leonardder left a comment

I'm sorry, I missed these imports, see below.

@@ -9,6 +9,7 @@
Braille display driver for Handy Tech braille displays.
"""

import ui
Copy link
Collaborator

@leonardder leonardder Jun 11, 2019

Choose a reason for hiding this comment

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

This now seems duplicated

source/brailleDisplayDrivers/handyTech.py Show resolved Hide resolved
@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jun 17, 2019

@michaelDCurran is there any way we could have this in 2019.2?

@michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Jun 17, 2019

@zstanecic
Copy link
Contributor

@zstanecic zstanecic commented Jun 17, 2019

@zstanecic
Copy link
Contributor

@zstanecic zstanecic commented Jun 17, 2019

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 17, 2019

@zstanecic
Copy link
Contributor

@zstanecic zstanecic commented Jun 17, 2019

@josephsl
Copy link
Collaborator

@josephsl josephsl commented Jun 17, 2019

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 17, 2019

@josephsl
Copy link
Collaborator

@josephsl josephsl commented Jun 17, 2019

@zstanecic
Copy link
Contributor

@zstanecic zstanecic commented Jun 17, 2019

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 17, 2019

@josephsl
Copy link
Collaborator

@josephsl josephsl commented Jun 17, 2019

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jun 17, 2019

An alternative could be stripping this pr back to the pure basics, i.e. adding support for basic braille plus, not for the window message handling. Then, we could do the window message handling in a new pr for 2019.3

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 17, 2019

@zstanecic
Copy link
Contributor

@zstanecic zstanecic commented Jun 17, 2019

@michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Jun 21, 2019

Since another bug was identified in the beta which means we need to now release a beta2, I am going to review this, and if I approve it I will merge it to beta.

@michaelDCurran michaelDCurran merged commit f8507be into nvaccess:beta Jun 21, 2019
1 check passed
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jun 21, 2019
@michaelDCurran michaelDCurran removed this from the 2019.3 milestone Jun 21, 2019
@michaelDCurran michaelDCurran added this to the 2019.2 milestone Jun 21, 2019
@michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Jun 25, 2019

Beta2 (containing this pr) has been published. Can you please test the beta and confirm the changes in this pr are functioning correctly in the beta?
2019.2Beta2 download: https://ci.appveyor.com/api/buildjobs/4kuwrvn7r0l2erd1/artifacts/output/nvda_2019.2beta2.exe
If everything is okay, then I will release rc1 within the next few days.

@FelixGruetzmacher
Copy link
Contributor Author

@FelixGruetzmacher FelixGruetzmacher commented Jun 25, 2019

seanbudd added a commit that referenced this issue Jul 9, 2021
When vendor-specific applications need direct access to a Handy Tech Braille display, they indicate this by sending a window message to a driver-created invisible window. #9691 introduced this window for the handy tech driver, but it had some serious issues when doing the following

ensures that only one invisible driver window can be active by saving it on the class instead on instances of the class.

Co-authored-by: buddsean <sean@nvaccess.org>
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.

None yet

7 participants