[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.

[Gary Poster]

On Jun 27, 2007, at 2:49 AM, Christian Zagrodnick wrote:

> 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. :/

Heh, yeah.

I suspect z3c.form is the way forward for us eventually.  One never  
has enough tests, and 100% line coverage is only one way to count  
things, but the z3c.form story seems improved in many ways,  
importantly including its testing story.

> 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 relate.

>> 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.

Right, I definitely understood your goal.

> 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?

As the comment says, looks like form_field is being used as a  
marker--.get should (hopefully) never return form_field, which is  
precisely the point: that way, if get returns form_field, that means  
that there is not a new value of any sort.

Anyway, to the original problem.  As I said, I don't know what to do  
except suggest that you revert here.  Are you willing and able?

Gary