cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Antonio Gallardo" <agalla...@agssa.net>
Subject Re: [CForms] Support for multipe unique-row-id in Repeater
Date Thu, 04 Mar 2004 04:04:19 GMT
Joerg Heinicke dijo:
>>  <wb:repeater id="myRepeaterId" parent-path="." row-path="TheRowPath">
>>    <wb:unique-row>
>>      <wb:unique-field id="myId1" path="myId1"/>
>>      <wb:unique-field id="myId2" path="myId2"/>
>>    </wb:unique-row>
>>    <wb:on-bind>
>>      <wb:value id="myId1" path="myId1"/>
>>      <wb:value id="myId2" path="myId2"/>
>>      <wb:value id="field1" path="field1"/>
>>      <wb:value id="field2" path="field2"/>
>>    </wb:on-bind>
>>  </wb:repeater>
>
> It was a good idea to replace the both attributes with a more
> sophisticated XML structure, but it is a bad realization in my opinion.

I posted a proposal for changes before I started to code it. Nobody answer
(a silent aproval?), then I started the coding. Only Tim answered and give
me his OK.

The good news is it is not too hard to change the code and/or tag names. I
agree with you: we can do it far better. But how to start if nobody cares
about this change? On the other way I don't wanted to have this as another
"aproved change" without implementation. Seems like coding is a good tool
to put little pressure on other committers to review a proposal. Your
comments are very welcomed. ;-D

> <rant>
> The above is redundant, irritating (unique-field is not really correctly
> named, is it?)

Not sure, we can change it, I don't got long time thinking in the "right"
name. I am willing to change it for a more descriptive name. Can you give
us a suggestion?

> and (because of the more java code we need) bloated.
                                              ^^^^^^^
                            (Don't understand the word).

> On
> the one hand the redundancy above is obvious,

The redundancy was always there (using the old attributes you will see
it), maybe now it is more clear (evident) than before....

> on the other hand
> sentences like "This unique-field element ... The id and path attributes
> have the same meaning as in <wb:value>. ... The wd:convertor ... For
> more info see the description of this element in <wb:value>." will get
> me suspicious. Why the ยง$%& we need an additional XML element if we have
> already one that seems to be perfect for it: wb:value as the frequent
> references above show?

I thought about this too. The <wb:unique-field> description don't need all
the attributes of a <wb:value> element. After thinking on it, I thought it
would be better (even from a descriptive POV) to have another tag
describing clearly what the <wb:unique-field> is intendend for. It is a
striped down version of <wb:value> and conceptually they are diferent.

On the other side I don't mean to change the binding to much to allow a
back compatibility with the old approach.

> Why do we have to specify @id and @path twice for
> those identifying elements? And so on.

Not necesary at all. Note, sometime you don't define (or want to define) a
<wb:value> inside the <wb:on-binding> the key values. So it is valid too:

  <wb:repeater id="myRepeaterId" parent-path="." row-path="TheRowPath">
    <wb:unique-row>
      <wb:unique-field id="myId1" path="myId1"/>
      <wb:unique-field id="myId2" path="myId2"/>
    </wb:unique-row>
    <wb:on-bind>
      <wb:value id="field1" path="field1"/>
      <wb:value id="field2" path="field2"/>
    </wb:on-bind>
  </wb:repeater>

> Such changes should be well
> thought out, otherwise we have to change to much later on when
> sophisticating our XML elements.

They are, it cannot interefere with a more complex XML elements. Less is
sometimes better than nothing. It is just a first step. If this first step
was not taken we will not being discussing this issue at all. ;-)

> And the latter we do this the more we
> will break. Unfortunately not many Woody developers are really active on
> the list at the moment.

Because of short of time I don't posted this change to the user list. It
is my fault. Now, I am not sure if I can post on the user list about the
change since this mail looks like a total non-approval of the proposal.
Then why to ring a bell to the users? We can easily undo all the changes
and nothing will happens, then from a POV of users: it never happen at
all. :-)

> </rant>
>
> Another thing that I don't like is the new restriction:
> "Note: This binding is only active in the 'load' operation, so
> specifying the direction="save" is meaningless."

This is another thing that was there all the time, even before the
changes. I just replicated the same approach at it was before. The change
is just related to have multivalue "unique fields" instead of just one
(old style). Nothing more nor nothing less.

> Sorry, Antonio, but you seem to be often the targets of my rants ...

No problem at all. This help us to improve Cocoon. I am glad this happen,
keep on it. ;-)

The worse to me, will be when nobody will care on the work I do.

>> NOTE: The "old style" is also supported. You don't need to rewrite your
>> code. But I think we can also deprecate the old way (using attributes).
>
> I would not let live these deprecated attributes that long. Just like we
> did it for @readonly => @direction I would remove it as fast as possible
> and therefore do a big ANNOUNCEMENT on the users list.

OK. But I need an approval before make an ANNOUNCEMENT to the user list.
Let discuss it. I am sure we can improve the current solution.

> BTW, the wd:convertor element can also be deprecated and removed as it
> is used in combination with the unique-row-id.

Yep. I forgot to write about this. I already review the changes you do on
wiki. Thanks for you time to do this.

Some comments about other ways we droped while designing:

1-An initial name we tought was:

<wb:unique-field> --> <wb:key>
But this was for us much DB oriented and we are talking about forms here.
Carlos also pointed aout that a "primary key" in fact is just one. That
the "primary key" can be composed from more than one field, is another
story. Then we droped the <wb:key> name in favor of the current ONE that
looks more descriptive.

2-Another initial proposal was:

<wb:repeater id="myRepeaterId" parent-path="." row-path="TheRowPath">
    <wb:on-bind>
      <wb:value id="myId1" path="myId1" unique="true"/>
      <wb:value id="myId2" path="myId2" unique="true"/>
      <wb:value id="field1" path="field1"/>
      <wb:value id="field2" path="field2"/>
    </wb:on-bind>
</wb:repeater>

NOTE: see the new @unique on <wb:value>.

PRO:

It is shorter to write.

CON: More dificult to parse, some problems can rise. Need to be used with
@direction to state when we need it just as a readonly attribute. This can
overcomplicate things.

If you review the old and the new RepeaterJXPathBinging code, you will
note there is an issue:

"What happen when we change a <wb:unique-row-id> or a <wb:unique-field>?"

In fact given the current (and old) implementation, a change of the value
in this is done in this way:

"Remove the old one, then create a new one".

Why care about this? Because many DB implementation allow us to update the
Primary Key - PK (read <wb:unique-row-id> or <wb:unique-field>) without
removing the register at all, also we can define FOREIGN KEYS as "auto
updatables" (in case the PK on the other table change).

While review the code, I wondered (when discovered it) how good OJB can
manage it. But seems like other guys on the user list (not using OJB) are
having problems with this issue. Desafortunatelly, I cannot tell how to
fix this since I am not sure if this is work for CForms at all.

Last week, I spent the time learning this code. Don't wonder why I can
talk too much about it ;-)

Best Regards,

Antonio Gallardo.

Mime
View raw message