Skip to content

Upgraded comtypes to 1.1.7#9441

Merged
feerrenrut merged 3 commits into
nvaccess:masterfrom
francipvb:i9440
May 7, 2019
Merged

Upgraded comtypes to 1.1.7#9441
feerrenrut merged 3 commits into
nvaccess:masterfrom
francipvb:i9440

Conversation

@francipvb

@francipvb francipvb commented Apr 2, 2019

Copy link
Copy Markdown
Contributor

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

@francipvb

Copy link
Copy Markdown
Contributor Author

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

Cheers,

@michaelDCurran

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Collaborator

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

@LeonarddeR

Copy link
Copy Markdown
Collaborator

We have:

I don't think that's related.

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

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

@francipvb

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

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 11:22
@feerrenrut

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

I've merged master and pushed the result.

@feerrenrut feerrenrut merged commit 0be486a into nvaccess:master May 7, 2019
@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

zstanecic commented May 7, 2019 via email

Copy link
Copy Markdown
Contributor

@feerrenrut

Copy link
Copy Markdown
Contributor

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

@zstanecic

zstanecic commented May 7, 2019 via email

Copy link
Copy Markdown
Contributor

@feerrenrut

Copy link
Copy Markdown
Contributor

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade to comtypes 1.1.7

7 participants