PS - in reading the opener, I didn't mean for it to sound like I'm jumping on Joanie for this.  Many many many apologies if it comes across that way.  :-( The intended tone is more of noticing that Joanie had been in this area of code.  I need her help solving the problem because I'm not fully familiar with the intended user experience for this portion of the code.

In particular, soffice's script.py has a 'presentTableInfo' method that might be moved to a _generateTableInfo method in soffice's speech_generator.  We that, we might add a tableInfo section to the formatting strings somewhere (e.g., a new ROLE_PARAGRAPH).

soffice's locusfFocusChanged method is pretty extensive, however, so I think this might end up being rather involved.

Created attachment 136517 [details] [review]
move the decision to the speech generator

It would/will be nice to move the logic currently in presentTableInfo into a _generateWhatever method. Because we have a similar method/need in structural_navigation.py, I will look at doing so as part of that refactor.

In the meantime, to address this bug, why not just move the logic into the speech generator for soffice?

Pylinted and regression tested. Will, please review. Thanks!

(In reply to comment #3)
> In the meantime, to address this bug, why not just move the logic into the
> speech generator for soffice?

Sounds good.

> Pylinted and regression tested. Will, please review. Thanks!

So, I see two things when looking at these two lines (the first is the logic from the old code and the second is the new logic added to the speech generator):

-        if not len(utterances[0]) and self.speakBlankLine(newFocus):
+            if not len(result) and settings.speakBlankLines:

1) The old logic looks at the length of the very first string returned by the generator whereas the new logic just looks at the length of the utterances. I'm not sure if there was any significance to looking at just utterances[0] and not just len(utterances).

2) The old logic calls soffice's speakBlankLine method, which performs some extra logic to determine if the line is blank.  I dunno if this is still needed with the new generator or not.

(In reply to comment #4)

> So, I see two things when looking at these two lines (the first is the logic
> from the old code and the second is the new logic added to the speech
> generator):
> 
> -        if not len(utterances[0]) and self.speakBlankLine(newFocus):
> +            if not len(result) and settings.speakBlankLines:
> 
> 1) The old logic looks at the length of the very first string returned by the
> generator whereas the new logic just looks at the length of the utterances. I'm
> not sure if there was any significance to looking at just utterances[0] and not
> just len(utterances).

Prior to the speechgenerator refactor, getSpeech would result in _getSpeechForTableCellRow being called which would in turn call _getSpeechForTableCell, which would eventually get around to doing this:

        displayedText = self._script.getDisplayedText( \
                          self._script.getRealActiveDescendant(obj))
        if not already_focused and not displayedText in utterances:
            utterances.append(displayedText)

getDisplayedText would return an empty string. Therefore, utterances wound up being [''].

Post speechgenerator refactor, we're getting a truly empty result/utterances under the same conditions -- or a result/utterances beginning with a pause. Hence the traceback.

> 2) The old logic calls soffice's speakBlankLine method, which performs some
> extra logic to determine if the line is blank.  I dunno if this is still needed
> with the new generator or not.

I don't believe it is needed. 

The old logic called speakBlankLines with newFocus (a paragraph within the cell) which would implement the accessible text interface). The new logic is in the speechgenerator which is looking at the cell itself, which does not implement the accessible text interface. So calling the soffice script's speakBlankLine would bail automatically unless we do additional work so that speakBlankLine can consider the child of cell which we care about:

    def speakBlankLine(self, obj):
        """Returns True if a blank line should be spoken.
        Otherwise, returns False.
        """

        # Get the the AccessibleText interface.
        try:
            text = obj.queryText()
        except NotImplementedError:
            return False

And if we did decide to do that work I don't see how things would be different. If len(result) == 0, it is because there is no text in the cell. Therefore, for the empty child paragraph, getTextAtOffset would return ('', 0, 0).

        # Get the line containing the caret
        caretOffset = text.caretOffset
        line = text.getTextAtOffset(caretOffset, \
            pyatspi.TEXT_BOUNDARY_LINE_START)

        # If this is a blank line, announce it if the user requested
        # that blank lines be spoken.
        if line[1] == 0 and line[2] == 0:
            return settings.speakBlankLines

which would cause speakBlankLine to return settings.speakBlankLines, which is what the new logic does.

Right??? <she says, popping more migraine meds and hoping she's not lost her mind> :-)

(In reply to comment #5)
> Right??? <she says, popping more migraine meds and hoping she's not lost her
> mind> :-)

Sounds right to me.  Many thanks for the analysis.  :-)

(In reply to comment #6)
> (In reply to comment #5)
> > Right??? <she says, popping more migraine meds and hoping she's not lost her
> > mind> :-)
> 
> Sounds right to me.  Many thanks for the analysis.  :-)

Phew! :-) Thanks.

Patch committed to master. Closing as FIXED.