This is currently in the orca spec.  The problem as I understand it is that Orca can not currently set dots independant of the text characters so we arn't able to use dots 7 and 8 to do the underlining.  

Mike and I talked about this yesterday.  We can go back to plan B, which is to have a setting in settings.py that defines which dots to use for underlining, allowing the user to override them.  We can do this in two steps:

1) Get the functionality correct - GNOME 2.19.4 (Jun 18)?

2) Allow the user to customize the setting via the GUI.
    GUI changes need to be done by GNOME 2.19.5 (Jul 9) 
    to avoid the UI announcement period.

I'm assigning this to Joanie since she's done some work in this space already.  Thanks Joanie!  :-)

Created attachment 90905 [details] [review]
first pass at this

This is the first pass at addressing this along with bug 400720 (indicate text attributes).

Rich and I tag-teamed this:  Rich created all of the necessary settings and made the necessary changes to the Orca Preferences dialog.  (Thanks Rich!!)  I did go in and add mnemonics to the radio buttons, so if those are broken, it's my fault. :-) I added the attribute mask related stuff to braille.py.  At the risk of jinxing things, it seems to work. :-)

The braille page now allows you to choose your selection indicator.  The text attributes page does the same.  If you happen to be selecting text that has an attribute being indicated, the indicators are combined.  Thus if dot 7 is your attribute indicator and dot 8 your selection indicator, selecting bold text will cause that text to be indicated by both dots 7 and 8.  Note that we are not doing this in the negative.  Thus if dot 7 is your attribute indicator and dots 7-8 your selection indicator, selecting bold text will cause it to be indicated with dots 7-8.

As Mike specified, attributes that are spoken have been separated out from those which are brailled.  As a result, arrowing around in the tree on the text attributes page can result in speech like: "attribute name checkbox checked checkbox checked".  Rich has looked into this and sent me some links.  I'll look at those momentarily.  In the meantime, he and I both think it's time for Mike to give this a try. :-)

So far from my testing this feature is looking quite nice.  I have just a couple questions/comments that hopefully one or more of you will have thoughts on.
1.  I think the brailling of selection should be on by default.  Should we have dots 7 and 8 or just one or the other.  I'm leaning toward 7 and 8.
2.   I'm wondering if it will be confusing for users to have the radio buttons for weather or not to braille attributes not on the braille tab.  I can't make up my mind on this so thoughts would help.  I'm just wondering if we should keep the check boxes on the attributes page but move the radio buttons for setting the dots to the braille page.  

(In reply to comment #4)
> 1.  I think the brailling of selection should be on by default.  Should we have
> dots 7 and 8 or just one or the other.  I'm leaning toward 7 and 8.

I'm cool with this -- both the on by default and the use of 7-8.  I'll do that in the next version of the patch (some time tomorrow).

> 2.   I'm wondering if it will be confusing for users to have the radio buttons
> for weather or not to braille attributes not on the braille tab.  

Personally I don't think it will be confusing.  If I were checking off what attributes I did and did not want displayed in braille, the first thing that would pop into my mind is: Where on earth do I change the indicator?  Therefore having the attribute radio buttons on that same page, to me, makes sense.  By the same token, if I didn't find them there, the very next place I'd look would be the braille page. :-)  So I don't feel strongly about keeping the radio buttons on the text attributes page if you feel strongly about moving them to the braille page.

> 
> Personally I don't think it will be confusing.  If I were checking off what
> attributes I did and did not want displayed in braille, the first thing that
> would pop into my mind is: Where on earth do I change the indicator?  Therefore
> having the attribute radio buttons on that same page, to me, makes sense.  By
> the same token, if I didn't find them there, the very next place I'd look would
> be the braille page. :-)  So I don't feel strongly about keeping the radio
> buttons on the text attributes page if you feel strongly about moving them to
> the braille page.
As long as neither of us feel really strongly about this lets just leave well enough alone and leave it the way it is now.  
So far I'm really happy with this functionality.
> 

Created attachment 90944 [details] [review]
minor adjustments, plus brlmon.py support attempt #1

This version does the following:

1. Make the default selection indicator 7-8 rather than none as per Mike
2. Attempt to reflect indicators on the braille monitor (as suggested by Rich)
3. Makes a correction to the StarOffice/OOo "adjustment" hack

Rich w.r.t. braille monitor, the pango markup officially has four types of underlining:  None, single, low, double.  I did not see any difference between single and low.  So here's what I did for an initial stab:  dot 7 is single, dot 8 is single, dots 78 are double.  I also pulled the markup strings out of writeText and created a number of constants at the top of brlmon.py so you can more easily adjust the markup.  Please let me know what you think. Thanks!

The brlmon stuff looks pretty nice. I've noticed that it doesn't
always keep up to date when you adjust the selection using the mouse.
Something the following fails to clear the braille monitor selection.

1/ Start up Orca. (I used dots 7-8 for braille selection).
2/ Start up gedit
3/ Type in "The quick brown fox"
4/ Use the mouse to select "brown". The braille monitor shows those
   characters with a double underline.
5/ Just click with the mouse somewhere else on the line of text.
   Sometimes the braille monitor selection isn't cleared.

When navigating my keyboard though, every thing is looking really
good.

> 5/ Just click with the mouse somewhere else on the line of text.
>    Sometimes the braille monitor selection isn't cleared.

It clears for me UNLESS where I click doesn't cause a caret-moved event to be issued.  In other words, if I use the mouse to select "brown" from left to right (i.e. caret is on the space after the "n") and click anywhere OTHER than just past the "n", the braille monitor selection is cleared.  If instead I click just past the "n", I don't get a caret-moved event; just a text-selection-changed event.  (Is this what you are seeing?)  Currently that event listener is mapped to noOp so we don't refresh.  I see in your work for bug 409731 that this will change.  :-) Perhaps your new onTextSelectionChanged() could do some checking for the caretOffset when the mouse button is released and, if appropriate, call _presentTextAtNewCaretPosition()??

> It clears for me UNLESS where I click doesn't cause a caret-moved event to be
> issued. 

You're right. That seems to be exactly what's happening.

> Perhaps your new onTextSelectionChanged() could do some checking for the 
> caretOffset when the mouse button is released and, if appropriate, call
> _presentTextAtNewCaretPosition()??

I originally tried using _presentTextAtNewCaretPosition() in
onMouseButton() to try to get it to speak the current text selection
state, and that didn't work.

Maybe it's a simple case of always just calling the brlmon writeText() routine 
(or whatever the appropriate routine is to update the braille monitor),
in onMouseButton() when a mouse button is released? ...

> I originally tried using _presentTextAtNewCaretPosition() in
> onMouseButton() to try to get it to speak the current text selection
> state, and that didn't work.

I just took your patch from bug 409731 and added the following at the very end of onTextSelectionChanged() (i.e. outside of the for statement):

    _presentTextAtNewCaretPosition()

Now we update the braille display and the braille monitor (both had the same issue; not just braille monitor) in the circumstances in question and we don't seem to be speaking anything different as a result.

> and added the following at the very end of onTextSelectionChanged()

Cool. I'll update the patch in bug #409731 tomorrow accordingly. Thanks!

Awesome.  Thank YOU!  (I do so like it when others make my problems/bugs go away for me :-) )

Assuming Mike doesn't have anything else that needs changing, that leaves what -- if anything -- to do about distinguishing dot 7 from dot 8 in braille monitor.  I did a bit of looking around to see why low and single were different.  I found this: http://gtk.php.net/manual/ja/pango.enum.underline.php

> Pango::UNDERLINE_LOW
> A single underline should be drawn at a position beneath the 
> ink extents of the text being underlined. This should be used
> only for underlining single characters, such as for keyboard 
> accelerators. Pango::UNDERLINE_SINGLE should be used for 
> extended portions of text. 

So, I guess technically I should be using low instead of single.  Fair enough.  I also found this:

> Pango::UNDERLINE_ERROR
> A wavy underline should be drawn below. This underline is 
> typicallyused to indicate an error such as a possilble mispelling; 
> in some  cases an contrasting color may automatically be used. 
> This type of underlining is available since Pango 1.4. 

So what we *could* do if we were so moved is have dot 7 be low, dot 8 be error, and dots 78 be double.  The question is, are we so moved?  If you want to try it, change the ATTRIBUTE_8 line that's near the top of brlmon.py to:

    ATTRIBUTE_8 = " underline='error'"

Then set either selection OR attribute to dot 7 and the other one to dot 8 in your Orca Preferences.  Finally, go into OOo Writer, type:

   The quick brown fox

Make "brown" underlined.  Then select "quick" through the "br" of "brown".

The advantage of going this route is, I think, being able to distinguish which indicators are present without having an actual display.  The disadvantage is that it's not immediately obvious what on earth is going on, plus there's the association between the wavy line and indicating an error.  So.... Rich, as one of the braille-display-less users of braille monitor, what would you like to see us doing here? :-)

> Awesome.  Thank YOU!  (I do so like it when others make my problems/bugs go
> away for me :-) )

I presume you've made sure that adding in this line isn't going to have
any side-effects of extra speech?  Multiple re-displays of the same thing
on the actual braille display aren't so bad (or really noticable), except
in regression test output.

> So.... Rich, as one of the braille-display-less users of braille monitor, 
> what would you like to see us doing here? :-)

What you suggest seems reasonable. I suggest putting together such a patch,
then I (and Will) can have a look at it.

Thanks.

Created attachment 90985 [details] [review]
change to indicators in braille monitor

Here you go Rich (and Will).

Mike is there anything you want changed and/or any problems you have found?

> I presume you've made sure that adding in this line isn't going to have
> any side-effects of extra speech?  

Well, I made sure that it solved the mouse problem. :-) But, you're right, it does have side effects of extra speech.  Oops.... I'll look at it some more. Perhaps just a direct call to updateBraille()....

Hey, that seems to do it.  If instead of calling _presentTextAtNewCaretPosition() at the end of onTextSelectionChanged(), I do this:

        self.updateBraille(event.source)

We don't double-speak when selecting text and we still update the braille display and the braille monitor if you click just past the 'n' of 'brown'.

Cool. I'll go with that in the updated patch (tomorrow) then. Thanks for
double-checking.

Just two minor things I'd like to see fixed.
1.  I'd like to hear the checkboxes on the attributes page spoken correctly.
2.  For the radio buttons on the braille page I think it should say "dots 7 and 8" instead of just "7 and 8".   
Everything else looks good I think.

> 1.  I'd like to hear the checkboxes on the attributes page spoken correctly.

From what I gathered from Rich, this may prove challenging given the current structure of the tree, but I'll take a look.  (Rich any further thoughts/ideas?)  In the meantime, please define "correctly" -- in other words exactly what should be said and under what conditions should it be said.

> 2.  For the radio buttons on the braille page I think it should say "dots 7 and
> 8" instead of just "7 and 8".   

Sounds good. Should 7 say "dot 7" and 8 "dot 8" as well?  And I assume we're also doing this on the text attributes page?

> From what I gathered from Rich, this may prove challenging given the current
> structure of the tree, but I'll take a look.  (Rich any further
> thoughts/ideas?)  In the meantime, please define "correctly" -- in other words
> exactly what should be said and under what conditions should it be said.
What should be said is the label before the checkbox both when arrowing between columns and when arrowing between rows in the tree.  See evolution's message tree for an example.  If this can't be fixed we'll need to rethink this control before the feature can be committed.  
> 
> 
> Sounds good. Should 7 say "dot 7" and 8 "dot 8" as well?  And I assume we're
> also doing this on the text attributes page?
yes to all 
> 

> From what I gathered from Rich, this may prove challenging given the current
> structure of the tree, but I'll take a look.

I originally thought that use some atk calls
to try to set an accessible name or description on those two checkboxes. Unfortunaly neither a cell renderer or a tree view column derives
from a gtk widget, so you can't run get_accessible() 
or it. At this point I'm not sure what to suggest.

> At this point I'm not sure what to suggest.

Well we could have an Orca script that works around this.
Seems ugly, but it may be doable.

Mike:  by label do you mean "braille" or "speak"?   If so.... Rich, what about repeating those items in the column?  That would *look* ugly, but it wouldn't involve special scripting.  Just a thought....

> Rich, what about repeating those items in the column?  

Sorry, I don't understand. You currently have the column headers as 
"Speak" and "Mark in braille". What should they be adjusted to now?

The braille behavior seems correct.   The problem now is that this list serves more than one function.  We should really be speaking:  "speak check box checked" and "mark in braille check box checked".  
How does evolution do this for example when a message both has an attachment and is flagged? 

> How does evolution do this for example when a message both has an attachment
> and is flagged?

Special scripting in Evolution.py. See section 3) in locusOfFocusChanged().

Created attachment 91210 [details] [review]
speak the checkboxes, relabel radio buttons

Mike, this should cause the checkboxes to be spoken.  (A bazillion thanks to Rich for his brilliant idea!  Look, ma, no special scripting needed! :-) )  

Also the radio buttons have been relabeled as you specified.  Please test.  Thanks!

I'm noticing a serious instability using evolution if this patch is applied.  To reproduce, do the following: 
1.  open a bug report message in evolution.  
2.  arrow down to the URL of the bug report.
3.  Right arrow over the URL.  
You should find that orca and the desktop hang.  Usually I can't even switch to a virtual terminal at this point.  The problem seems to be slightly easier to reproduce if braille is enabled but a braille display is not connected. 

Lovely.... And confirmed.  Yikes. :-(  Thanks!!  Debugging now....

Created attachment 91285 [details] [review]
latest version.  Should not endless loop like the last one. :-)

The instability was the result of our getting different end offsets for attributes depending on whether or not we're using OOo Writer --  or, more accurately, my being lame and failing to adjust accordingly for when we're *not* in Writer.  Sheesh....

Please try this version of the patch Mike.  Thanks again very much for catching that!!!

I just found another oddity/instability with Evolution.  <sigh>  Let me investigate that further and then post (another) revised patch.

Created attachment 91286 [details] [review]
another adjustment for evolution

Mike please test this version.  Really, I mean it this time. <smile>  And thanks!

Created attachment 91311 [details] [review]
version 7 -- more robust checking and bailing when needed

Mike please test.  Thanks!

Created attachment 91316 [details] [review]
change Evolution script to call updateBraille()

I noticed that we weren't indicated attributes in Evolution when arrowing up or down into a new paragraph but were as soon as left or right was pressed.  Evolution.py's locusOfFocusChanged() was calling braille.displayMessage() with the string at the line with focus rather than calling self.updateBraille().  As a result, we weren't calling braille.refresh and the attributeMask stuff was not occurring.  This patch changes Evolution.py to call updateBraille() instead and we now see the attributes when arrowing up and down into text within a message.

I am now unable to reproduce any hangs with the latest patch.  I am also no longer having the row/column problem we talked about in IM.  I'd say it's OK to check this in now.  Because of the instability problems that we cought lets ask the user community to really pound on this feature over the next couple days.  I assume Joanie is going to send out a note to the list on this feature when it is checked in.
Joanie, thanks much for your work on this.  It is a really nice and useful feature.    

Thanks Mike.  It's now checked in.  I'll write up something for the list.

Thanks again VERY much to Rich for all of this help with this!!

Created attachment 91446 [details] [review]
added translator docs to orca_gui_prefs.py

I have a question about these lines in the patch:

        renderer.set_property("text", speakAttrColumnLabel)
        renderer.set_property("visible", False)
...
        renderer.set_property("text", markAttrColumnLabel)
        renderer.set_property("visible", False)

My understanding is that the motivation for these were to give the checkboxes a name which is the same as the column header so as to provide more context when doing 'read table cell row'.  The side effect this introduces, however, is that when navigating the table left/right cell by cell, you will hear the column header spoken twice.  For example: "Mark in braille column header.  Mark in braille checkbox checked."

As an alternative to this clever hack, I wonder if more smarts should be added to speechgenerator.py:_getSpeechForTableCellRow to insert the column header if it detects that the table cell is one of these 'checkbox only' kind of things.  This might provide better support across applications that present these kinds of things (e.g., the Tree View demo of gtk-demo, the message summary lists of things like Evolution and Thunderbird, etc.).

Mike, Joanie -- if you agree, we probably should open a new RFE to handle this.

Makes sense to me!

After talking with Will this morning I think a more general solution is a great idea.  This might also allow us to remove the hak that Rich had to add to evolution to implement the needed functionality.

(In reply to comment #41)
> After talking with Will this morning I think a more general solution is a great
> idea.  This might also allow us to remove the hak that Rich had to add to
> evolution to implement the needed functionality.
> 

Logged as bug 455230.  Thanks!

Lets call this one good.

Thanks!  Closing as FIXED.