On Fri, 5 Nov 1999, Cyrille Chepelov (home) wrote:
> However, if I look at newzzline.cpp (cpp ?!?), there's still
Oops, to much looking at Mozilla code at work i suppose.
> plenty of code I think is useless -- useless because of C's lack of
> built-in inheritance mechanism.
> For instance, we'll find this, verbatim, in every orthconn-derived object :
> static void
> zigzagline_select(Zigzagline *zigzagline, Point *clicked_point,
> Renderer *interactive_renderer)
> {
> orthconn_update_data(&zigzagline->orth);
> }
Yes, i agree this one is useless, and in fact since update_data has been
moved to ObjectOps this could be moved to the object base class. So that
you just set object_select as your select function if you don't need
anything fancy.
> Or this :
>
> static real
> zigzagline_distance_from(Zigzagline *zigzagline, Point *point)
> {
> return orthconn_distance_from((OrthConn *)zigzagline, point,
> zigzagline->line_width);
> }
> Since zigzagline inherits from orthconn, we should not need to mention
> that code at all, period. The second routine is a bit trickier, but that's
> probably because orthconn ought to own the line_width property.
This one i don't agree to. Not all orthconn objects are sure to behave in
this way. Wavy or rounded zigzaglines for instance would (should) not use
this code. And you can't just move line_width to orthconn, because all
orthconn based objects might not need/want a line_width property.
> Same for zigzagline_add_segment_callback : this routine adds no value
> (it's not the case for, say, bus_add_dhandle_callback() ; I'm not saying
> that the rightclick menu callback system is wrong, just that it should be
> possible to simply refer to a orthconn_add_segment_callback() directly
> wherever an object doesn't change this part of orthconn's behaviour.
Actually, it should be possible to do use orthconn_add_segment_callback
here now, since update_data was moved to ObjectOps.
> However, I strongly disagree with you with your statement that '''if an
> object type is to be a base type, then it belongs to lib/ .''' (my
> reformulation) ; first, there's the case of object re-branding : I want to
> re-use the current Ethernet bus object, and call it "OR vergence" (to draw
> IEC848 grafcet sheets). I should be able to just provide a new ObjectType
> structure, giving the bus_type_ops and bus_type_properties and a new name.
> Then I need to have a "bus"-like object, with two base lines drawn at a
> line width interval, instead of a single base line,
> and call it "AND vergence". However, I add no new state, no new
> properties, no nothing to the OR vergence, except a new draw routine. I
> should be able to provide only this new draw routine (it wouldn't make
> sense to clutter the ethernet bus' draw routine with the possibility to
> draw two base lines instead of one, so here it makes sense to fork *this*
> routine and keep most the rest[maybe a few tweaks to bus_distance_from]).
I don't think this is a good idea. You add an artificial dependency from
the OR vergence to the Ethernet bus. If the bus object is later changed to
add support for something new (ring topology or whatever) you
automatically get this in the OR vergence object too. This way you can get
a lot of "undocumented" dependencies that makes it hard to change objects
without breaking others.
If you really see the bus-like object as a reusable template for other
objects i think it should be factored in two, just like OrthConn and
ZigzagLine is (and Connection vs. Line, Rectangle vs Element etc).
> okay, I proposed to do this job in XML, externally to the code, but this
> makes sense too.
> I'd add a way to specify min, max value and increment step to the
> Properties structure (maybe a union { void *na; /* N/A = NULL */
> struct { real min,max,step } bounds;
> } as an additional parameter);
Yes, something like that. And you would need the possibility to make some
properties non-editable, like the object position that needs to be saved,
but are edited in other ways.
> and of course, macros in the line of
> #define LINE_COLOR_PROPERTY(objecttype) \
> { "line_color", PROPERTY_COLOR, G_STRUCT_OFFSET(objecttype,
> line_color), &color_black, N_"Line Style:", NULL }
> to shorten the definition and encourage use of common property
> definitions.
Yes. That would be nice.
> However, I totally don't get the point of having this :
>
> [ State functions and structs snipped ]
>
> Isn't there a way to use plain but hidden versions of objects wherverer
> object state is needed ? Then, it could be possible to replace the current
> foo_copy(Foo *foo), foo_set_state(), foo_get_state() by a single
> foo_copy_state(Foo *dest, Foo *src).
Well, state are stored on the undo stack and should be as small as
possible. In this case the states stuff was extremely simple, but not
always should all of the object struct be stored (some might be calculated
on each update_data anyway). Also you don't want to lug around the whole
Base class (OrthConn in this case). Then you need the free routine that is
in ObjectState (it might be possible to use Object.ops->destroy).
It might actually be possible to generate the get/set state functions
given the property table, at some extra memory and performance penalty.
This needs to be carefully thought out though, undo stuff isn't as easy as
it looks sometimes.
> Oh, I noticed that a lot of strings in newzzline.cpp are defined N_()
> though they seem to appear in the UI (see property_list and object menu).
> How's i18n to be handled ?
Well, I'm no expert on i18n (hallon?), but i suppose they have to be
fixed up at some stage of reading objects.
> I know you
> don't like this, but there really should be a PROPERTY_INVISIBLE flag in
> the Properties structure : though it may make sense to have the ability to
> turn an entity into a weak entity, in Grafcet it makes absolutely no sense
> to turn a OR vergent into a AND one. And it wouldn't make sense to fork
> the whole object type definition just for that...
On the other hand, it might make sense to factor of the common parts of
the OR vergent and the AND vergent either in lib/* or in
objects/grafcet/*.
/ Alex
