Re: [g-a-devel]gnome-mag IDL

[Michael Meeks <michael ximian com>]

Hi Bill,

On Mon, 2002-09-16 at 13:24, Bill Haneman wrote:
> * ZoomRegions are now objects in their own right, and methods have been
> moved to the ZoomRegion API where appropriate, to avoid the use of the
> "int" parameter specifying which ZoomRegion a method applies to.

	Good; that's great. Int parameter indexing is a festering, racy mess
;-)

> * Magnifier and ZoomRegion now have getProperties(), which returns a
> Bonobo::PropertyBag.

	That's fine, as a convenience helper for accessing part of the
aggregate, we do that in Bonobo in some places.

> * properties now covered by the PropertyBag interface include the old
> property "is-managed" (formerly markUnmanaged).

	Hmm;

> [We might want to move Magnifier::sourceDisplay and
> Magnifier::targetDisplay to the PropertyBag set also.]

	Interesting.

> A number of new properties concerning the cursor, display
> contrast/inverse-video/etc, and zoom region alignment and borders are
> slated for control via the PropertyBag properties (see the inline
> comments below).

	Ok, sounds good.

> Feedback is welcome...

	I have a number of comments:

>     /**
>      * boolean getSourceDisplaySize:
>      * Return the size of the specified source display.
>      * @x1: the minimum X coordinate of the zoomed area bounding box
>      * @x2: the maximum X coordinate of the zoomed area bounding box
>      * @y1: the minimum Y coordinate of the zoomed area bounding box
>      * @y2: the maximum Y coordinate of the zoomed area bounding box
>      **/
>     boolean getSourceDisplaySize (out long x1, out long y1, 
> 				  out long x2, out long y2); 

	First of all; I'd bin the 'out long x1, y1, x2, y2' business,
completely;

	I would personally have a:

struct Dimensions {
	long x, y;
	long width, height;
};

	And pass that to all the methods; it's marginally more efficient, and
better expresses what is going on to my mind; also it can be used to
provide properties of the same name:

>     /**
>      * short createZoomRegion:
>      * Creates a new zoom region for the magnifier.
>      * The new region is initially unmanaged'.
>      * @zx: the scale factor in the x direction for the new zoom region
>      * @zy: the scale factor in the y direction for the new zoom region
>      * @x1: the minimum X coordinate of the zoomed area bounding box
>      * @x2: the maximum X coordinate of the zoomed area bounding box
>      * @y1: the minimum Y coordinate of the zoomed area bounding box
>      * @y2: the maximum Y coordinate of the zoomed area bounding box
>      **/
>     short createZoomRegion (in float zx, in float zy,
> 			    in long x1, in long y1, 
> 			    in long x2, in long y2); 

	This looks like a method to set the ROI / bounding area of the zoom
window, in which case there is a duplicate method method on the
zoomRegion for one of these; I'd tend to avoid duplicating only part of
that code. - Perhaps I misunderstood setROI / resizeWhatnot ...

	I think the indexing thing is probably a mistake; I'd just do:

	ZoomRegion addZoomRegion ();

	But I'd also have a :

	typedef sequence<ZoomRegion> ZoomRegionList;

	ZoomRegionList getZoomRegions ();

	method as well ...

>     /** 
>      * void exit: 
>      * Unmap the current magnifier from the display.
>      **/

	I think it's best to use the method name 'Dispose' to dispose this, (
and the ZoomRegion ) interfaces.

	Indeed; I'd very much lika a 'Bonobo_Disposable' interface inside
libbonobo, if you'd like to hack it up that way [ since this is 2.2 work
yes ? ].

>     void setMagFactor (in float magX, in float magY);

	Again; I'd have a struct here, to make it cleaner, and so we can have a
MagFactor property in the bag, that takes the same struct. [ indeed why
expose these set/gets at all ? ].

>     oneway void setROI (in long x1, in long y1, in long x2, in long y2);


>     /**
>      * oneway void setROI:
...
>     void markDirty (in long x1, in long y1, in long x2, in long y2); 

	cut and paste error on the copy ;-)

>     void destroyZoomRegion ();

	dispose is a good name again.

	HTH,

		Michael.

-- 
 mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot