To get started, you'll need to check BrlAPI and BrlTTY out of CVS. The Python bindings aren't in the latest stable release. See http://mielke.cc/brltty/download.html. You'll also need to install Pyrex ('yum install Pyrex' should work on FC6) and any other reqs listed by the BrlTTY/API docs.

Created attachment 76219 [details]
instructions on how to fix brltty version 0.5.0 build

Created attachment 76220 [details]
pydoc generated document of python binding for brlapi

Next step: implement an initial LSR adapter to BrlAPI.

1) Look at http://cvs.gnome.org/viewcvs/lsr/src/AEOutput/. The Base.py file contains an abstract base class that all LSR device adapters must implement.
2) Create a new file called BrlAPIDevice.py. Put it anywhere for now. It doesn't need to be in the lsr folder.
3) Create a new class with the same name as the file. Derive it from "object" for now.
4) Implement the following methods: init, close, sendString, sendTalk, isActive, sendStop. Look at the docstrings in the AEOutput base class for help on what they should do. They're somewhat speech-centric so ask if they're confusing.
5) Write a test script to test your code outside of LSR.

Post your code here once this much is working.

*** Bug 362619 has been marked as a duplicate of this bug. ***

Created attachment 76268 [details]
preliminary BRLAPIDevice module

Created attachment 76270 [details]
preliminary BRLAPIDevice module

Instructions for installing/using the Braille device: (assumes you have
lsr installed from CVS)

! Remember to switch the base class to AEOutput.AEOutput.

1) Put the BrlAPIDevice.py file where you'd like to work on it hereafter (e.g.
~/work/)
2) To make LSR aware of the device: lsr -i ~/work/BrlAPIDevice.py
3) To make sure the device loads with the user and developer profiles: lsr -a
BrlAPIDevice -p user -p developer

Run "lsr -l debug" to start LSR with logging set to the debug level. Look at the initial set of messages to see if LSR successfully loaded the device or not. If not, post the error here. 

LSR quite possibly will ignore the Braille device because it doesn't provide any services that the speech devices do not. This is correct behavior. LSR runs through a list of devices when it starts and performs the following tests:

1) Can I import the module?
2) Does the class with the same name as module provide new functions not already provided by a device I've already loaded?
3) Can I instantiate the class?
4) Can I initialize the device?

If any answer is "no," LSR skips the device and moves on. This is how we support zero-config for the user: LSR just tries all of its device drivers and uses the first one of each type (e.g. speech, Braille) that works.

To overcome this behavior, we'll have to define a new subclass of AEOutput for Braille so that LSR can tell that this device is different from the others. Pete will work on adding this subclass.

Scott, for now, will derive from the AEOutput base class and see what happens.

Derive the BrlAPIDevice class from AEOutput.Braille after doing a CVS update and an install. This will allow the DeviceManager (which loads all devices) to determine that your code is for a Braille device and should be loaded in addition to any speech device.

Created attachment 76291 [details]
preliminary BrlAPIDevice.py with added style classes

If you update from CVS, you can now remove the addIndexListener.

More notes to follow on next steps.

Created attachment 76333 [details]
build fix and requirements for brltty5.0

Next steps:

1) Generate a new Perk called BasicBraillePerk.
2) In the Perk.init method, get a reference to the BrlAPIDevice by name. (See the Task.Tools doc for the method to call. We're going to change this later so you don't have to specify this particular Braille device by name). Store the ref in an instance variable.
3) Have the Perk listen for caret events and selector events using the appropriate Task subclasses. (See the existing DefaultPerk.py if you need help)
4) Use sayItemText (look at Task.Tools.Output documentation) to send the current chunk of text to the Braille device whenever the active selection changes or the text caret enters a new line of text. Look at the DefaultPerk.py for help on how we decide if the caret moved across lines. You'll implement something similar in your Perk for now, and we may refactor in the future if it's exactly the same code.
5) Test, debug, repeat!

When this Perk is working, the Braille display should start showing text like the BrlTTY AT-SPI driver.

General things to do:

1) Make the number of cells available on the Braille device a property of a default Braille style and populating that value in the BrlAPIDevice default style instance at startup.
2) Having options for rendering the caret position on the display in the BasicBraillePerk.
3) Defining input tasks the user can invoke to step forward and backward through a line of text too long for the display.
4) Implementing the AEInput interface.

Created attachment 76338 [details]
Simple short term roadmap and ideas

Questions from Scott with my answers:

1.  Using brlapi requires an auth key which has permissions 600 and is
owned by root.  This requires any clients of brlapi to also be root.  What
is the best solution for this problem?  Run lsr as root?  Change
permissions of auth key?  sudo?  Something else?

I'm not familiar with how users run BrlTTY. Best to ask the developers what the typical use case is. I can say that running lsr as root is definitely not the right solution. Changing the permissions on the key is acceptable for development if it works.

2.  I get a handle to the BrlAPIDevice in the init function of the
BasicBraillePerk.  However, I will need access to the device within the
registered tasks.  Is it best to pass a reference to the device using the
**kwagrs when I register the task or would it be best to get a handle to
the device in the tasks themselves?

For now, just access it as self.perk.the_reference in a Task. Once I have the big patch committed I'm working on, you won't need access to the reference at all. The Perk init method will call self.setPerkIdealOutput('braille') and then all say methods on Tasks in the same module will default to using a Braille device, if available.

3. Should the BasicBraillePerk be named DefaultBraillePerk to better match
DefaultPerk?  Should DefaultPerk be renamed DefaultSpeechPerk?

DefaultPerk is going to change names to BasicSpeechPerk in the near future.

4.  The concept of registering tasks so they can be reused is great.  As a
developer, how can you view the registered tasks (besides digging into the
source) ?  As the number of tasks grow it would be nice to be able to view
information about a particular task and have it be searchable.

You can see the execution of Tasks in response to events in the Task Monitor in the developer profile. We also have a Help dialog planned which will list for the user which Tasks have been bound to input gestures (e.g. hotkeys, Braille device buttons).

I think what you're asking for is a key we can press to dump to stdout or a dialog we can show that lists the *names* of all registered Tasks in the current Tier. Is this right? 

5.  You wrote about a method in the TaskTools.Output module called
sayItemText.  I assume you meant sayItem.  Is this correct?

Correct. My mistake.

Created attachment 76515 [details]
First version of base braille perk implementing selector and caret change

simple proof of concept base braille perk

Next steps:

1) Define a CaretStyle (or appropriately named) class variable on your default style object in BrlAPIDevice.
2) Define an arbitrary, but sequential, set of global variables representing the different caret rendering options. (For now, put them in your device class, but we'll probably move them to AEOutput/Style.py later)
3) Fill-in getSettings mimicking what's done in AEOutput/Style.py in the _newWordGroup method for WordDef enumeration.
4) Test your code to see if the settings shows up in the LSR settings dialog and persists its current value between runs of LSR.

If you want to define another style attribute, add CellColumns to the style object. This one should not be part of getSettings because it's not configurable. In the init method, read the number of cells actually on the device using brlapi and store it in self.default_style.CellCount. 

I think BrlAPI lets you determine the number of rows also. If so, add and populate an equivalent CellRows attribute.

Later:

1) Figure out how to get caret information to the device. (Will discuss with Larry.)
2) Figure out what code is primarily responsible for scrolling the current line (device or Perk). I believe most of the work is going to fall on the Perk since it must bind tasks to buttons on the device or keys on the keyboard and resend the entire line for display. However, it might be possible for the device to do this work too by caching all text sent and the last caret position, and maintaing a sliding window the size of the Braille display in cells.

Scott, email the BrlAPI/TTY folks and ask what they do when more text is sent to a device than there are cells to render it. Is it simply truncated and discarded?

3) Status both on devices that have dedicated cells and ones that don't. (Will discuss with Larry).

Other topics discussed:

1) Use getAccLabel when getItemText returns an empty string or None. (Don't worry about the case where they both exist yet.)

Created attachment 76661 [details]
updated BrlAPIDevice

Created attachment 76662 [details]
updated BasicBraillePerk

Permanent action items now located at http://live.gnome.org/LSR/ScatchPad/Braille.

Misspelling corrected: http://live.gnome.org/LSR/ScratchPad/Braille

Created attachment 76994 [details]
updated BrlAPIDevice.  Includes ellipsis, caret pos, sliding window, input

Created attachment 76995 [details]
updated BasicBraillePerk.  sliding window, ellipsis, input, caret pos

Created attachment 77211 [details]
refactored, fixed scrolling bug

Comments about latest code:

1) Caret default should be to display with dots 7/8
2) Caret doesn't render as dots 7/8 when a non-text control (e.g. a button) has focus. It shows up as a single dot for some reason.
3) try/except errors when brltty shutdown with LSR running. Keep the input thread running at all costs. Catch and log (or ignore for now) all other errors on output.
4) Need a way to handle the case where brltty is not running and the BasicBraillePerk tries to load. Don't load it? Seems wrong since the user might plug-in their device later and want to start using it immediately with applications. This means the Perk must be loaded on those Tiers when the device is later available for use. However, this is a big waste of memory if the user never plans to use a Braille device. Let's think about this one before implementing a solution.

Have some more. Will post later.

1) Caret default should be to display with dots 7/8

Agreed, this is the most accepted symbol for the caret and the caret should be displayed by default.

2) Caret doesn't render as dots 7/8 when a non-text control (e.g. a button) has
focus. It shows up as a single dot for some reason.

A dot in the 7th position indicates capitalization.

3) try/except errors when brltty shutdown with LSR running. Keep the input
thread running at all costs. Catch and log (or ignore for now) all other errors
on output.

- try/except in place.
- input thread running
- lsr fails to exit cleanly on demand
- should brlapidevice try to reconnect?
- why should thread not be killed?


4) Need a way to handle the case where brltty is not running and the
BasicBraillePerk tries to load. Don't load it? Seems wrong since the user might
plug-in their device later and want to start using it immediately with
applications. This means the Perk must be loaded on those Tiers when the device
is later available for use. However, this is a big waste of memory if the user
never plans to use a Braille device. Let's think about this one before
implementing a solution.

> A dot in the 7th position indicates capitalization.

OK. So how do you indicate a capital letter under the cursor?

> 1) try/except in place.
> 2) input thread running
> 3) lsr fails to exit cleanly on demand
> 4) should brlapidevice try to reconnect?
> 5) why should thread not be killed?

3) When the close method is called on BrlAPIDevice, it has to stop the thread. Otherwise, Python won't exit.
4&5) I was suggesting that transient exceptions should be caught and ignored. For instance, what if one particular output command fails but the next one will work fine for some reason? When the failure is fatal, then we'd probably want to reconnect. But no other device does this yet so don't worry about it for Braille. I'm not quite sure how well reconnection would work. What if the brltty process was shutdown permanently? We wouldn't want to keep trying to reconnect on every command if it's never coming back.
 
> 
> 4) Need a way to handle the case where brltty is not running and the
> BasicBraillePerk tries to load. Don't load it? Seems wrong since the user might
> plug-in their device later and want to start using it immediately with
> applications. This means the Perk must be loaded on those Tiers when the device
> is later available for use. However, this is a big waste of memory if the user
> never plans to use a Braille device. Let's think about this one before
> implementing a solution.
> 

Created attachment 77340 [details]
alpha version of new user friendly version

Created attachment 77341 [details]
updated BrlAPIDevice

Created attachment 77421 [details]
latest version.

Created attachment 77422 [details]
update BrlAPIDevice: change to brlapi.write; user select ellipsis

Created attachment 77504 [details]
BasicBrlPerk: paging with overlap and snap to word

Created attachment 77506 [details]
BrlAPIDevice: separated left and right ellipsis definitions. epydoc updates

Comment on attachment 77506 [details]
BrlAPIDevice: separated left and right ellipsis definitions. epydoc updates

>MAX_KEY_CHORD = 3

Remember to ask the brltty devels about getting button press as well as release events so we can support chords on the device.

>class BrlAPIStyleDefault(AEOutput.StyleDefault):
>  '''
>  Overrides the base L{AEOutput.Style.StyleDefault} class, filling in 
>  fields for supported style properties with their appropriate values.
>  
>  @cvar CaretNone: Caret style enum indicating no caret
>  @type CaretNone: integer
>  @cvar CaretTwoBottom: Caret style enum indicating pins 7 and 8
...

Good doc. Thank you.

This entire class should probably be moved to AEOutput/Style.py now and become the BrailleStyleDefault class declared like so:

class BrailleStyleDefault(StyleDefault):

The BrlAPIStyleDefault would then be declared as:

class class BrlAPIStyleDefault(AEOutput.BrailleStyleDefault):

There are some hang-ups. Continue reading.

>  # Caret display styles
>  CaretNone = 1
>  CaretTwoBottom = 2
>  CaretBottomRight = 3
>  CaretAll = 4
>  CaretStatusCells = 5  # not shown in settings dialog now

These should be moved to AEConstants/Output.py. Change their format to the naming convention followed in that file (e.g. CARET_NONE = 1). Also, add a CARET_LAST and set it to CARET_STATUS_CELLS for now.

>  MaxCaretDisplay = 5
>  MinCaretDisplay = 1
>  CaretStyle = CaretTwoBottom

These will now use the CARET_* constants from AEConstants.

>  EllipsisContinuation = 0
>  EllipsisEllipsis = 1
>  MinEllipsisDisplay = 0
>  MaxEllipsisDisplay = 1

Same with these. They should become AEConstants now.

>  EllipsisStyle = EllipsisEllipsis
>  EllipsisStyles = [ (chr(brlapi.BRL_DOT4 + \
>                       brlapi.BRL_DOT5 + brlapi.BRL_DOT6)+ \
>                    chr(brlapi.BRL_DOT1 + brlapi.BRL_DOT2 + brlapi.BRL_DOT3+\
>                       brlapi.BRL_DOT4 + brlapi.BRL_DOT6)+chr(0),chr(0)+chr(brlapi.BRL_DOT4+\
>                       brlapi.BRL_DOT5 + brlapi.BRL_DOT6)+ \
>                    chr(brlapi.BRL_DOT1 + brlapi.BRL_DOT2 + brlapi.BRL_DOT3+\
>                       brlapi.BRL_DOT4 + brlapi.BRL_DOT6)), \
>                       (''.center(3,chr(brlapi.BRL_DOT3))+chr(0),\
>                       chr(0)+''.center(3,chr(brlapi.BRL_DOT3)))]

The EllipsisStyles attribute cannot be easily moved to the default class because it relies on constants from brlapi as implemented. The EllipsisStyles should not be part of the style class, but coded somewhere as a global variable or method in your BrlAPIDevice class. It's the representation of EllipsisContinuation and EllipsisEllipsis specific to brlapi. Other Braille APIs will have to provide their own.

>  # braille display properties
>  DisplayColumns = 0
>  DisplayRows = 0 
>  TotalCells = 0

These should definitely move to the BrailleDefaultStyle class.

You can make patches against AEOutput/Style.py and AEConstants and post them to this report and I'll apply them.

>  def addIndexListener(self, bogus):
>    pass

Axe this method. It's no longer part of the AEOutput.Base interface.

Created attachment 77514 [details] [review]
braille style patch

Created attachment 77647 [details]
missing cell addition to BrlAPIDevice

Created attachment 78206 [details] [review]
updated BasicBraillePerk.  touch cursor

Created attachment 78207 [details] [review]
updated BrlAPIDevice: touch cursor

Created attachment 78208 [details] [review]
core changes to support touch cursor

Known issues:
1)  snap to word ineffective when ellipsis is selected
2)  missing cells shift padding

Comment on attachment 77514 [details] [review]
braille style patch

Uses old-style settings (i.e. class variables). Needs updating to use settings objects in some cases.

Comment on attachment 78206 [details] [review]
updated BasicBraillePerk.  touch cursor

># to support translation of strings in this Perk, uncomment the following line
># and provide the proper domain and path to your translation file
># _ = bind(domain, locale_dir)

Wipe the above.

>class BasicBraillePerkState(Perk.PerkState):
>  '''
>  Settings for BasicBraillePerk
>  @ivar Overlap: Number of original chars remaining on page forward/backward
>  @type Overlap: L{Setting}
>  @ivar LeftPadding: remaining cells on left side during scrolling
>  @type LeftPadding: L{Setting}
>  @ivar RightPadding: remaining cells on right side during scrolling
>  @type RightPadding: L{Setting}
>  @ivar SnapOnUpdate: snap braille display to previous location upon update
>  @type SnapOnUpdate: L{Setting}
>  @ivar SnapToWord: snap braille display to nearest word upon paging
>  @type SnapToWord: L{Setting}
>  '''

These are no longer ivars and epydoc will complain if it can't find them. Use the format I used in AEOutput/Style.py to document settings for now.

>    # register braille named tasks for input events
>    self.registerTask(HandleScrollForward('scroll forward'))
>    self.registerTask(HandleScrollBackward('scroll backward'))
>    self.registerTask(HandlePageForward('page forward'))
>    self.registerTask(HandlePageBackward('page backward'))
>    self.registerTask(HandleTouchCursor('touch cursor'))

I recommend naming these tasks 'braille scroll forward', 'braille scroll backward', etc.

>    return _('Basic Braille Perk manages basic braille output')

Can shorten to "Manages basic braille output" if you want. The name of the Perk is shown next to the description in the settings dialog.

>class HandleTouchCursor(Task.InputTask):
>  '''
>  Handle touch cursor sets caret at given location in text accessibles or
>  left clicks the center of the given accessible
>  '''
>  def execute(self, **kwargs):
>    cellnumber = kwargs['brlarg'][0]

Two comments here:

1) Don't unpack manually. Change the method signature to be "def execute(self, brlarg, **kwargs):".
2) How exactly are the arguments getting stored in the AEEvent? Try to avoid using positional parameters. For instance, I have no idea what brlarg[0] is here. If you have a tuple for some reason in brlarg, I recommend splitting it up into more explicit keyword args like button_num, modifier, etc.

>    self.setTouchCursor(EVENT_SYNTHMOUSE_B1C, self.task_por)

I'm not sure this is the right method name. The method is used by the touch cursors, but has nothing fundamentally to do with them. Something along the lines of clickPOR might be better.

>      for i in xrange(self.perk.scroll_offset+self.perk.pre_left_slice, \
>                      -1, -1):
>        if self.perk.current_text[i] == ' ' or i == 0:
>          if i != 0:
>            wordadj += 1
>          found = True
>          break
>        else:
>          wordadj -= 1 
>      if found:
>        self.perk.scroll_offset += wordadj

This beasty needs some comments. :)

>      for i in xrange(self.perk.scroll_offset+self.perk.pre_left_slice, \
>                      -1, -1):
>        if self.perk.current_text[i] == ' ' or i == 0:
>          if i != 0:
>            wordadj += 1
>          found = True
>          break
>        else:
>          wordadj -= 1 
>      if found:
>        self.perk.scroll_offset += wordadj

Same here. Please comment.

>  def executeText(self, por, text, **kwargs):
>    '''
>    Announces the localized word selected or unselected followed by the total 
>    number of characters in the selection.
>    
>    @param por: Point of regard where the selection event occurred
>    @type por: L{POR}
>    @param text: The currently selected text
>    @type text: string
>    '''
>    pass  

Remove if not using.
    
>class HandleCaretChange(Task.CaretTask):
>  '''  
>  Task that handles a L{AEEvent.CaretChange}. The L{execute} method performs
>  actions that should be done regardless of whether text was inserted or
>  deleted or the caret moved. The other three methods handle these more
>  specific cases.
>  
>  @ivar changed: Tracks whether the last caret move event was a caused by a
>    change (insert/delete) or just navigation
>  @type changed: boolean
>  '''
>  def init(self):
>    pass

Remove if not using.

>    state = self.perk.getState()

You can access the state object with self.perk_state too if you want. OK if you want to leave it as a method call.

Comment on attachment 78207 [details] [review]
updated BrlAPIDevice: touch cursor

>MAX_KEY_CHORD = 1

Put a comment as to why it's 1 only.

>class BrlAPIStyle(AEOutput.Style):
>  '''
>  Overrides the base L{AEOutput.Style} class, filling in 
>  fields for supported style properties with their appropriate values.
>  
>  @ivar CaretStyle: Current caret style  
>  @type CaretStyle: L{Setting}    
>  @ivar EllipsisRight: User wants right ellipsis shown  
>  @type EllipsisRight: L{Setting}    
>  @ivar EllipsisLeft: User wants left ellipsis shown  
>  @type EllipsisLeft: L{Setting}    
>  @ivar CellMask: Missing cell string  
>  @type CellMask: L{Setting}        
>  '''

No longer ivars. See the note I put in the BasicBraillePerk class about what to do.

>  def init(self, dev):
>    self.newEnum('CaretStyle', CARET_TWO_BOTTOM, _('Caret Display Style'), 
>                {_('No Caret') : CARET_NONE,
>                _('Two Bottom Pins') : CARET_TWO_BOTTOM,
>                _('Bottom Right Pins') : CARET_BOTTOM_RIGHT,
>                _('All Pins') : CARET_ALL},
>                _('Defines the caret representation.'))
>                
>    self.newBool('EllipsisRight', False, _('Ellipsis right?'), 
>                _('When set, ellipsis will appear when additional text is to the\
>  right of the viewport.'))
>    self.newBool('EllipsisLeft', False, _('Ellipsis left?'), 
>              _('When set, ellipsis will appear when additional text is to the\
>  left of the viewport.'))
>    self.newEnum('EllipsisStyle', ELLIPSIS_ELLIPSIS, _('Ellipsis Display Style'), 
>                {_('Pin 3 in 3 cells') : ELLIPSIS_ELLIPSIS,
>                _('Continuation symbol') : ELLIPSIS_CONTINUATION},
>                _('Defines the caret representation.'))
>                
>    self.newString('CellMask', '', _('Broken Cell String'), 
>                  _('String of zeros and ones indicating the functionality of each\
>  cell'))

Yesterday, we never finished talking about the settings and which objects they should be on. I think all of these should be created for the default only at present. We only wanted to create them on all objects for testing purposes. Now we want all of the new* calls to be in a block starting with "if self.isDefault():"


>  def getGroups(self):
>    '''
>    Gets configurable absolute settings affecting all output from this device.
>    
>    @return: Root group of all configurable settings
>    @rtype: L{AEState.Setting.Group}
>    '''
>    root = self.newGroup()
>      
>    c = root.newGroup(_('Caret'))
>    c.append('CaretStyle')
>      
>    d = root.newGroup(_('Ellipsis'))
>    d.append('EllipsisRight')
>    d.append('EllipsisLeft')
>    d.append('EllipsisStyle')
>      
>    e = root.newGroup(_('Broken Cells'))
>    e.append('CellMask')
>   
>    return root

Same here. Everything should be protected with an "if self.isDefault():" though things will work fine at present if they're not.
  
>      try:
>        key = brlconn.readKey(block=False)
>      except brlapi.OperationError:
>        #TODO: log error. careful about loop!

Good comment.


>    self.log = logging.getLogger('Device')

I'd make this a global like in our other modules and give it a better channel name, like "BrlAPIDevice" not just "Device". Then you can reuse it across all classes in this file (e.g. your thread above).

>    
>    try:
>      self.brlconn = brlapi.Connection()  
>      self.brlconn.enterTtyMode()
>      self.writestruct = brlapi.Write()
>    except brlapi.ConnectionError:
>      raise AEOutput.InitError

In the future, this is where we would want to ignore all commands to start. Later, when a Perk registers an InputTask, we'll somehow get the Task.Tools to start paying attention to the appropriate keys. The trick is, what do we do when the device is unregistered and re-registered later? All loaded Perks will be listening for keys already by the device starts off ignoring everything. I think we can solve this if the DeviceManager hangs onto copies of registered keys when loading/unloading devices.

No need to act on this today. But maybe you want to put a reminder as a comment at this location.


>  def getbrlconnection(self):
>    '''
>    Returns the connection object to brltty 
>    
>    @return: A brlapi connection object
>    @rtype: brlapi.Connection
>    '''
>    return self.brlconn

Try to match the camel case naming convention: getBrlConnection.

>    # notify listeners of the gesture with given args at the timestamp
>    self._notifyInputListeners(gesture, time.time(), brlarg=(argument,flags))

This is what I was suggesting you change in the comments I left for your Perk:

self._notifyInputListeners(gesture, time.time(), argument=argument,flags=flags)

Much clearner and more explicit. If possible, try to find better names for argument and flags too. They're pretty generic. Think about what data they store and whether that information might be available from another Braille API in the future.

>  def _notifyInputListeners(self, gesture, timestamp, **kwargs):
>    '''
>    Notifies registered listeners about a L{Gesture} seen on the input device. 
>    Catches all exceptions from the callback and logs them.
>    
>    @param gesture: L{Gesture} to send to listeners
>    @type gesture: L{Gesture}
>    @param timestamp: Time at which at the gesture happened
>    @type timestamp: float
>    '''      
>    for listener in self.listeners:
>      try:
>        listener(gesture, timestamp, **kwargs)
>      except Exception:
>        log.exception('AEInput exception')

You don't want to override the default implementation to support kwargs. We want to extend the default implementation in the base class. Make that a separate patch.

>  def sortGesture(self, gesture):
>    '''    
>    Sort the provided L{Gesture} according to a sort order
>    specific to the type of input device.
>    
>    @param gesture: L{Gesture} object to sort
>    @type gesture: L{Gesture}
>    @return: List of action codes sorted according to the specific device 
>        implementation
>    @rtype: list of integer
>    '''
>    codes = gesture.getActionCodes()
>    codes.sort()
>    return codes

Is this not implemented in AEInput/Base.py? Is it from AEInput/SystemInput.py? Seems like it should be moved to the base to become the default implementation.
  
>  def getMaxActions(self):
>    '''
>    Gets the maximum number of actions that can be in a L{Gesture} on this 
>    input device.
>    
>    @return: Maximum number of actions per L{Gesture} supported by this device
>    @rtype: integer
>    '''
>    return MAX_KEY_CHORD

Same here. Maybe MAX_KEY_CHORD should become a class variable like USE_THREAD. Then the base class can implement this simple logic.

>  # -------------    begin output related methods  ----------------------------
>  def _applyStyle(self, style):
>    pass

Remove if you're not using it. It's not part of the output interface anyways.
  
>  def createDistinctStyles(self, num_groups, num_layers):
>    '''
>    Creates distinct styles following the guidelines set forth in the base
>    class. 
>    
>    @param num_groups: Number of sematic groups the requestor would like to
>      represent using distinct styles
>    @type num_groups: integer
>    @param num_layers: Number of content origins (e.g. output originating from
>      a background task versus the focus) the requestor would like to represent
>      using distinct styles
>    @type num_layers: integer   
>    @return: Styles
>    @rtype: list of L{AEOutput.Style}
>    ''' 
>    styles = []
>    for i in xrange(num_groups*num_layers):
>      s = BrlAPIStyle(self.default_style)
>      styles.append(s)
>    return styles

I don't think you want to implement this method. You're not using it for anything and styles are created on demand if they're not all created up front by this method. Right now, it's just wasting memory by creating groups*layers styles (around 40 objects) whereas you're only ever using one: the style for the semantic tag 'item' in the 'focus' layer.

>      self.log.warn(_('braille output failed'))

Don't mark log messages for translation. They're typically just for developers at present. The Perk should be responsible, ultimately, of tell the user about I/O failures according to their preferences and what other devices it can find to us.

Comment on attachment 78208 [details] [review]
core changes to support touch cursor

>>   def doMouseClickAtPor(mouseevent):
>>     '''
>>     Performs a mouse event on the center of a given point of regard.  If the
>>     accessible has a role of text, then set cursor at given location in por
>>     
>>     @param mouseevent: mouse event to perform
>>     @type mouseevent: integer of type EVENT_SYNTHMOUSE*
>>     @return: Indicator of whether the acc was clicked or not
>>     @raise LookupError: When the accessible object is dead
>>     '''
>>     pass
>>   def doMouseClickAtCenter(mouseevent):
>>     '''
>>     Performs a mouse event on the center of the given point of regard
>>     
>>     @param mouseevent: mouse event to perform
>>     @type mouseevent: integer of type EVENT_SYNTHMOUSE*
>>     @raise LookupError: When the accessible object is dead
>>     '''
>>     pass
>>     

I'm not convinced these two methods should both be part of the public IAccessibleAction interface. I think we just want doMouseClickAtPOR, and then let the code decide what that means exactly. The place where this will break is on text with a hyperlink in it, but we can do that programmatically too at a later time.

>>   
>Index: src/Tier.py
>===================================================================
>RCS file: /cvs/gnome/lsr/src/Tier.py,v
>retrieving revision 1.37
>diff -r1.37 Tier.py
>483a484,485
>>       gesturedata = event.getDataForTask()
>>       gesturedata['count'] = count
>488c490
><       self._executeTask(task, None, layer, {'count' : count}, True, timestamp)
>---
>>       self._executeTask(task, None, layer, gesturedata, True, timestamp)
>Index: src/AEConstants/Event.py
>===================================================================
>RCS file: /cvs/gnome/lsr/src/AEConstants/Event.py,v
>retrieving revision 1.1
>diff -r1.1 Event.py
>69a70,86
>> # synthesized mouse events
>> # Maps directly to AT-SPI output strings
>> EVENT_SYNTHMOUSE_B1P = 'b1p'
>> EVENT_SYNTHMOUSE_B1R = 'b1r'
>> EVENT_SYNTHMOUSE_B1C = 'b1c'
>> EVENT_SYNTHMOUSE_B1D = 'b1d'
>> EVENT_SYNTHMOUSE_B2P = 'b2p'
>> EVENT_SYNTHMOUSE_B2R = 'b2r'
>> EVENT_SYNTHMOUSE_B2C = 'b2c'
>> EVENT_SYNTHMOUSE_B2D = 'b2d'
>> EVENT_SYNTHMOUSE_B3P = 'b3p'
>> EVENT_SYNTHMOUSE_B3R = 'b3r'
>> EVENT_SYNTHMOUSE_B3C = 'b3c'
>> EVENT_SYNTHMOUSE_B3D = 'b3d'
>> EVENT_SYNTHMOUSE_ABS = 'abs'
>> EVENT_SYNTHMOUSE_REL = 'rel'
>> 
>Index: src/AEConstants/Output.py
>===================================================================
>RCS file: /cvs/gnome/lsr/src/AEConstants/Output.py,v
>retrieving revision 1.3
>diff -r1.3 Output.py
>5d4
>< @todo: PP: make common styles into constants
>17,19d15
>< # no-op command for all devices
>< CMD_NOOP = 0
>< 
>21,24c17,21
>< CMD_STOP = 1
>< CMD_TALK = 2
>< CMD_STRING = 3
>< CMD_FILENAME = 5
>---
>> CMD_STOP = 0
>> CMD_TALK = 1
>> CMD_STRING = 2
>> CMD_STRING_SYNC = 3
>> CMD_FILENAME = 4
>27,28c24,25
>< CMD_CARET = 6
>< CMD_TRUNCATE = 8

These don't match up with CVS. Make sure you update before making the patch.
   
>>   def doMouseClickAtPor(self, mouseevent):
>>     '''
>>     Performs a mouse event on the center of a given point of regard.  If the
>>     accessible has a role of text, then set cursor at given location in por
>>     
>>     @param mouseevent: mouse event to perform
>>     @type mouseevent: integer of type EVENT_SYNTHMOUSE*
>>     @return: Indicator of whether the acc was clicked or not
>>     @raise LookupError: When the accessible object is dead
>>     '''
>>     acc = self.accessible
>>     try:
>>       itext = Interfaces.IText(acc)
>>       if self.char_offset is not None and self.item_offset is not None:
>>         # setCaretOffset returns boolean
>>         if not itext.setCaretOffset(self.char_offset+self.item_offset):
>>           doMouseClickAtCenter(mouseevent)
>>       else:
>>         doMouseClickAtCenter(mouseevent)
>>     except LookupError:
>>       doMouseClickAtCenter(mouseevent)
>>      
>>     return True
>>       
>>   @pyLinAcc.errorToLookupError
>>   def doMouseClickAtCenter(self, mouseevent):
>>     '''
>>     Performs a mouse event on the center of the given accessible
>>     
>>     @param mouseevent: mouse event to perform
>>     @type mouseevent: integer of type EVENT_SYNTHMOUSE*
>>     @raise LookupError: When the accessible object is dead
>>     '''
>>     vpt_x, vpt_y = IAccessibleInfo(self.subject).getAccVisualPoint()
>>     devcont = pyLinAcc.Registry.getDeviceEventController()
>>     devcont.generateMouseEvent(vpt_x, vpt_y, mouseevent)
>>   
>>   @pyLinAcc.errorToLookupError

Again, there should be only one public method here: doMouseClickAtPOR. If you want to keep doMouseClickAtCenter to keep the code cleaner, that's fine. But prefix it with _ and remove it from LSRInterfaces.py.

>>   def setTouchCursor(self, mouseevent, por=None):

See comments about name of this method in the Perk code. Should probably be clickPOR or something.

>>     ''' 
>>     Deselects the text from the given starting L{POR} to the ending L{POR}. If
>>     n is None, all selected ranges are removed, else the nth selection is
>>     removed. When L{por} is None, the L{task_por} is used.
>> 
>>     @param mouseevent: Mouse event OF type EVENT_SYNTHMOUSE_*
>>     @type mouseevent: String
>>     @param por: Point of regard to the text area; use the current task's 
>>       L{POR} if none supplied
>>     @type por: L{POR}
>>     @return: Did touch cursor succeed?
>>     @rtype: boolean
>>     @raise PORError: When the L{POR} is invalid
>>     @raise SelectError: When the deselection operation fails
>>     '''

Wrong first paragraph in docstring?

>>         raise SelectError

Probably not a selection error which means select this item, but do not click it. We might need a new one called ClickError. Add it to Task.Tools.Errors.

Created attachment 78231 [details] [review]
revised version after latest comments (comments 43-45)

Created attachment 78232 [details] [review]
revised version after latest comments (comments 43-45)

Created attachment 78233 [details] [review]
revised version after latest comments.  Touch cursor support

Comment on attachment 78232 [details] [review]
revised version after latest comments (comments 43-45)

>class BrlInputEventListener(Thread):
>  '''
>  Monitors brlapi for input events from braille device.
>  
>  @param brldev: a handle to the braille device object
>  @type object: L{BrlAPIDevice}
>  '''

@type field is wrong. Not a biggie. Just noting to myself if I apply the patch as is.

>  
>  def init(self):
>    '''
>    Initializes the braille interface.
>    
>    @ivar brlconn:  connection object to brlapi
>    @type brlconn: object    
>    @ivar writestruct:  write structure for brlapi.write()
>    @type writestruct: object 
>    @ivar inputmngr: Input listener thread reference 
>    @type inputmngr: object 
>    @ivar last_style:  The last style object seen
>    @type last_style: integer 
>    @ivar commands:  List of output commands
>    @type commands: list 
>    @ivar caretpos: The one-based cell position of the caret.
>    @type caretpos: integer 
>    
>    @raise AEOutput.InitError: When the device can not be initialized
>    '''
>    AEInput.AEInput.__init__(self)

I missed this last time. I think the call to AEInput.AEInput.__init__ should go in an __init__ method of this class alongside the call to AEOutput.Braile.__init__. Then construction order is explicity and belongs where it should, in the overriding constructor.

Comment on attachment 78233 [details] [review]
revised version after latest comments.  Touch cursor support

>>   def doMouseClickAtPor(mouseevent):
>>     '''
>>     Performs a mouse event on the center of a given point of regard.  If the
>>     accessible has a role of text, then set cursor at given location in por
>>     
>>     @param mouseevent: mouse event to perform
>>     @type mouseevent: integer of type EVENT_SYNTHMOUSE*
>>     @return: Indicator of whether the acc was clicked or not
>>     @raise LookupError: When the accessible object is dead
>>     '''
>>     pass

Should be capital POR in the method name. Add @see field with reference to AEConstants. Will change when committing.


>>   def doMouseClickAtPor(self, mouseevent):
>>     '''
>>     Performs a mouse event on the center of a given point of regard.  If the
>>     accessible has a role of text, then set cursor at given location in por
>>     
>>     @param mouseevent: mouse event to perform
>>     @type mouseevent: integer of type EVENT_SYNTHMOUSE*
>>     @return: Indicator of whether the acc was clicked or not
>>     @raise LookupError: When the accessible object is dead
>>     '''

Same here. Capital POR.


>===================================================================
>RCS file: /cvs/gnome/lsr/src/Choosers/SettingsChooser.py,v

What caused all the changes in this file all of a sudden?

>Index: src/Task/Tools/View.py
>===================================================================
>RCS file: /cvs/gnome/lsr/src/Task/Tools/View.py,v
>retrieving revision 1.42
>diff -r1.42 View.py
>2413c2413,2444
><     return self.tier.getLastKey()
>\ No newline at end of file
>---
>>     return self.tier.getLastKey()
>>   
>>   def clickPOR(self, mouseevent, por=None):
>>     ''' 
>>     Performs a mouse event on the center of a given point of regard.  If the
>>     accessible has a role of text, then the cursor is set at given location in
>>     por.
>> 
>>     @param mouseevent: Mouse event OF type EVENT_SYNTHMOUSE_*
>>     @type mouseevent: String
>>     @param por: Point of regard to the text area; use the current task's 
>>       L{POR} if none supplied
>>     @type por: L{POR}
>>     @return: Did touch cursor succeed?
>>     @rtype: boolean
>>     @raise PORError: When the L{POR} is invalid
>>     @raise SelectError: When the deselection operation fails
>>     '''

Correct @return field description. Link to L{POR} in paragraph.

Created attachment 78239 [details] [review]
revised version after latest comments.

Fixed in the development version. The fix will be available in the next major release. Thank you for your bug report.