Skip to content

External win event limiter#10556

Closed
feerrenrut wants to merge 5 commits into
masterfrom
externalWinEventLimiter
Closed

External win event limiter#10556
feerrenrut wants to merge 5 commits into
masterfrom
externalWinEventLimiter

Conversation

@feerrenrut

Copy link
Copy Markdown
Contributor

Link to issue number:

Summary of the issue:

There is some suspicion that NVDA may be missing some events by not handling windows events callbacks fast enough. Not only this, but processing and queuing events takes up time on the main thread when it isn't necessary to do so.

Description of how this pull request fixes the issue:

This is a prototype to move the event queuing out into separate C++ threads.

Testing performed:

Known issues with pull request:

  • Put this behind a feature flag, with a toggle in advanced settings
  • Further unit tests required
  • C++ unit tests should run with other unit tests
  • More documentation required for why we limit to certain numbers of events.
  • Some naming is.... sub-optimal.
  • Several todo items remaining

Change log entry:

Section: New features, Changes, Bug fixes

@codeofdusk

This comment has been minimized.

@feerrenrut

This comment has been minimized.

@feerrenrut

feerrenrut commented Mar 27, 2020

Copy link
Copy Markdown
Contributor Author

I have been looking at this again, remaining / proposed changes:

  • Rename C++ library so that to reduce confusion with source/eventHandler.py
    • Proposed name: WinEventCache
  • Extract python code that registers for win events in source/IAccessibleHandler.py
    • Convert IAccessibleHandler into a package to group related files, and make backwards compat easier.
    • Proposed name: internalWinEventHandler.py
  • Since the initial intention is to duplicate behavior, ensure any modifications to changed files since commit 4b53f8764 are captured as appropriate. 4b53f8764 was the master commit this was initially based on.
  • Add an option in the Advanced panel to swap between threaded C++ event handler and python event handler
    • This may require NVDA to restart to take effect.

@feerrenrut

Copy link
Copy Markdown
Contributor Author

Decided on naming this WinEventCache since it's shorter, unique, and "threaded" is an implementation detail.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

Would it be worht it to test this during daily use using a try build? Or is this still not mature enough?

@LeonarddeR

Copy link
Copy Markdown
Collaborator

@feerrenrut: thinking about this again, have you considered implementing the C++ code in this as a Python extension. I really understand that for historical reasons, the local NVDAHelper library was implemented as a stand alone library accessed using ctypes. However, now we are at Python 3.7 and no longer rely on old versions of VC++ to build extension modules, I think it should be considered, as the ctypes layer could be stripped.

@feerrenrut

Copy link
Copy Markdown
Contributor Author

have you considered implementing the C++ code in this as a Python extension.

I hadn't considered that. I'm not familiar with the ins and outs of python extensions. I don't think I would want to hold this work up further right now.

@codeofdusk

This comment was marked as off-topic.

@codeofdusk

This comment has been minimized.

@feerrenrut

This comment has been minimized.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

@feerrenrut What are you current plans with this pr? I'd love to see it going, together with a similar pr for the UIA event handling.

@feerrenrut

Copy link
Copy Markdown
Contributor Author

There aren't any plans for this. It was considered a risky change and hard to justify. To be able to prioritize the work on this, we need some reproducible test conditions to measure the impact this would have for users. After the initial work was done here, there were some significant changes made to the in-python eventLimiter (more detailed logging and more event filtering). This would need to be duplicated here, or a common abstraction would be required.

Due to the potential for regressions, the initial intention was for a period where both (in python, and external) event limiters were used.

@codeofdusk

This comment was marked as off-topic.

@feerrenrut

This comment was marked as off-topic.

@codeofdusk

This comment was marked as off-topic.

@feerrenrut

This comment was marked as off-topic.

@feerrenrut

Copy link
Copy Markdown
Contributor Author

I'm going to close this as abandoned.

@feerrenrut feerrenrut closed this Dec 8, 2022
@feerrenrut feerrenrut added the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants