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

No longer require pyWin32 #9639

Merged
merged 5 commits into from Jun 3, 2019
Merged

No longer require pyWin32 #9639

merged 5 commits into from Jun 3, 2019

Conversation

michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented May 29, 2019

Link to issue number:

None.

Summary of the issue:

Since 2006 NVDA has relied on the pyWin32 package for particular things such as some Windows constants, certain COM interfaces, and clipboard functionality.
However, over the years we have replaced much of the code that relies on pyWin32, except for a small particular code paths.

Description of how this pull request fixes the issue:

this pr totally removes the requirement for pyWin32 by doing the following:

  • hardcodes any constants we were still fetching from win32con, removing the dependency on this module.
  • Directly implements get and set clipboard data functionality, removing the dependency on the win32clipboard module.
  • Directly implements code for richEdit that fetches a text representation of an embedded OLE object, removing the dependency on the pythoncom module.

Testing performed:

  • navigated an older-style richedit document (classic Wordpad) with an embedded Excel object: NVDA could successfully get the text representation on the object.
  • With the PythonConsole, imported api, and ran copyToClip and getClipData functions multiple times to ensure that NVDA can successfully get and set clipboard data.

Known issues with pull request:

None.

Change log entry:

changes for developers:

  • The pyWin32 Python package is no longer included with NVDA.

@feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented May 30, 2019

I think add-on developers currently rely on our inclusion of pyWin32, also we have an updated dependency ready to go: nvaccess/pywin32-bin#1

We may still want to remove pyWin32, but we should talk about what this will mean for add-on authors first, as well as consider other risks.

@beqabeqa473
Copy link
Contributor

@beqabeqa473 beqabeqa473 commented May 30, 2019

nvdaHelper/local/oleUtils.cpp Outdated Show resolved Hide resolved
source/NVDAObjects/window/edit.py Outdated Show resolved Hide resolved
source/api.py Show resolved Hide resolved

GMEM_MOVEABLE=2

class HGLOBAL(HANDLE):
Copy link
Collaborator

@leonardder leonardder May 30, 2019

Choose a reason for hiding this comment

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

Could you add a short descriptive doc string here?

source/winUser.py Outdated Show resolved Hide resolved
source/winUser.py Outdated Show resolved Hide resolved
# But make sure not to free the memory accidentally -- it is not ours
h=winKernel.HGLOBAL(h,autoFree=False)
with h.lock() as addr:
if addr:
Copy link
Collaborator

@leonardder leonardder May 30, 2019

Choose a reason for hiding this comment

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

What happens if not addr?

source/winUser.py Show resolved Hide resolved
source/winUser.py Show resolved Hide resolved
source/winUser.py Outdated Show resolved Hide resolved
@leonardder
Copy link
Collaborator

@leonardder leonardder commented May 30, 2019

I think that most add-ons that utilize pywin32 use win32con. There is also an explicit requirement in setup.py to include win32api, see #7642.

If we decide to keep win32con and win32api, I'm fine with that. I really like getting rid of win32com, though.

@leonardder leonardder added this to In progress in Update NVDA to Python 3 via automation May 30, 2019
@leonardder leonardder moved this from In progress to Needs review in Update NVDA to Python 3 May 30, 2019
@leonardder
Copy link
Collaborator

@leonardder leonardder commented May 31, 2019

It looks like this failed building because PyGetWindow did not install properly for system tests.

@michaelDCurran
Copy link
Member Author

@michaelDCurran michaelDCurran commented May 31, 2019

@leonardder
Copy link
Collaborator

@leonardder leonardder commented May 31, 2019

@michaelDCurran commented on 31 May 2019, 11:54 CEST:

Since we are breaking all add-ons anyway, and there are easy ways to
replace win32api now with ctypes, I don't see the need for keeping
pyWin32 modules as a dependency for the sake of add-on developers.

Agreed

However, if the consensus is to keep them for add-ons...

We could consider keeping win32con, but it feels slightly weird to keep a file with constants only for the sake of add-ons. Furthermore, add-ons can easily bundle it themselves.

@michaelDCurran
Copy link
Member Author

@michaelDCurran michaelDCurran commented Jun 3, 2019

@leonardder Are there any pending review actions from you I need to address?

Update NVDA to Python 3 automation moved this from Needs review to Reviewer approved Jun 3, 2019
Copy link
Collaborator

@leonardder leonardder left a comment

Could you remove the reference to win32api from setup.py as well? AFter that, feel free to merge

@michaelDCurran michaelDCurran merged commit ef3ed13 into threshold Jun 3, 2019
1 check passed
Update NVDA to Python 3 automation moved this from Reviewer approved to Done Jun 3, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jun 3, 2019
@Brian1Gaff
Copy link

@Brian1Gaff Brian1Gaff commented Jun 3, 2019

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jun 3, 2019

@Brian1Gaff
Copy link

@Brian1Gaff Brian1Gaff commented Jun 4, 2019

@Brian1Gaff
Copy link

@Brian1Gaff Brian1Gaff commented Jun 4, 2019

@leonardder
Copy link
Collaborator

@leonardder leonardder commented Jun 4, 2019

This pull request is closed, and I don't think this is the right place to discuss implications for add-on authors. Please use the development list for this if you really think it is necessary that this should be reconsidered.

Again, authors will have to update their add-ons in any case, with or without this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants