I like your idea of renaming, although I think I will rename the method in LzNode to be acceptTypeValue and presentTypeValue (because they take a type argument and convert a value according to that type). I will take your suggestion to rename the method in components to acceptValue and presentValue (because, although I permit an optional type, by default they will use the type declared on the component to convert the value of the component).

Now that I look at it that way, I see that really _all_ components should have this acceptValue/presentValue interface, and those components that are text (basic, simpletext, edittext, etc.) should use their text as their value. Then there really can be a default method for applyData/updateData that uses acceptValue/presentValue, and it is only certain components that have additional behavior on updateData that will have to override that.

As a result, I am going to revise this change and re-send it to you for review.

Sorry, but you earned it!  :)

On 2008-11-16, at 10:45EST, André Bargull wrote:

Approved.

I wonder whether it makes sense to switch "LzNode#[accept/ present]Value(..)" with lz.basevaluecomponent#[accept/present](..)", so that we call the general method "accept" and "present" and place them on LzNode, and use "acceptValue" resp. "presentValue" for lz.basevaluecomponent.

Some reasons:
1. the new "accept{..}" constraint (if it gets implemented), uses the LzNode method. If it is "accept()" instead of "acceptValue()", users will see more easily the connection between the constraint and the method. 2. "value" (as a term) is used in basevaluecomponent, but not anywhere in LzNode. So it's more natural to call it "acceptValue()" if we want to change the "value" attribute on lz.basevaluecomponent.


On 11/16/2008 12:48 AM, P T Withington wrote:
Change 20081115-ptw-x by [EMAIL PROTECTED] on 2008-11-15 18:26:45 EST
   in /Users/ptw/OpenLaszlo/trunk
   for http://svn.openlaszlo.org/openlaszlo/trunk

Summary: Respond to review comments on r11780 and r11781

Bugs Fixed:
LPP-7339  Can't use LzNode#presentAttribute in a constraint (previous
fix broke DHTML color conversion)
LPP-7340  basevaluecomponent should have a 'type' so you know how to
accept/present it (previous fix broke updateData protocol)

Technical Reviewer: [EMAIL PROTECTED] (pending)
QA Reviewer: [EMAIL PROTECTED] (pending)

Details:
   LzUtils, PresentationTypes: move (incorrect) conversion of color
   value to string name from LzColorUtils.inttohex to
   ColorPresentationType.present

   baselist: Remove useless override

   baseslider: Use present, not updateData to get the thumb label

   baseformitem: use new present API from basevaluecomponent to
   compute updateData

   basevaluecomponent: Add new API's accept and present which can be
   used to set/retrieve the value as a string according to type.  Fix
   getValue dependencies.  Remove incorrect applyData/updateData and
   updateData dependencies method.  Make sure present uses getValue
   to retrieve the value to be presented.  Correct present
   dependencies method.

Tests:
   Andre's test case from LPP-7340, Lou's color example (revised to
   use 'present' in place of 'updateData').

Files:
M      WEB-INF/lps/lfc/services/LzUtils.lzs
M      WEB-INF/lps/lfc/core/PresentationTypes.lzs
M      lps/components/base/baselist.lzx
M      lps/components/base/baseslider.lzx
M      lps/components/base/baseformitem.lzx
M      lps/components/base/basevaluecomponent.lzx

Changeset: http://svn.openlaszlo.org/openlaszlo/patches/20081115-ptw-x.tar



Reply via email to