1.  Firefox apparently doesn't like sentence boundaries.  If I change default.py's textLines() to use line bounaries instead (and if a number of other things happen to be the case), I can get at least a single line out of SayAll in Firefox.

2. Having made the adjustment, after SayAll reads the first line, I get the following error:

Traceback (most recent call last):

+ Trace 125051

3.  Even with the adjustment made in #1 above, sometimes you still get nothing at all.  :-(

Still investigating....

Hang five on this for a bit Joanie. Will and I just talked. It's
probable that the solution I'll have to put in place for Evolution
will be close to what is needed for Firefox.

Stay tuned...

Created attachment 86020 [details] [review]
patch to hopefully solve the problem

Seems to work....

Mike can you give this a spin and let me know what you think?

Created attachment 86023 [details] [review]
minor revision to catch more list numbers

It sure seems to be working a whole lot better than it was.  I've only tested on a few sites so far but I like it.  

Will and I talked about this and he asked some really good questions, like "Why did you do such-and-such?" And my response was generally, "I don't recall, things were blowing up or getting stuck, and .... well.... it made sense at the time." :-)

So now I'm going back over Will's question list, recreating things and documenting them.  I'll do one question at a time as I recreate them.

WW Question:  Why is there all that stuff in the "moreLines" (tail end) section of textLines?

JD Answer:  Turns out that in certain circumstances, a recursive call to getObjectContentsAtOffset was blowing up thusly:

Traceback (most recent call last):

+ Trace 126690

If we check for an actual child before making the recursive call in getObjectContentsAtOffset(): 

            if text[offset] == self.EMBEDDED_OBJECT_CHARACTER:
                child = obj.child(self.getChildIndex(obj, offset))
                if child:
                    contents.extend(self.getObjectContentsAtOffset(child, 0))

I can do what you suggest, namely, simplify the moreLines stuff:

            moreLines = False
            while obj and not moreLines:
                obj = self.findNextObject(obj)
                if obj:
                    [obj, offset] = self.findFirstCaretContext(obj, offset)
                    moreLines = True
            if not moreLines:
                done = True

> Will and I talked about this and he asked some really good questions, like "Why
> did you do such-and-such?"

But in a nice way.  ;-)

> I can do what you suggest, namely, simplify the moreLines stuff:
> 
>             moreLines = False
>             while obj and not moreLines:
>                 obj = self.findNextObject(obj)
>                 if obj:
>                     [obj, offset] = self.findFirstCaretContext(obj, offset)
>                     moreLines = True
>             if not moreLines:
>                 done = True

Can self.findFirstCaretContext(obj, offset) return an obj == None?

> But in a nice way.  ;-)

For which I am undeserving, but nonetheless grateful. :-)

> Can self.findFirstCaretContext(obj, offset) return an 
> obj == None?

The fact that you're asking makes me think it can.  :-) I originally had done this;

                if obj:
                    [obj, offset] = self.findFirstCaretContext(obj, offset)
                    if obj:
                        moreLines = True

And then took out the second check after looking at findFirstCaretContext().   It seemed *to me* that as long as you called it with a obj != None, you were going to get that obj back.  It might have a characterOffset of -1, but that's not the same issue.  So.... Under what circumstances will it return None?

As long as I'm taking a comment break...

You had suggested clumping all the object utterance stuff together.  Based on our follow-up conversation, I made a new clumpUtterances() which is pretty much speakContents().  speakContents() now calls clumpedUtterances().  textLines() does as well.  And for the most part, the length of contents and clumped are the same, which makes for much cleaner textLines() code.  It may even solve the issue of getting line numbers.  

There are a few instances where the length of contents is not the same as clumped.  One of those instances seems to be the result of how we added the speaking of heading levels.  We had:

                if level:
                    utterances.append([" ", self.getACSS(obj, " ")])
                    # Translators: this is in reference to a heading level
                    # in HTML (e.g., For <h3>, the level is 3).
                    #
                    utterances.append([_("level %d") % level, None])

If we change that to:

                if level:
                    acss = self.getACSS(obj, " ")
                    # Translators: this is in reference to a heading level
                    # in HTML (e.g., For <h3>, the level is 3).
                    #
                    utterances.append([" " + _("level %d") % level, acss])

contents and clumps associated with headings match.  Is there a reason why we don't want to make that change?

Now it's just a matter of tracking down the other (few) occasions where clumps and contents don't quite match....

> > Can self.findFirstCaretContext(obj, offset) return an 
> > obj == None?
> 
> The fact that you're asking makes me think it can.  :-) 

Naw...no Socratic method related to that query.  Just laziness on my part.  I just checked the code and it looks like it cannot return obj == None.

> You had suggested clumping all the object utterance stuff together.  Based on
> our follow-up conversation, I made a new clumpUtterances() which is pretty much
> speakContents().  speakContents() now calls clumpedUtterances().  textLines()
> does as well. 

Cool.  Definitely sounds clean.  :-)

> ...
>                     utterances.append([_("level %d") % level, None])
> 
> If we change that to:
> 
>                     utterances.append([" " + _("level %d") % level, acss])
> 
> contents and clumps associated with headings match.  Is there a reason why we
> don't want to make that change?

Looks good to me.  :-)

> Now it's just a matter of tracking down the other (few) occasions where clumps
> and contents don't quite match....

The clumping is intended to build up complete strings based upon contiguous utterances with the same ACSS.  Thus...the clumped stuff may end up being smaller in length by design.  What is the impact on the code you're working on if they are not the same length?

 
> Naw...no Socratic method related to that query.  Just laziness on my part.  I
> just checked the code and it looks like it cannot return obj == None.

Phew!  :-)
 
> Cool.  Definitely sounds clean.  :-)

It's much lovelier.  Thanks for asking all the questions you did.  As usual, you were correct, and the end result will be much better.  Just a few more kinks to work out....
 
> Looks good to me.  :-)

Actually, there's a difference.  Mike mentioned the possibility and I didn't notice it until I slowed speech down a bit.  The difference is:

Your way: "heading <pause> level x"
My proposed way: "heading level x"

If the pause is important, scratch my proposal.
 
> The clumping is intended to build up complete strings based upon contiguous
> utterances with the same ACSS.  Thus...the clumped stuff may end up being
> smaller in length by design.  What is the impact on the code you're working on
> if they are not the same length?

Clumped is almost always smaller than utterances -- as expected.  The problem is that clumps and contents are sometimes not the same length.  In the case of headings, I wind up with one more clumped than contents -- if I keep the speaking of levels the way it is (otherwise they wind up being the same length). In the case of sections, paragraphs, lists, and list items, I wind up with more contents than clumped.  In the latter case, sayAll dies with a traceback.  I *suspect* this has something to do with what we're doing in getObjectContentsFromOfset().  As an example, when I do a sayAll with my new version on our wiki, the TOC gets rendered along the lines of:

Contents 1.
Welcome to Orca link 2.
About link 3.
[...]
How Can I Help? link 7.
More Information link <traceback>

I think in some cases we're extending when we should be appending.  I'll investigate that more further later.  Sometimes you have to take a break and look at something different to clear your mind.  I chose to look at the separator issue.  Amusing (to me) findings to be posted on that bug momentarily.

Created attachment 86456 [details] [review]
new version

Will, I *believe* I addressed all of your previous questions in this version of the patch.

The differences in the number of clumps versus the number of contents was the result of the characterOffset being given to findFirstCaretContext() -- which is one of the things I think you asked me about. :-)  Start with an appropriate offset and, with the exception of headings, the number of clumps and contents match.  Go figure.... :-) :-)

Last, but not least, it seems that if you start the SayAll from an image that is a link (e.g. the Orca logo on our wiki), SayAll() just reads that image.  We're hitting the else In default.py's sayAll():

    def sayAll(self, inputEvent):
        if not orca_state.locusOfFocus:
            pass
        elif orca_state.locusOfFocus.text:
            speech.sayAll(self.textLines(orca_state.locusOfFocus),
                          self.__sayAllProgressCallback)
        else:
            speech.speakUtterances(
                self.speechGenerator.getSpeech(orca_state.locusOfFocus, False))
        return True

locusOfFocus in this instance is the image which we set as locusOfFocus in Gecko.py's onFocus().

Does this mean we want to override sayAll() in Gecko.py or adjust it in default.py or something else?

Thanks!

P.S.  I see that Rich is working on SayAll preferences.  The above patch doesn't take his changes into account.  When those are in place, I'll adjust accordingly.

> is one of the things I think you asked me about. :-)  Start with an appropriate
> offset and, with the exception of headings, the number of clumps and contents
> match.  Go figure.... :-) :-)

I'm still confused about the comparison between the number of clumps and contents.  For example, let's assume we had 5 check boxes on a single line, and that label for/by relations were set up appropriately:

[x] red [x] blue [x] green [x] yellow [x] cyan

I *think* we'd end up with contents of length 10, an utterances of length 5, and  a clump of length 1.  But...am I missing something?

> We're hitting the else In default.py's sayAll():
> 
>     def sayAll(self, inputEvent):
>         if not orca_state.locusOfFocus:
>             pass
>         elif orca_state.locusOfFocus.text:
>             speech.sayAll(self.textLines(orca_state.locusOfFocus),
>                           self.__sayAllProgressCallback)
>         else:
>             speech.speakUtterances(
>                 self.speechGenerator.getSpeech(orca_state.locusOfFocus, False))
>         return True

The original SayAll code was assuming it was dealing with a single large text object.  We're finding that this assumption is only good for something like gedit and (maybe) gnome-terminal.  In other apps, such as OpenOffice (which breaks things into paragraphs) and Firefox (where anything goes), this assumption is false.

> Does this mean we want to override sayAll() in Gecko.py or adjust it in
> default.py or something else?

I'm not sure.  Part of me thinks maybe the 'else' clause should be moved to the 'if not text' clause at the beginning of textLines:

if not text:
  yield self.speechGenerator.getSpeech(orca_state.locusOfFocus, False))
  return

I guess __sayAllProgressCallback should then make sure context.obj.text exists before calling setCaretOffset.

But, this wouldn't catch the case in Firefox where sayAll was started on an object without text.  So...that kind of calls for overriding sayAll in Firefox.

I'm not sure, though.  What do you think?

Created attachment 86462 [details]
test case/Will's example

> I'm still confused about the comparison between the number of clumps 
> and contents.  For example, let's assume we had 5 check boxes on a
> single line, and that label for/by relations were set up appropriately:
>
> [x] red [x] blue [x] green [x] yellow [x] cyan
>
> I *think* we'd end up with contents of length 10, an utterances of 
> length 5, and  a clump of length 1.  But...am I missing something?

I added some debugging in my latest patch in the while loop after:

            contents = self.getObjectContentsAtOffset(obj, characterOffset)
            utterances = self.getUtterancesFromContents(contents)
            clumped = self.clumpUtterances(utterances)

And tossed sayAll() at the test case I just attached.  Seems that each checkbox is its own object as is each label. 

--------------------------------------------------------
obj role: paragraph
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Which colors do you like?
utterances[0][0]: Which colors do you like?
contents[0]: [<orca.atspi.Accessible instance at 0x86258ac>, 0, 25]
--------------------------------------------------------
obj role: check box
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Red check box not checked
utterances[0][0]: Red check box not checked
contents[0]: [<orca.atspi.Accessible instance at 0x861ef2c>, -1, -1]
--------------------------------------------------------
obj role: label
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Red
utterances[0][0]: Red
contents[0]: [<orca.atspi.Accessible instance at 0x862544c>, 0, 3]
--------------------------------------------------------
obj role: check box
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Blue check box not checked
utterances[0][0]: Blue check box not checked
contents[0]: [<orca.atspi.Accessible instance at 0x862548c>, -1, -1]
--------------------------------------------------------
obj role: label
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Blue
utterances[0][0]: Blue
contents[0]: [<orca.atspi.Accessible instance at 0x86253cc>, 0, 4]
--------------------------------------------------------
obj role: check box
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Green check box not checked
utterances[0][0]: Green check box not checked
contents[0]: [<orca.atspi.Accessible instance at 0x86254ac>, -1, -1]
--------------------------------------------------------
obj role: label
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Green
utterances[0][0]: Green
contents[0]: [<orca.atspi.Accessible instance at 0x862530c>, 0, 5]
--------------------------------------------------------
obj role: check box
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Yellow check box not checked
utterances[0][0]: Yellow check box not checked
contents[0]: [<orca.atspi.Accessible instance at 0x8877dec>, -1, -1]
--------------------------------------------------------
obj role: label
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Yellow
utterances[0][0]: Yellow
contents[0]: [<orca.atspi.Accessible instance at 0x8877c4c>, 0, 6]
--------------------------------------------------------
obj role: check box
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Yellow check box not checked
utterances[0][0]: Yellow check box not checked
contents[0]: [<orca.atspi.Accessible instance at 0x861ee4c>, -1, -1]
--------------------------------------------------------
obj role: label
len(clumped): 1, len(utterances): 1, len(contents): 1
clumped[0][0]: Cyan
utterances[0][0]: Cyan
contents[0]: [<orca.atspi.Accessible instance at 0x8877c8c>, 0, 4]

> I *think* we'd end up with contents of length 10, an utterances of 
> length 5, and  a clump of length 1.  But...am I missing something?

A lightbulb just went off (maybe).  Are you thinking getLineContentsAtOffset()?  THAT will result in what you describe.  I'm using getObjectContentsAtOffset() so that we can leave it up to the synthesizer to parse/correctly pause after sentences on our behalf.  

(In reply to comment #16)
> > I *think* we'd end up with contents of length 10, an utterances of 
> > length 5, and  a clump of length 1.  But...am I missing something?
> 
> A lightbulb just went off (maybe).  Are you thinking getLineContentsAtOffset()?
>  THAT will result in what you describe.  I'm using getObjectContentsAtOffset()
> so that we can leave it up to the synthesizer to parse/correctly pause after
> sentences on our behalf.  

Ahh...that's it!  I was thinking getLineContentsAtOffset.  But, if you pass an uber object (e.g., the list object holding a bulleted list) to getObjectContentsAtOffset, I think might not see the 1:1 mapping.  All I'm trying to say is that I think this might be a red herring.

You do raise a very good point with the red/blue/green/cyan/yellow case, though.  In having textLines go object by object, we can easily run into the case where we might hear the label spoken twice: once when we speak the check box, and then again when we hit the label.  Are you running into this?  If so, I'm not quite sure of a graceful way around that.  Maybe have textLines skip a label if it is a label for something?

Created attachment 86621 [details] [review]
includes handling for the new sayAllBy setting

This version:

1. adds in support for say all by line.  I stole all of Will's hard work with goNextLine() and put it in a findNextLine() so that we can find lines as well as objects now.  Adjusted goNextLine() so that it calls findNextLine().  Did the same with previousLine() for the sake of symmetry.  Also adjusted the progress callback:  The way it was before is fine for dealing with whole objects, but not mere lines within objects:  For instance, you kept landing near the top of the current paragraph because currentOffset was always far greater than len(utterance) - 1.

2. overrides default.py's sayAll().

3. Seems to work.  :-) Mike, mind giving it a spin?

This version does *not* address Will's question w.r.t. the checkboxes in any way:

> In having textLines go object by object, we can easily run into the
> case where we might hear the label spoken twice: once when we speak
> the check box, and then again when we hit the label.  Are you 
> running into this?  If so, I'm not quite sure of a graceful way around
> that.  Maybe have textLines skip a label if it is a label for something?

Yes, I'm running into that as well.

When I posted the checkbox test case, I mistakenly (fortuitously?) neglected to update the id of the last checkbox.  As a result, what's on the screen is the label "Cyan," but that checkbox is called the "yellow" checkbox by Orca, just like its predecessor.  If we were to skip the label, we'd no longer hear the on-screen text "Cyan."   

Independent of sayAll() and textLines(), if you use Tab to move focus to the Cyan checkbox, Orca also calls it the "yellow" checkbox and doesn't speak the "Cyan" text at all.  Granted, this IS because I screwed up in the HTML. :-)  But it seems like we should speak the on-screen text....  I also wonder if there are times when a page contains checkboxes that have both text and a label, each of which are different and each of which are meaningful.  So maybe we want to only skip a label in textLines() if the label is not the same as the associated ID text??  Then again, given my test case, will it be confusing to sometimes hear the onscreen text and sometimes not hear it?

Thoughts?

> 1. adds in support for say all by line.  I stole all of Will's hard work with
> goNextLine() and put it in a findNextLine() so that we can find lines as well
> as objects now.  Adjusted goNextLine() so that it calls findNextLine().  Did
> the same with previousLine() for the sake of symmetry.  Also adjusted the
> progress callback:  The way it was before is fine for dealing with whole
> objects, but not mere lines within objects:  For instance, you kept landing
> near the top of the current paragraph because currentOffset was always far
> greater than len(utterance) - 1.

Cool.

> When I posted the checkbox test case, I mistakenly (fortuitously?) neglected to
> update the id of the last checkbox.  As a result, what's on the screen is the
> label "Cyan," but that checkbox is called the "yellow" checkbox by Orca, just
> like its predecessor.  If we were to skip the label, we'd no longer hear the
> on-screen text "Cyan."   

Aha.  Yeah...the code definitely doesn't provide any defensive mechanisms for that kind of thing.  

Assuming the HTML wasn't screwy, though, what should be presented for the following when you up/down arrow to the line and/or do a SayAll by line?

[ ] Red [ ] Blue [ ] Green [ ] Yellow [ ] Cyan

I'm guessing both the up/down arrow and the SayAll should emit the same thing.  And it should be something like this for speech:

"Red checkbox not checked Blue checkbox not checked ..."

and this for Braille:

"Red < > Checkbox Blue < > Checkbox Green < > Checkbox Yellow < > Checkbox Cyan < > Checkbox"

Is that right?

> So maybe
> we want to only skip a label in textLines() if the label is not the same as the
> associated ID text?? 

How would this work?  Would we follow the label for property on the label and examine the labelled object some how?  For example, make the assumption that the object's name is always going to be the text of the label that's labelling it?

> Independent of sayAll() and textLines(), if you use Tab to move focus to the
> Cyan checkbox, Orca also calls it the "yellow" checkbox and doesn't speak the
> "Cyan" text at all.  Granted, this IS because I screwed up in the HTML. :-) 

Yeah.  I'm not quite sure what to do here.  You called it the Yellow checkbox, so it's the Yellow check box.  The Cyan text should speak if you arrow to it, though.

> But it seems like we should speak the on-screen text....  

Agreed.  I'd guess that the change above (e.g., skip a label in textLines) should probably live in isLabellingContents.  That way, it could be handled by various things that use it.

> associated ID text??  Then again, given my test case, will it be confusing to
> sometimes hear the onscreen text and sometimes not hear it?

Probably would be confusing, but what can you do when a knucklehead is making up lies?  :-)

Created attachment 86665 [details] [review]
make sayAll by sentence handle labels consistently with sayAll by line

> I'm guessing both the up/down arrow and the SayAll should emit 
> the same thing. And it should be something like this for speech:
>
> "Red checkbox not checked Blue checkbox not checked ..."

Yup, that was already the case for SayAll by line -- not surprisingly.  The problem was strictly with SayAll by sentence.  In particular, the problem was that we were calling getUtterancesFromContents() with a single object, namely a label.  getUtterancesFromContents() was in turn calling isLabellingContents() with the label and a list of one item (i.e. the label).  The label isn't labeling itself, therefore the conclusion is that it needs speaking.  That's why all of the labels were getting spoken.  From the SayAll perspective, we want SayAll by sentence to be consistent with SayAll by line -- namely, don't speak the labels with the exception of Cyan because it's not labeling anything technically.  This patch modifies textLines() to do that:  If we have a label with no relations, then it's not labeling anything, therefore speak it; otherwise skip it.

> Agreed.  I'd guess that the change above (e.g., skip a label in 
> textLines) should probably live in isLabellingContents.  That way, 
> it could be handled by various things that use it.

Having looked at this more closely, I think it makes more sense in textLines() as you originally proposed.  isLabellingContents() answers the question:  "Does this particular item label any of the objects in this list?" Whether or not a label should be skipped because it's redundant -- or should be spoken because some knucklehead screwed up her HTML ;-) -- is a different question I believe. 

I think (hope) it's all good now.  Any thing else that needs addressing/tweaking?

The only problem I still encounter here is that once in a while on slashdot when sayall encounters a heading it will stop.  I'd like to see this committed for the next release is I think it is a big improvement.   

> I think (hope) it's all good now.  Any thing else that needs
> addressing/tweaking?

Looks good to me.  It's a great improvement and we can fix things as we come across them later.  Thanks!

Thanks guys! <wipes brow> ;-)

Patch committed.

Closing as FIXED.