Comment 1 by manish on 2013-07-19 09:19
The attached patch adds reading by sentence in ms word with the keys NVDA+control+upArrow and NVDA+control+downArrow.

Using the alt+arrow combinations was not working because the alt key causes the ribbon toolbar in word to get focus and I haven't found a way of supressing this so far.

Comment 2 by manish on 2013-07-19 09:42
If there is a tab character between two sentences, the move to next sentence command is not working with this patch. I'll work on this and submit another patch that works with tab characters between sentences.

Comment 3 by manish on 2013-07-19 12:18
Fixed the issue with tab characters and updated the patch file attached.

Attachment 0001-fix-for-ticket-3288-read-by-sentence-in-word.patch added by manish on 2013-07-19 12:19
Description:
patch with fix for tab characters included.

Attachment 0001-Fix-for-ticket-3288-read-by-sentence-in-word.patch added by manish on 2013-07-21 07:48
Description:
error at end of doc fixed.

Comment 4 by manish on 2013-07-21 07:54
Uploaded a new patch to fix the following problems* :

• run time error at the end of document
• The second part of a sentence having a new line character in the middle was not read while moving forward.
• A tab chaaracter at the end of a sentence was still not correctly handled.

Also attached a small document with sample test sentences.

ps: although I selected replace existing file, I still see the old patch file. there is no option to delete it either. Please ignore the old patch.

Comment 5 by jteh on 2013-07-22 00:36
It seems that most applications don't implement these commands, even Word. Therefore, it's worth questioning whether these are in scope for a screen reader, as users who aren't using a screen reader don't get these commands either. Is this really any easier for a sighted user than a screen reader user?

If these do get implemented, these would be NVDA specific commands and this should be very clear to users. They should probably also work everywhere sentences are supported, not just Word, so the code would need to be in the editableText module.

Comment 7 by siddhartha_iitd on 2014-09-17 06:32
I've taken up this enhancement. I'm not sure how to proceed with generalizing this feature to all editable texts. Please provide your suggestions for the same.
For English locale, I'm thinking of using Regular Expressions to detect beginning and end of a sentence. However, to make it general it has to be implemented for all the locales and, in other locales I'm not sure what characters are used to signify end of sentence.

Comment 9 by jteh on 2014-09-17 14:36
I'd probably just support these commands wherever sentences are supported by the app. You can't reliably support sentences in all other controls, as not all controls are offset-based and moving by character might move more than one Unicode character.

My point in comment:5 is that these commands need to be placed in the EditableText class so they can be used in other places.

Comment 10 by jteh on 2014-09-24 03:39
See also #1873.

Comment 11 by siddhartha_iitd on 2014-12-01 07:30
First draft of the sentence navigation code is available.

Please check the branch in_t3288_sentnav at the following url:
https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

Comment 15 by mdcurran on 2014-12-04 04:18
We would like this code to be moved out of GlobalCommands as otherwise it will run for other controls such as combo boxes etc. Also, I assume that the reason you expand, collapse end rather than using move is because MS Word's move implementation is buggy? If so can you provide some detail on this?

Things to change:

1. Move the work-around code (expand, collapse end) into WordDocumentTextInfo's move, specifically handling sentences, so that MS Word's move works reliably. You only need do this for direction==1 or -1, and endPoint is None.
2. Add nextSentence and previousSentence scripts to editableText.EditableText, using the focus object's TextInfo's move.
3. Add nextSentence and previousSentence scripts to cursorManager.CursorManager. You should be able to make use of CursorManager._caretMovementScriptHelper. Look at the moveByLine_forward script from cursorManager as an example.
   Thanks.

Comment 17 by manish (in reply to comment 15) on 2014-12-25 02:45
I have incorporated the code review comments. The code is available in branch in_t3288 at:
https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

Words previous and next sentence functionality only collapses to the current unit and has problems with white space in the form of tabs in the middle of sentences. I used the attached doc file for sample problems. If we remove the changes I made in the winword Move() method, sentence navigation doesn't work for the various cases listed in this file.
Even after my current changes, The third and sixth sentence examples in the sample file are less than perfect but technically correct since the cursor is placed on the white space once in these cases.
Please let me know if this looks ok. I could not figure out how to test the changes in cursorManager. Can you suggest some way of determining if this code is correct?

Thanks,
Manish
Replying to mdcurran:

We would like this code to be moved out of GlobalCommands as otherwise it will run for other controls such as combo boxes etc. Also, I assume that the reason you expand, collapse end rather than using move is because MS Word's move implementation is buggy? If so can you provide some detail on this?

Things to change:

1. Move the work-around code (expand, collapse end) into WordDocumentTextInfo's move, specifically handling sentences, so that MS Word's move works reliably. You only need do this for direction==1 or -1, and endPoint is None.
2. Add nextSentence and previousSentence scripts to editableText.EditableText, using the focus object's TextInfo's move.
3. Add nextSentence and previousSentence scripts to cursorManager.CursorManager. You should be able to make use of CursorManager._caretMovementScriptHelper. Look at the moveByLine_forward script from cursorManager as an example.

Thanks.

Attachment test-read-by-sentence.docx added by manish on 2014-12-25 02:54
Description:

Comment 18 by manish on 2014-12-25 15:52
Simplified the WordDocument Move changes. All we need to do is to update the caret and collapse the range after the range.move.

Comment 19 by bhavyashah on 2014-12-28 13:23
Hi,
Is this sentence navigation command implementation only for Word 2010 yet?
If so, please, could this be extended to all other programs as well, i.e. a global sentence navigation command?
Hope I made sense.

Comment 20 by manish (in reply to comment 19) on 2014-12-28 17:48
The command is available globally but only word exposes this functionality and so, this currently works in ms word only. The key stroke "alt+up and down arrows are passed onto the underlying program implementation and if it supports sentence navigation, the sentence will be spoken by NVDA.
Replying to bhavyashah:

Hi,

Is this sentence navigation command implementation only for Word 2010 yet?

If so, please, could this be extended to all other programs as well, i.e. a global sentence navigation command?

Hope I made sense.

Comment 22 by mdcurran on 2014-12-29 03:27
The cursorManager implementation is working fine -- good job. I tested that by enabling browse mode in MS Word.

For the editable text implementation:

• In what case have you seen an application natively support alt+downArrow / alt+upArrow to move by sentence? For me, MS Word either doesn't support it, or, this code causes MS Word to jump to the top of the document, putting me in an endless loop when trying to move by sentence. Can you explain your testing/reasoning for sending the gesture?
• The updateCaret in WordDocumentTextInfo's move method should be removed, and instead updateCaret should be called in EditableText after the call to move. The idea is that move should only affect the TextInfo itself, not the caret. Where as the sentence helper function in EditableText knows it is working with the caret.
• Depending on how you answer my question on the sending of the gesture, I would say that you can remove that.

Comment 23 by manish (in reply to comment 22) on 2014-12-29 04:25
Replying to mdcurran:

For the editable text implementation:

• In what case have you seen an application natively support alt+downArrow / alt+upArrow to move by sentence? For me, MS Word either doesn't support it, or, this code causes MS Word to jump to the top of the document, putting me in an endless loop when trying to move by sentence. Can you explain your testing/reasoning for sending the gesture?

The reason I am sending the gesture to the app first is to give the application a chance to use the key strokes, if it natively supports this. I am not aware of such an application, but don't see why NVDA should prevent the keystrokes from being sent to the app first. I basically looked at how the other keystrokes are being handled in the editableText._caretMovementScriptHelper method.
I tested this with several documents but did not run into the infinite loop situation. Is it possible for you to upload a sample doc that will cause this or the steps that cause this to happen? Did you assign the alt+arrow keys in ms word itself to move by sentence?

• The updateCaret in WordDocumentTextInfo's move method should be removed, and instead updateCaret should be called in EditableText after the call to move. The idea is that move should only affect the TextInfo itself, not the caret. Where as the sentence helper function in EditableText knows it is working with the caret.

The updateCaret() and Collapse() calls are specific to how we need to do it in MS word. I am not sure if we will need to call this in another application depending on how the move method for that application's textInfo works.

• Depending on how you answer my question on the sending of the gesture, I would say that you can remove that.

Comment 24 by mdcurran (in reply to comment 23) on 2014-12-29 04:37
Replying to manish:
When I try with MS Word 2010, with the test.docx attached, Starting at the top of the document, pressing alt+downArrow moves to the next line. Pressing it again jumps back to the top of the document for me.
It looks like that for My MS Word at least, alt+downArrow natively jumps to the top of the current section in the document.

As for the updateCaret: This would be needed for any implementation of move, not just MS Word. No implementation of move should be affecting the caret at all, only the TextInfo itself. Note that a TextInfo is a freely moving range of text, disconnected from the caret or selection. For instance, a TextInfo is used for the review cursor...

Attachment Test document.docx added by mdcurran on 2014-12-29 04:38
Description:
test document shoing many features

Comment 25 by manish (in reply to comment 24) on 2014-12-29 05:15
This may be a stupid question...but do you have the latest from the branch?
I tried with the document you uploaded and still do not get the wierd problem you have mentioned.

I will make the change to call updateCaret and collapse calls from the editableText method.

Comment 26 by mdcurran (in reply to comment 25) on 2014-12-29 05:21
Replying to manish:
I'm testing 8af2678.

The call to collapse can stay in the MS Word move method as that is probably specific to MS Word. But do move updateCaret.

Comment 27 by manish (in reply to comment 26) on 2014-12-29 05:50
That is indeed the latest.
I have now removed the code to send the gesture to the app. Please let me know if this fixes the problem you are facing.
I also moved the updateCaret call to editableText.

The collapse call is not required after the latest changes in master, it seems.

However, in the current master, nvda+up and down arrow are currently throwing an exception in ms word. I am not sure if that is work in progress or already being tracked in a separate ticket.

Comment 28 by mdcurran (in reply to comment 27) on 2014-12-29 07:57
Replying to manish:
Thanks. In_t3288 is working great for me.

I can't reproduce the NVDA+down / NVDA+up exceptions. Could you please attach a log or paste the traceback?

Attachment nvda.log added by manish on 2014-12-29 12:43
Description:
the log file with errors for nvda+up and down arrows.

Comment 29 by manish (in reply to comment 28) on 2014-12-29 12:44
Added the log file with the nvda+up and down arrow exceptions.

Comment 30 by manish (in reply to comment 29) on 2014-12-29 12:52
The error went away after I did a fresh scons build. I realized there were changes in the c++ components that were not compiled on my local machine. We are good here.

Comment 31 by mdcurran on 2015-01-08 06:44
I just noticed that it still has trouble when moving to the next sentence when the two sentences are separated by a tab character. The problem is that MS Word's move (sentence) method lands on the tab character as the start of the next sentence, yet MS Word's expand (sentence) method ends up covering both sentences, if positioned on the tab character.
I guess wordDocumentTextInfo's expand method should somehow handle this, I'm not sure.

Comment 32 by manish (in reply to comment 31) on 2015-01-11 00:37
I know of this issue - I mentioned it in my comments above about the third sentence in the test document. I am not sure what the correct behavior should be in this case. Should we skip over all white space explicitly? I am not sure if that will be acceptable to all users. I just left it how ms word handles it out of the box.

Replying to mdcurran:

I just noticed that it still has trouble when moving to the next sentence when the two sentences are separated by a tab character. The problem is that MS Word's move (sentence) method lands on the tab character as the start of the next sentence, yet MS Word's expand (sentence) method ends up covering both sentences, if positioned on the tab character.

I guess wordDocumentTextInfo's expand method should somehow handle this, I'm not sure.

Comment 33 by Michael Curran <mick@... on 2015-02-03 09:52
In [0f3d404]:

Merge branch 't3288' into next. Incubates #3288

Changes:
Added labels: incubating

Comment 34 by Michael Curran <mick@... on 2015-03-16 05:20
In [36d6d96]:

Merge branch 't3288'. Fixes #3288

Changes:
Removed labels: incubating
State: closed

Comment 35 by mdcurran on 2015-03-16 22:38
Changes:
Milestone changed from None to 2015.2

Comment 36 by nvdakor on 2015-06-23 17:55
Hi,
As some people found out recently, it introduces delay when working with large documents. A sample document (an RTF) sent by a user that was 1 megabyte in size exhibits delay when moving to next sentence. The problematic section was trace to textInfos.move implementation in Word (more on #5177).
Thanks.