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

Upgraded comtypes to 1.1.7 #9441

Merged
merged 3 commits into from May 7, 2019

Conversation

Projects
None yet
7 participants
@francipvb
Copy link
Contributor

commented Apr 2, 2019

Link to issue number:

Closes #9440, #8522

Summary of the issue:

This PR is mainly to improve UIAutomation support.

I use visual studio regularly and it is a particularly heavy application. It runs background tasks and these impacts the user interface.

However, I upgraded comtypes locally to do some testing and I saw some results regarding performance.

Description of how this pull request fixes the issue:

This upgrades from enthought/comtypes@edbaf3b to enthought/comtypes@1d3d38b

Testing performed:

Tested NVDA running from source code against Visual Studio 2017 and 2019. Got performance improvements with both of versions.

Known issues with pull request:

None known.

Change log entry:

Added by @leonardder

  • Changes for developers
    • Updated comtypes package to 1.1.7. (#9440)
@francipvb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

P.S: I'm posting this mainly to test binary builds from the build server.

Cheers,

@michaelDCurran

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Do you have any guess as to what exactly changed to give the better performance? I don't see anything too interesting in their changelog...

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

Here is a signed try build, which really helps in testing these things.

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2019

We have:

I don't think that's related.

@leonardder
Copy link
Collaborator

left a comment

It looks like I can't reproduce the issue I had that was caused by enthought/comtypes@98f6a42, so that's good.

Could you please update the readme to refer to comtypes 1.1.7?

@lukaszgo1

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

As readme is now updated could you add #8522 to the list which this pr closes @francipvb ?

@francipvb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Hello,

I have addressed the changes you requested.

@michaelDCurran wrote

Do you have any guess as to what exactly changed to give the better performance? I don't see anything too interesting in their changelog...

About performance comment on the issue, I did some testing before posting this PR and I saw that the performance improvement is a side effect of another change, but I was unable to determine what is the cause, there are no changes on UIAHandler's code nor UIA objects since release 2019.1 (python part).

Well, perhaps I didn't found them.


I think that this is interesting for addon developers:

  • Fix using temp directory as a cache folder when it has no write permission.

Cheers,

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

the performance improvement is a side effect of another change

Just to make sure, are you saying that Comtypes 1.1.7 does not affect performance?

@francipvb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Hello,

I ran two profiling sessions and I got some results.

In fact, there is no difference in the performance, so I don't know where is the change that caused this.

For completeness, here are the profile session files.

Cheers,

profiler-output.zip

@leonardder leonardder requested a review from feerrenrut May 7, 2019

@feerrenrut

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Ah... it looks like in commit 92d8487 the readme file has been changed causing a conflict here. I assume line endings have been changed.

@feerrenrut

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

I've merged master and pushed the result.

@feerrenrut feerrenrut merged commit 0be486a into nvaccess:master May 7, 2019

1 check was pending

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@nvaccessAuto nvaccessAuto added this to the 2019.2 milestone May 7, 2019

feerrenrut added a commit that referenced this pull request May 7, 2019

@zstanecic

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@feerrenrut

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

For now it's been merged to master. The threshold branch will be updated from master occasionally. Why do you ask?

@zstanecic

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@feerrenrut

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

I expect the threshold branch to be quite unstable initially, we will likely be quite aware of this. You are welcome to continue using it, however, please wait until we start requesting testers and feedback before creating new issues for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.