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

Improve working with message lists and calendars in Outlook #7949

Merged
merged 17 commits into from Apr 30, 2018

Conversation

@leonardder
Collaborator

leonardder commented Jan 31, 2018

Link to issue number:

Fixes #6911
Fixes #8028
Reverts #6219

Summary of the issue:

  1. In the Outlook messages list, replied to or forwarded status is not announced by NVDA.
  2. In outlook's calendar view, the month view is overly verbose, announcing redundant information (e.g. 00.00AM to next day 00.00AM
  3. When a message has an associated draft, this is not reported.

Description of how this pull request fixes the issue:

  1. This pr gets the announcement of replied/forwarded state from the UIA value of rows in the Outlook messages list. NVDA will now also announce meeting requests based on this information, as well as appointment cancellations.

    The initial implementation of this pr relied on the Outlook object model. However, this caused security warnings on some systems with a tight security policy.

  2. When an appointment or time slot is now exactly one day (86400 seconds), and starts at 00.00, the time slot is announced as 26 february 2018 (entire day) instead of 26 februari 2018 00.00 to 27 februari 2018 00.00. While looking at this code, I noticed that Microsoft strongly discourages the use of kernel32 GetTimeFormat and GetDateFormat functions, so I changed this implementation to use the *Ex alternatives for these functions.

  3. The associated draft column in Outlook 2016 is no longer silenced. After implementing #6219, it seems Microsoft decided to present an empty text node when there is no associated draft, rather than spamming everyone with the "no associated draft" message. Therefore when this particular pr is merged, everyone who gets "no associated draft" for almost every message is advised to update Outlook 2016 to the most recent build.

Testing performed:

  1. Made sure that the current implementation doesn't cause security warnings when setting outlook's program access settings to the most secure option available.
  2. Tested replied/forwarded messages in Outlook 2016 and 2013, the state was reported correctly. The replied state did even report for messages I replied to using Thunderbird. Also tested this in Outlook 2010, and as Outlook 2010 doesn't expose this information using UIA, we fall back to the object model for the unread state.
  3. Tested the announcement of a whole day appointment in the calendar

Known issues with pull request:

  1. The replied or forwarded state is localized by Outlook itself, not by NVDA. This shouldn't be a problem though, as the headers are also localized by Outlook itself.

  2. In theory, Outlook could expose every piece of relevant or irrelevant information using the UIA value. I've only seen the following strings until now:

    • Message: usually the first word in the value of a message
    • Contact: usually the first word in the value of a contact when the contacts are displayed in a list view
    • Appointment: usually the first word in the value of a calendar item when the calendar is displayed in a list view
    • Read/Unread: usually the last word in the value of a message, contact or appointment
    • Forwarded/Replied: usually the second word in a string containing three words, e.g. Message Replied Read

    I'm quite sure JAWS also relies on the value here, as replied/forwarded state is localized using Outlook, not using JAWS. The wild should make clear whether parsing the value this way is too non-restrictive.

  3. The replied to all state is not reported as it is not exposed using the object's value.

  4. When the root of a conversation in conversation view has focus, NVDA reads the headers of every single message in the conversation. There is currently no suitable workaround for this. Note that this was already the case before this pr. This issue is caused by a bug in UIA, where the tree scope is ignored when providing a custom tree filter for a cache request.

Change log entry:

  • New features

    • In Microsoft Outlook message lists, NVDA now reports if a message has been replied to or forwarded. (#6911)
  • Changes

    • NVDA is now less verbose when an appointment or time slot in the Outlook calendar covers an entire day. (#7949)
@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Feb 2, 2018

Collaborator

Blocked by #7953

Collaborator

leonardder commented Feb 2, 2018

Blocked by #7953

@leonardder leonardder added the blocked label Feb 2, 2018

Implementation had to be changed completely.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Feb 3, 2018

Collaborator

I had to change this into a completely different implementation due to #7953. @michaelDCurran. It would be great if you could review this as soon as possible to fix #7953.

Collaborator

leonardder commented Feb 3, 2018

I had to change this into a completely different implementation due to #7953. @michaelDCurran. It would be great if you could review this as soon as possible to fix #7953.

@leonardder leonardder removed the blocked label Feb 3, 2018

@leonardder leonardder changed the title from Show replied/forwarded status for messages in Outlook to Improve working with message lists and calendars in Outlook Feb 26, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Feb 26, 2018

Collaborator

@michaelDCurran: I made the scope of this pr somewhat broader and also addressed some other concerns there were with the current way of dealing with Outlook. The initial message of the pr covers all relevant information.

Collaborator

leonardder commented Feb 26, 2018

@michaelDCurran: I made the scope of this pr somewhat broader and also addressed some other concerns there were with the current way of dealing with Outlook. The initial message of the pr covers all relevant information.

Show outdated Hide outdated source/appModules/outlook.py
Show outdated Hide outdated source/appModules/outlook.py
Show outdated Hide outdated source/appModules/outlook.py
Show outdated Hide outdated source/appModules/outlook.py
Show outdated Hide outdated source/appModules/outlook.py
Show outdated Hide outdated source/appModules/outlook.py
@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Mar 5, 2018

Collaborator

what about saying "all day" instead of "entire day"

Collaborator

derekriemer commented Mar 5, 2018

what about saying "all day" instead of "entire day"

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Mar 5, 2018

Collaborator

@derekriemer commented on 5 mrt. 2018 15:53 CET:

what about saying "all day" instead of "entire day"

It is now saying "all day" as per the last commits from today morning.

Collaborator

leonardder commented Mar 5, 2018

@derekriemer commented on 5 mrt. 2018 15:53 CET:

what about saying "all day" instead of "entire day"

It is now saying "all day" as per the last commits from today morning.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Mar 6, 2018

Collaborator

@michaelDCurran, this pr has been overhauled and is ready for another review I think. I'm afraid there's no way to work around relying on the localized contents of the UIA value.

Collaborator

leonardder commented Mar 6, 2018

@michaelDCurran, this pr has been overhauled and is ready for another review I think. I'm afraid there's no way to work around relying on the localized contents of the UIA value.

@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Mar 15, 2018

Collaborator

Hi,

I wonder if this is the right PR for this: in Outlook 2016's Inbox view, when I perform read current line (caret) command with no messages shown, I get:

Collaborator

josephsl commented Mar 15, 2018

Hi,

I wonder if this is the right PR for this: in Outlook 2016's Inbox view, when I perform read current line (caret) command with no messages shown, I get:

@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Mar 15, 2018

Collaborator

Thanks.

Collaborator

josephsl commented Mar 15, 2018

Thanks.

@josephsl

This comment has been minimized.

Show comment
Hide comment
@josephsl

josephsl Mar 15, 2018

Collaborator

Hi,

Er, I don't know how the traceback got cut off...

{{{
ERROR - scriptHandler.executeScript (22:15:33.871):
error executing script: <bound method GlobalCommands.script_reportCurrentLine of <globalCommands.GlobalCommands object at 0x0584E7D0>> with gesture u'NVDA+up arrow'
Traceback (most recent call last):
File "scriptHandler.pyc", line 187, in executeScript
File "globalCommands.pyc", line 142, in script_reportCurrentLine
File "textInfos\offsets.pyc", line 348, in expand
File "textInfos\offsets.pyc", line 330, in _getUnitOffsets
File "textInfos\offsets.pyc", line 249, in getLineOffsets
File "NVDAObjects_init
.pyc", line 42, in _getStoryText
File "baseObject.pyc", line 34, in get
File "baseObject.pyc", line 115, in getPropertyViaCache
File "NVDAObjects_init
.pyc", line 1027, in _get_basicText
File "baseObject.pyc", line 34, in get
File "baseObject.pyc", line 115, in _getPropertyViaCache
File "appModules\outlook.pyc", line 451, in get_name
File "comtypes_init
.pyc", line 279, in getattr
ValueError: NULL COM pointer access
}}}

Collaborator

josephsl commented Mar 15, 2018

Hi,

Er, I don't know how the traceback got cut off...

{{{
ERROR - scriptHandler.executeScript (22:15:33.871):
error executing script: <bound method GlobalCommands.script_reportCurrentLine of <globalCommands.GlobalCommands object at 0x0584E7D0>> with gesture u'NVDA+up arrow'
Traceback (most recent call last):
File "scriptHandler.pyc", line 187, in executeScript
File "globalCommands.pyc", line 142, in script_reportCurrentLine
File "textInfos\offsets.pyc", line 348, in expand
File "textInfos\offsets.pyc", line 330, in _getUnitOffsets
File "textInfos\offsets.pyc", line 249, in getLineOffsets
File "NVDAObjects_init
.pyc", line 42, in _getStoryText
File "baseObject.pyc", line 34, in get
File "baseObject.pyc", line 115, in getPropertyViaCache
File "NVDAObjects_init
.pyc", line 1027, in _get_basicText
File "baseObject.pyc", line 34, in get
File "baseObject.pyc", line 115, in _getPropertyViaCache
File "appModules\outlook.pyc", line 451, in get_name
File "comtypes_init
.pyc", line 279, in getattr
ValueError: NULL COM pointer access
}}}

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Mar 15, 2018

Collaborator

@josephsl commented on 15 mrt. 2018 06:18 CET:

I wonder if this is the right PR for this.

Looks like it is. It turns out it wasn't a good idea to remove the tree filter from the cache request that forced the fetched children to be text controls. I now added a new tree filter that requires the children to be content children. After that, I couldn't reproduce the issue any more.

I also noticed another bug in the code that tried to access the object model for flag information when there was no selection. That bug was actually obfuscated by the treeFilter one. We are now ignoring flags when there is no object model access, just as we did before this pr.

Collaborator

leonardder commented Mar 15, 2018

@josephsl commented on 15 mrt. 2018 06:18 CET:

I wonder if this is the right PR for this.

Looks like it is. It turns out it wasn't a good idea to remove the tree filter from the cache request that forced the fetched children to be text controls. I now added a new tree filter that requires the children to be content children. After that, I couldn't reproduce the issue any more.

I also noticed another bug in the code that tried to access the object model for flag information when there was no selection. That bug was actually obfuscated by the treeFilter one. We are now ignoring flags when there is no object model access, just as we did before this pr.

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Mar 28, 2018

Collaborator

Currently blocked by #8129. When getting the name for message list items, a NULL COM pointer access error is raised because the custom UIA query fails. This is a bit strange, since until now, it fails only on two systems. @bramd has a system where it fails and another system where it works correctly, same windows and office builds.

Note that such an error has been reported by @josephsl in #7949 (comment), the fix for that has yet not been committed to Next. However, testing by @bramd tells that the fix for #7949 (comment) does not aplly to this particular case of the null com pointer access error.

I guess we can do an if check on cachedChildren, but I'd like to no why getting cached children would reveal a null com pointer in the first place. @michaelDCurran: Have you seen these cases earlier?

Collaborator

leonardder commented Mar 28, 2018

Currently blocked by #8129. When getting the name for message list items, a NULL COM pointer access error is raised because the custom UIA query fails. This is a bit strange, since until now, it fails only on two systems. @bramd has a system where it fails and another system where it works correctly, same windows and office builds.

Note that such an error has been reported by @josephsl in #7949 (comment), the fix for that has yet not been committed to Next. However, testing by @bramd tells that the fix for #7949 (comment) does not aplly to this particular case of the null com pointer access error.

I guess we can do an if check on cachedChildren, but I'd like to no why getting cached children would reveal a null com pointer in the first place. @michaelDCurran: Have you seen these cases earlier?

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Mar 28, 2018

Contributor
Contributor

michaelDCurran commented Mar 28, 2018

michaelDCurran added a commit that referenced this pull request Mar 28, 2018

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Apr 4, 2018

Collaborator

I reverted the announcement of importance, attachments and flags based on UIA children, as it is unreliable in conversation view. The announcement of these fields was based on the assumption that the UIA implementation treats relevant fields as content (i.e. with the IsContentElement property) whereas irrelevant fields (no attachment, no reminder) are non content. However, when conversation view is enabled, none of the relevant children have the IsContentElement property set.

Collaborator

leonardder commented Apr 4, 2018

I reverted the announcement of importance, attachments and flags based on UIA children, as it is unreliable in conversation view. The announcement of these fields was based on the assumption that the UIA implementation treats relevant fields as content (i.e. with the IsContentElement property) whereas irrelevant fields (no attachment, no reminder) are non content. However, when conversation view is enabled, none of the relevant children have the IsContentElement property set.

Properly get the value for grid rows, also when they are dynamic type…
… mixed with tree view items (e.g. in conversation view)
@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Apr 12, 2018

Collaborator

@michaelDCurran: Could you please review the last few commits?

Collaborator

leonardder commented Apr 12, 2018

@michaelDCurran: Could you please review the last few commits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment