-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
Report comments in MS Word with UIA enabled #9631
Conversation
Before merging this, can you provide the try build?
|
@zstanecic the build from this PR is here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this myself, but here are some comments based on just reading the code.
@@ -68,11 +70,16 @@ def getCommentInfoFromPosition(position): | |||
UIAElement=UIAElementArray.getElement(index) | |||
UIAElement=UIAElement.buildUpdatedCache(UIAHandler.handler.baseCacheRequest) | |||
obj=UIA(UIAElement=UIAElement) | |||
if not obj.parent or obj.parent.name!='Comment': | |||
if (obj.parent.role !=controlTypes.ROLE_GROUPING and not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the ``if not obj.parent` here? This breaks if the object has no parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I focused on replacing obj.name and missed it. Thanks for pointing this.
@@ -68,11 +70,16 @@ def getCommentInfoFromPosition(position): | |||
UIAElement=UIAElementArray.getElement(index) | |||
UIAElement=UIAElement.buildUpdatedCache(UIAHandler.handler.baseCacheRequest) | |||
obj=UIA(UIAElement=UIAElement) | |||
if not obj.parent or obj.parent.name!='Comment': | |||
if (obj.parent.role !=controlTypes.ROLE_GROUPING and not | |||
obj.parent.UIAElement.getCurrentPropertyValue(UIAHandler.UIA_IsAnnotationPatternAvailablePropertyId)): | |||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to have some documentation about how you came up with the if clause above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First I wanted to replace obj.name with something which doesn't change and decided for role which is grouping. In case it would be not enough I checked developer info for that object and discovered that comments have AnnotationPattern which may be significant for them. Then I found UIAElement.getCurrentPropertyValue checks for its presence. It requires one parameter which is propertyId. For AnnotationPattern it is 30118 but I didn't want to use that as itself it means nothing. Thats why as a parameter I used UIAHandler.UIA_IsAnnotationPatternAvailablePropertyId which provides that Id.
Thats how the process of creating looked like but I don't know how much of it should be documented. Maybe you have some ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is ok as it is. Thanks for providing the details here. Leaving this treat unresolved until @michaelDCurran or someone else has looked at it.
continue | ||
comment=obj.makeTextInfo(textInfos.POSITION_ALL).text | ||
dateObj=obj.previous | ||
date=dateObj.name | ||
if not dateObj.previous: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is more readable if you check obj.previous.previous first. Now, you are assigning an object to the dateObj variable and then, when it doesn't seem to be a dateObj, you're assigning the same object to authorObj.
For example:
comment=obj.makeTextInfo(textInfos.POSITION_ALL).text
tempObj = obj.previous.previous
authorObj = tempObj or obj.previous
author =authorObj.name
if not tempObj:
return dict(comment=comment,author=author)
dateObj=obj.previous
date=dateObj.name
return dict(comment=comment,author=author,date=date)
@@ -84,6 +91,9 @@ class CommentUIATextInfoQuickNavItem(TextAttribUIATextInfoQuickNavItem): | |||
@property | |||
def label(self): | |||
commentInfo=getCommentInfoFromPosition(self.textInfo) | |||
if len(commentInfo) ==2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if len(commentInfo) ==2: | |
if "date" not in commentInfo: |
@@ -320,6 +330,7 @@ class WordDocument(UIADocumentWithTableNavigation,WordDocumentNode,WordDocumentB | |||
# Microsoft Word duplicates the full title of the document on this control, which is redundant as it appears in the title of the app itself. | |||
name=u"" | |||
|
|||
@script(gesture ="kb:NVDA+alt+c") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lacks the description. Please reuse the description for the script for the object model implementation of Word.
@script(gesture ="kb:NVDA+alt+c") | |
@script( | |
gesture="kb:NVDA+alt+c", | |
# Translators: a description for a script that reports the comment at the caret. | |
description=_("Reports the text of the comment where the System caret is located.") | |
) |
@@ -334,17 +345,20 @@ def script_reportCurrentComment(self,gesture): | |||
UIAElement=UIAElementArray.getElement(index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really amazes me that this is not calling getCommentInfoFromPosition
. Could you please change this in such a way that it uses that function and speaks a message based on what is in the dictionary? I think it is even possible to abstract this further and pas the result of CommentUIATextInfoQuickNavItem.label to ui.message.
If this sounds too complex, I"m happy to take this for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works with getCommentInfoFromPosition but when I tried CommentUIATextInfoQuickNavItem.label I got its adress in a memory or "Property object is not callable" error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be best to create a new _getPresentableCommentInfoFromPosition function that returns the label. Ten, you can make the label property return what that helper function returns for the particular textInfo, and same for the script.
Does that clear things up for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope I understood you well. If so, should this new function has its doc string?
I also added a message for no comments in script.
For me this build works as expected.
Sorry for late reply.
|
@LeonarddeR Could you have another look on that? |
@jakubl7545: Could you merge current master into this? |
@LeonarddeR: done. |
@jakubl7545 I'm sorry that it took me some time to come back to this. Could you plase merge master again and run |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@LeonarddeR It's done. Sorry for such a long delay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jakubl7545 , looks good
@michaelDCurran Would it be possible to merge this PR into 2019.3? It passed a review from @LeonarddeR so I think it's ready. |
Any updates on this PR? CC @michaelDCurran. |
This has been reported several times and it looks like this PR is just awaiting review. @michaelDCurran Do you intent to look at this one? |
I'm quite worried that this implementation might have broken with newer
builds of Office 365. I shall give it a test and report back.
|
What is the latest news of this PR and is it possible to be merged in 2020.4? |
@michaelDCurran Ping. This has been asked about again in #11989 |
@michaelDCurran Can you review this |
This is not reporting comments for me in MS Word build 16.0.13707 nor has it been for many months. Can someone else confirm that this code still works in Microsoft Word for them? We need to also work out what MS Word version this broke in, and perhaps try and come up with a solution that works for both. |
This pr does not work for me with Microsoft Word build 16.0.13707, nor has it worked with builds of MS Word that I have had for some months now. I am right now looking into how we can make this code more robust. At very least, as the annotation pattern is available, we can actually use that pattern to get the needed info directly now. |
Here is an updated copy of getCommentInfoFromPosition which uses annotation properties directly, and does not need to walk as many objects. This code works on my 16.0.13707 build of MS Word:
However, as MS Word now supports comment threads, much of the time, the comment reported will just be "comment thread started by Michael Curran" or similar. Users should really now start viewing comments in the comment thread pain. |
@michaelDCurran Yes, it works for me in MSO (16.0.13530.20368). It reads comment itself, author and date exactly as it should. |
It works fine for me: Microsoft 365 MSO (16.0.13530.20132) 64 Bit |
See test results for failed build of commit 52eb987190 |
@michaelDCurran I've tried your code but it didn't work. Probably because AnnotationType_Comment is not available. Only AnnotationType_Unknown is returned. So I've combined both approaches to ensure that comments are read in different versions of Office. Could you check if it works for you now? |
@jakubl7545 Yes, this is working for me. Thanks for your work on this. |
@jakubl7545 I think there may be one or more review actions from @feerrenrut before he can also approve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes addressed
In version Microsoft 365 MSO (16.0.14228.20200) 64-bit the pr reported an incorrect comment: |
@cary-rowen Please create a new issue for this and preferably attach a sample document reproducing the problem. |
Link to issue number:
closes #9285
Summary of the issue:
With UIA enabled it is impossible to read comments. This is because comments ar filtered by its name which appeared to be different across languages.
Also when there are more than 13 comments dates can't be retrieved because don't fit on the screen.
Description of how this pull request fixes the issue:
Instead of name comments are filtered by its role and AnnotationsPattern.
In situations when date is not on the screen only comment itself and author are reported.
Testing performed:
Tested with NVDA running from source with word document which has comments like the attached one.
Known issues with pull request:
None
Change log entry:
Section: Bug fixes
It is now possible to read comments in MS Word with UIA enabled.
test.docx