[Zope3-dev] Re: [Checkins] SVN: zope.app.form/trunk/src/zope/app/form/browser/te FileWidget tries to be smarter about not deleting the currently stored content when the user did not upload a new file.

[Christian Zagrodnick]

On 2007-06-26 22:41:25 +0200, Gary Poster <gary at zope.com> said:

> 
> On Jun 23, 2007, at 6:38 AM, Christian Zagrodnick wrote:
> 
>> Log message for revision 76975:
>>   FileWidget tries to be smarter about not deleting the currently  
>> stored content when the user did not upload a new file.
> 
> ...
> 
>> Modified: zope.app.form/trunk/src/zope/app/form/browser/textwidgets.py
>> ===================================================================
>> --- zope.app.form/trunk/src/zope/app/form/browser/textwidgets.py	 
>> 2007-06-23 10:25:23 UTC (rev 76974)
>> +++ zope.app.form/trunk/src/zope/app/form/browser/textwidgets.py	 
>> 2007-06-23 10:38:37 UTC (rev 76975)
>> @@ -475,6 +475,11 @@
>> 
>>      def _toFieldValue(self, input):
>>          if input is None or input == '':
>> +            # There was no input. With File-Upload this usually  means 
>> that the
>> +            # value should *not* change. Let's try to get the old  value.
>> +            content = self.context.context
>> +            if self.context.interface.providedBy(content):
>> +                return self.context.get(content)
>>              return self.context.missing_value
>>          try:
>>              seek = input.seek
> 
> Hey.  This has a couple of bugs, IMO.  It might also be a misfeature,  
> but I'm less sure about that.
> 
> First, "return self.context.get(content)" assumes that this widget  
> will be used on an edit form (not true for us).
> 
> Second, "if self.context.interface.providedBy(content):" assumes that  
> the schema field is part of an interface (not true for us).

Hum. From that I make that there are not enough tests. :/

I figure that the .interface attribute doesn't seem to be part of any 
interface anyway. So in fact this is not a nice thing to do.

> 
> Neither of these are valid assumptions generally for a form field, IMO.
> 
> I'm also not sure about the semantics.  Shouldn't this be handled  more 
> at the form level?  I can see why you want to do this here, but  it 
> seems pretty gray, and I'm not sure how to solve the bugs in a  sane 
> and reliable way.

No, thinking about it it's not the right way I did it. Basically I 
wasn't aware about the different conexts. In general I know that, but 
sometimes you're just blind :)

> 
> I agree that this is a tricky problem, and it would be nice to solve  
> it, but your solution is not correct for a generic form widget.  The  
> only suggestion I have ATM is to revert this, unfortunately, but I  
> welcome other approaches that solve the issues I raised.

Solving it on form level is of course possible but not really the thing 
you want to think about. I suspect there should be some 'i have not 
changed' marker value.

There is some strange thing in zope.formlib.form.applyChanges 
preventing updates of the value already:

def applyChanges(context, form_fields, data, adapters=None):
    ...
    for form_field in form_fields:
 		...
        name = form_field.__name__
        newvalue = data.get(name, form_field) # using form_field as marker
        if (newvalue is not form_field) and (field.get(adapter) != newvalue):
            changed = True
            field.set(adapter, newvalue)

    return changed


newvalue is not formfield? To me this does not make much sense. Why 
would I return self.context to indicate the value has not changed?

Questions over questions :)


-- 
Christian Zagrodnick

gocept gmbh & co. kg  ·  forsterstrasse 29 · 06112 halle/saale
www.gocept.com · fon. +49 345 12298894 · fax. +49 345 12298891