Created attachment 76224 [details]
Starting code

Instructions for installing/using the magnifier script as-is: (assumes you have lsr installed from CVS)

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

From what George last told me, if you now run lsr, the magnifier should start. When you quit LSR (e.g. Ctrl-C at the prompt, pkill lsr, etc.) it should shutdown properly. This may have changed as we haven't yet tested it on GNOME 2.16.

More notes following about the plan of attack for adding magnifier support...

Clean-up:

1) Switch __del__(self) to close(self) on GnomeMagPerk class. (bug #359113 is fixed in CVS).
2) Get rid of the ReadPerkName class and all references to it. (It was there for testing.)

Plan of attack:

1) Mouse tracking based on the MouseTrack option.
2) Focus tracking based on a new option.
3) Active selector tracking.
4) Caret tracking.
5) Basic settings for enabling/disabling the magnifier, full screen mode, etc.
5) Fancy settings for zoom level, smoothing, etc.

Iterating over each of these in separate comments...

Mouse tracking:

1) Create a subclass of Task.MouseTask.
2) Override the execute methods. Look at lsr/src/Task/MouseTask.py to see what parameters this method expects. (It's new and not in the generated HTML documentation yet).
3) Look at the IDL for the gnome-mag bonobo component. (http://cvs.gnome.org/viewcvs/gnome-mag/idl/GNOME_Magnifier.idl?rev=1.11&view=markup)
4) Call the appropriate method on the magnifier instance to move the mouse pointer. You can access the magnifier instance from the Task class you're writing via self.perk.mag. (Our convention is that isntance variables in a Perk class are public to Tasks in the same module. If that bothers you, go ahead and implement getters/setters or delegation methods on the Perk. Up to you.)
5) Test, debug, iterate!

Once tracking is implemented, we'll tie it the user configurable setting. Get this far first, attach your code to this bug report, and I'll describe what to do next.

Created attachment 76309 [details]
Initial mouse tracking support

Not perfect, but it works.
Seems pretty laggy over here, but your mileage may vary.
Gnopornicus has a different behavior in which the cursor could push the boundary box around, kind of like using an oversized X server. Maybe there should be a setting to toggle between different mouse behaviors. Don't know.

Created attachment 76311 [details]
Initial mouse and focus tracking support

I refactored it a bit, and added focus tracking support.
Just set MOUSE_FOLLOW to False to see that. It doesn't actually center the magnifier on the widget yes, but it obviously behaves correctly.

Excellent. I tried using your original code and found it a little laggy too, but not terribly so. We can optimize later in a number of ways. However, don't spend too much time on mouse tracking. We've got a enhancement request in to expose the gnome-mag to track the mouse itself. If that becomes available, we'll just flip that switch instead of tracking the events ourselves. (Clearly, Python is adding some overhead to the handling and dispatch of all those events).

I'll try out your new code today. Continue on with the active selection (gray rectangle around the active item in a list, tree, table, tree table, etc.) You can override Task.SelectorTask.executeActive for notification.

The getAccFocalPoint code was really just thrown into our API without any testing in order to get something that might work for the magnifier. 

Later today we can talk about how to make magnifier settings available in the LSR settings dialog. If you want a head start, look at the Perk.PerkState subclass at the top of your file. All you need to do is mimick what George did for zoom level for the following settings:

1) Track mouse?
2) Track focus?
3) Track caret?

Look at the DefaultPerk if you want to see a slightly more advanced use of settings in groups. Look at SettingsTask for the class you'll need to subclass and register to receive notifications about changes to the settings.

You're right that we'll want settings like gnopernicus to choose whether the magnifier window follows the mouse cursor around. Once the basic ones are in place, we can add others or remove current ones as necessary, change types, etc. as necessary.

Also, you might want to clean-up the initialize() method. There's a pointless for loop at the end of it, for instance.

I have a feature request to programatically turn on and off the mouse tracking.  I believe that gnopernicus and orca drive the magnifier without mouse tracking turned on.

http://bugzilla.gnome.org/show_bug.cgi?id=359071

PS: dont forget about the multiple zoom areas.

Created attachment 76339 [details] [review]
proposed fix for getAccVisualPoint

The coordinate math there seemed to be a bit off. I think my change is the intentional behavior.

Created attachment 76342 [details] [review]
Fixes getAccVisualPoint TextAdapter.py

'text' was a undefined local variable.

Created attachment 76356 [details] [review]
Fixes getAccVisualPoint TextAdapter.py

Besides cleaning up the previous change (which made two text interface instances).
This one makes the method return sensible coordinates, although I am not sure if it is bomb-proof.

Another suggestion, perhaps there should be another method besides getAccVisualPoint that returns the upper left coordinate, width, and height (x, y, w, h), this could maybe be used in the future to make more informed decisions about placement in gnome-mag.

Created attachment 76360 [details]
GnomeMagPerk

Added caret tracking.
Made all the tracking methods configurable.

(In reply to comment #12)
> Another suggestion, perhaps there should be another method besides
> getAccVisualPoint that returns the upper left coordinate, width, and height (x,
> y, w, h), this could maybe be used in the future to make more informed
> decisions about placement in gnome-mag.
> 

getAccVisualExtents is defined in Task.Tools.View to give you the bounding box as a four-tuple (x,y,w,h).

Plan of attack revisited:

1) Mouse tracking based on the MouseTrack option. (DONE)
2) Focus tracking based on a new option. (DONE)
3) Active selector tracking. (DONE)
4) Caret tracking. (DONE)

5) Basic settings for enabling/disabling the magnifier, full screen mode, etc.
6) Fancy settings for zoom level, smoothing, etc.

To listen for changes to the magnifier settings:

a) Subclass SettingTask.
b) Compare the state param to self.perk_state to make sure it's a setting change of interest to the magnifier Perk (i.e. not a change for another Perk).
c) Use the string name to decide what setting changed. This is the name of the class/instance variable of the setting, not the localized name displayed to the user.
d) Use either the value provided to the SettingTask execute method or the value of getattr(state,name) to update the magnifier appropriately. Both values are equivalent.

Additional options to define:

Enable magnifier? (boolean)
Smooth panning? (boolean)
Stick to mouse? (boolean, magnifier rect follows the mouse pointer instead of being fixed on the screen, find a better name?)
Screen smoothing (enumeration/choice for all options supported by the magnifier)
Viewport position (two int options, will be disabled when stick to mouse is true and settings support preconditions)
Viewport size (two int options)
Cursor color (probably a choice/enumeration of some reasonable values)
Cursor scale (bounded float)
Border size (bounded int)
Broder color (probably a choice/enumeration of some reasonable values)

New additions:

7) Smarter handling of magnifier changes in cases where a selector/caret event immediately follows focus, languages are right-to-left instead of left-to-right, the caret/selector moved within the magnified region, etc.

Try "biasing" the magnifier as Larry suggested in his email to avoid nasty jumps and leaving the magnifier in place when the caret/selector moves within the magnified region. See what you can find out about rtl languages in AT-SPI.

8) I forgot it by the time I got here. Doh! I'll post a follow up when I remember.

Created attachment 76508 [details]
GnomeMagPerk

Added Panning effect, refactored all magnifier translation code in to
a seprate class (MagTranslate).

TODO:

Traceback (most recent call last):

+ Trace 86163

<parente> when i start lsr, the magnifier doesn't zoom on the terminal application immedialy
 er, immediately
 this is because the focus event on the terminal happened before LSR was running
 it could be because of that
 if you override ViewTask
 executeFirstGained
 you can get a reference to the foreground view (window) when LSR starts

Created attachment 76535 [details]
GnomeMagPerk

I moved all gnome-mag specific code to a separate module. And I made new classes that share the attributes of the ORBit objects.

I tried your code and it runs for me. I'm a bit concerned about the growing code size just to get the smooth panning working. Keep that code for now, but concentrate on getting the other features in Comment #15 implemented. Make sure you can at least switch panning on and off with an option in the settings dialog, and maybe even have that option toggle the use of all the extra code for panning or not. We'll revisit panning when the basics are working.

Created attachment 76614 [details]
GnomeMagePerk

Put zoom and magnifier instances on the state
object. Added a few configurable options:
	* Zoom
	* Smoothing
	* Cursor scale
	* Cursor color
	* Crosswire size

Did further refactoring and useless abstracting.

Created attachment 76626 [details]
GnomeMagePerk

Added basic viewport resizing and moving

Action items moved to http://live.gnome.org/LSR/ScratchPad/Magnifier

Created attachment 77776 [details]
GnomeMagPerk

Magnifier code implemented as device.

Created attachment 77777 [details]
GnomeMagDevice

Device file

Created attachment 78157 [details]
Perk for magnifier

Adjusted to work with new AEState object

Created attachment 78158 [details]
Device for magnifier

New AEState changes

Created attachment 78195 [details] [review]
GnomeMagPerk

Further cleanup and documented. I could probably continue doing this for a long time. Maybe I'll post a new version tomorrow.

Created attachment 78196 [details]
GnomeMagDevice

Further cleanup and documented. I could probably continue doing this for a long time. Maybe I'll post a new version tomorrow.

Comment on attachment 78195 [details] [review]
GnomeMagPerk

>import gobject

Now is the time to get rid of the gobject reference from the Perk. Let's discuss how to do this.

>class GnomeMagState(Perk.PerkState):
>  '''
>  Defines a single set of state variables for all instances of the 
>  L{GnomeMagPerk} and their L{Task}s. 
>  The variables are configuration settings that should be respected 
>  in all instances of L{GnomeMagPerk}.
>
>  @cvar MouseTrack: Track cursor
>  @type MouseTrack: boolean
>  @cvar FocusTrack: Track focus
>  @type MouseTrack: boolean
>  @cvar CaretTrack: Track caret
>  @type MouseTrack: boolean
>  @cvar SmoothPan: Smooth pan
>  @type MouseTrack: boolean
>  @cvar PanAccel: Pan acceleration
>  @type PanAccel: int
>  @cvar PanVelocity: Pan velocity
>  @type PanVelocity: int

The settings are no longer class variables and epydoc will complain if you document them as such. Look at AEOutput/Style.py in the AudioStyle class for how I documented settings there and mimick that for now.

>  end_point = None

Document this. I don't know what it is. (Well, I do, but I won't tomorrow.)


>  def getDescription(self):
>    return _('Gnome Magnifier.')

The Perk is no longer specific to the gnome-mag. Better description might be "Basic magnification services" or something like that.

>  def goToPos(self, pos):
>    '''
>    Send command to magnifier device to center zoomer on given coordinate.
>    
>    @param pos: Coordinate
>    @type pos: tuple
>    '''
>    self.send(CMD_GOTO, pos)

This will become a Task.Tool method in the future so it's good that you wrapped it already here.

>class IncreaseZoom(Task.InputTask):
>  '''
>  Increase the magnification factor.
>
>  @see: L{DecreaseZoom}
>  '''
>  def execute(self, **kwargs):
>    style = self.getStyle()
>    if style.Zoom + 0.5 <= style.ZoomMax:
>      style.Zoom += 0.5
>
>class DecreaseZoom(Task.InputTask):
>  '''
>  Decrease the magnification factor.
>
>  @see: L{IncreaseZoom}
>  '''
>  def execute(self, **kwargs):
>    style = self.getStyle()
>    if style.Zoom - 0.5 >= style.ZoomMin:
>      style.Zoom -= 0.5
>

We don't *have* to have keyboard commands for zooming in and out. George just added those to test before we had proper settings. I have no idea how much a typical mag user zooms in and out, so you probably want to axe them until there's a real use case.

Comment on attachment 78196 [details]
GnomeMagDevice

>@organization: IBM Corp
>@copyright: Copyright (c) 2006 IBM Corp

Don't abbreviate Corporation if it's not abbreviated in all the other modules. It causes additional hits when I do the code scan for legal review. (Stupid, yes, I know.)

>  def init(self, device):
>    self.newBool('RunMag', True, _('Start magnifier on startup'),
>                 _('Start magnifier with screen reader.'))

I'm not 100% sure if we want this to be an option or not. There is already infrastructure elsewhere in LSR to enable/disable devices. Let's leave it out for now.

>
>    self.newBool('FullScreen', False, _('Full screen'),
>       _('Magnify in full screen mode, this only affects magnifiers that are in a seperate screen.'))
>    self.newRange('Zoom', 2, _('Zoom Factor'), 1, 13, 1,
>       _('Change the magnification zoom factor.'))
>    self.newBool('Invert', False, _('Invert magnifier'),
>       _('Invert the colors displayed by the magnifier'))
>    self.newEnum('SmoothingType', 'none', _('Smoothing method'),
>              {'None':'none','Bilinear':'bilinear-interpolation'},
>              _('Change the method used to smooth the magnification'))
>
>    self.newNumeric('TargetDisplayX', 0, _('Left boundary'), 0, 1600, 0,
>                 _('Left boundary of magnifier display'))
>    self.newNumeric('TargetDisplayY', 0, _('Top boundary'), 0, 1200, 0,
>                 _('Top boundary of magnifier display'))
>    self.newNumeric('TargetDisplayW', 100, _('Magnifier width'), 0, 1600, 0,
>                 _('Width of magnifier display'))
>    self.newNumeric('TargetDisplayH', 100, _('Magnifier height'), 0, 1200, 0,
>                 _('Height of magnifier display'))
>    self.newString('TargetDisplayScreen', '', _('Magnifier screen'),
>                 _('Put the magnifier on a separate display'))
>    self.newBool('TargetDisplayConfigured', False, 
>                 _('Target Display Configured'), 
>                 _('Set to True once the target display has been configured'))
>
>    self.newRange('CursorScale', 2, _('Cursor scale'),1, 13, 1,
>               _('Scale of cursor in magnified display'))
>    self.newColor('CursorColor', (0x0, 0x0, 0x0), _('Cursor color'),
>              _('Color of cursor in magnified display'))
>    self.newNumeric('CrosswireSize', 1, _('Cursor crosswire size'), 0, 600, 0,
>                 _('Size of cursor crosswire in magnified display'))
>    self.newColor('CrosswireColor', (0x0, 0x0, 0x0), _('Crosswire color'),
>              _('Color of crosswire in magnified display'))
>    self.newBool('CrosswireClip', False, _('Clip crosswire'),
>              _('Clip crosswire around cursor'))
>
>    self.newRange('ContrastRed', 1, _('Red Contrast'), 0, 50, 1,
>                _('Adjust the red contrast of the magnifier'))
>    self.newRange('ContrastGreen', 1, _('Green Contrast'), 0, 50, 1,
>                _('Adjust the green contrast of the magnifier'))
>    self.newRange('ContrastBlue', 1, _('Blue Contrast'), 0, 50, 1,
>                _('Adjust the blue contrast of the magnifier'))

Make sure all these get documented. The basic ones should probably move to AEOutput/Style.py to a class called MagStyle later.

>  property_trans_zoom = {'Invert': 'inverse-video',
>                         'SmoothingType': 'smoothing-type',
>                         'ContrastRed': 'red-contrast',
>                         'ContrastGreen': 'green-contrast',
>                         'ContrastBlue': 'blue-contrast'}
>  property_trans_mag = {'TargetDisplayScreen': 'target-display-screen',
>                        'CursorScale': 'cursor-scale-factor',
>                        'CursorColor': 'cursor-color',
>                        'CrosswireSize': 'crosswire-size',
>                        'CrosswireColor': 'crosswire-color',
>                        'CrosswireClip': 'crosswire-clip'}

Good.


>  def init(self):
>    '''
>    Initializes the gnome magnifier.
>
>    @raise AEOutput.InitError: When the device can not be initialized
>    '''
>    data = bonobo.activation.activate_from_id(MAGNIFIER_IID)
>    if data is None:
>      raise RuntimeError('Could not activate:', MAGNIFIER_IID)

We actually have an error defined in AEOutput/Error.py which you want to raise to indicate the device could not initialize. DeviceManager watches for that exception and does the right thing (TM).

>  def postInitOutput(self, sett_man):
>    '''
>    Apply default style preferences and initialize zoom region.
>    
>    @param sett_man: Settings manager
>    @type sett_man: L{SettingsManager}
>    '''
>    AEOutput.AEOutput.postInitOutput(self, sett_man)
>    self._initMag()
>
>    self._getZoom()
>
>    for setting in self.default_style:
>      setting.addObserver(self._updateSetting)
>      if not setting.name.startswith('TargetDisplay'):
>        self._updateSetting(self.default_style, setting)

Don't do this per our IRC convo. We'll come up with an explicit method to override that will be called after the style has been initialized.
    

>  def close(self):
>    '''
>    Stop and close the magnifier device.
>    '''
>    self.mag.dispose()

Do we need to del self.mag too or self.mag.unref?

>  def send(self, name, value, style=None):
>    '''
>    Perform given command, or simply apply all dirty style properties.
>    '''
>    if name is CMD_GOTO:
>      if value:
>        self.setPos(self.zoom,value)
>      else:
>        return self.getPos(self.zoom)

Might want a separate command for getting position. A bit hidden to reuse CMD_GOTO for both cases. Maybe a different prefix on constant names like ASK_POS might help too. Definitely split this in two though.

>  def getScreenSize(self, display_name):
>    '''
>    Get the size of a given screen.
>    
>    @param display_name: Name of display.
>    @type display_name: string
>    @return: Width and height of display, or (None, None) if there
>    was trouble retrieving display size.
>    @rtype: tuple
>    '''
>    try:
>      display = Display(display_name)
>    except RuntimeError:
>      return None, None
>    screen = display.get_default_screen()
>    w, h = screen.get_width(), screen.get_height()
>    return w,h

If used only by this class, prefix with _.

>  def isDisplay(self, display_name):
>    '''
>    Checks if given screen exists
>    
>    @param display_name: Name of display.
>    @type display_name: string
>    @return: True if screen exists, False if not.
>    @rtype: boolean
>    '''
>    try:
>      display = Display(display_name)
>    except RuntimeError:
>      return False
>    return True

Same here.

>  def setPos(self, zoom, pos):
>    '''
>    Center zoomer at coordinate
>    
>    @param zoom: Zoomer to adjust.
>    @type zoom: GNOME.Magnifier.ZoomRegion
>    @param pos: position in (x, y)
>    @type pos: tuple
>    '''
>    roi = zoom.getROI()
>    w = roi.x2 - roi.x1
>    h = roi.y2 - roi.y1
>    x, y = pos
>    self._roi = GNOME.Magnifier.RectBounds (x - w/2,
>                                            y - h/2,
>                                            x + w - w/2,
>                                            y + h - h/2)
>    zoom.setROI(self._roi)
>
>  def getPos(self, zoom):
>    '''
>    Get center coordinate of zoomer
>    
>    @param zoom: Zoomer to adjust.
>    @type zoom: GNOME.Magnifier.ZoomRegion
>    @return: Position in (x, y)
>    @rtype: tuple
>    '''
>    roi = zoom.getROI()
>    x_offset = (roi.x2 - roi.x1)/2
>    y_offset = (roi.y2 - roi.y1)/2
>    return roi.x1 + x_offset, roi.y1 + y_offset

And here and here.

>  def _updateSetting(self, style, setting):
>    '''
>    Apply style attribute to magnifier or zoomer.
>    @param style: Style object ot retrieve attribute from
>    @type style: L{AEOutput.Style}
>    @param name: Name of attribute
>    @type style: string
>    '''

I assume you registered this method as the callback for value changes on all settings that need to be updated as soon as they change. Is that working for you? I didn't catch the bit of code that does the registration above. Are you doing it in the Style class's init() method where you have a reference to the device? Or in the device init? Or in the postInitOutput (which is changing)?

Created attachment 78241 [details] [review]
GnomeMagPerk

Updated with latest comments

Created attachment 78343 [details] [review]
GnomeMagPerk

Created attachment 78344 [details] [review]
GnomeMagDevice

Eitan,

[Tier ERROR 2006-12-14 14:04:52,045] MouseTrack: 0 {'button': None, 'kind': 0, 'pos': (205, 430)}
Traceback (most recent call last):

+ Trace 94194

Get this when I unload the magnifier device in the settings dialog.

Created attachment 78388 [details] [review]
GnomeMagPerk

* GnomeMagDevice.py (Module): Silently fail on COMM errors
* GnomeMagPerk.py (Module):  1. smooth load/unload of device 2. no jerkiness when all tracking methods are enabled 3. center vertically, and only snap when necessary 4. Added caret panning option

Created attachment 78390 [details] [review]
GnomeMagDevice