cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sylvain Wallez <sylv...@apache.org>
Subject Re: before we release - multipart uploads
Date Fri, 13 Feb 2004 10:21:05 GMT
Geoff Howard wrote:

> Sylvain Wallez wrote:
>
>> Geoff Howard wrote:
>>
>>> I dug into upload tonight to help some confusion on the users list 
>>> and noticed some new things which were introduced since 2.1.3 (in 
>>> nov. by Sylvain to support woody).  I'd like to propose some tweaks 
>>> which seem wise enough to me to get in quickly or it will be too late.
>>
>
> ...
>
>>> 2) The method dispose() was added by having Part implement 
>>> Disposable. I like the method, but didn't like implementing a 
>>> lifecycle method on what is otherwise not a component.  I think 
>>> dispose (or another name if the overlapping name is deemed too 
>>> confusing) should just be added to the abstract Part without 
>>> Disposable.
>>
>>
>> I personally don't see any problem with using this interface with 
>> non-components, rather the contrary. Disposable clearly identifies 
>> that some cleanup is required on the object, and everybody knows and 
>> understands its semantics: do some cleanup at the end of the object's 
>> life and never use it again. Sure, we can have dispose() without 
>> implementing the interface, but doing it so IMO gives better 
>> visibility to this important contract.
>
>
> Ok.  Curious - is this the only case we have of a lifecycle interface 
> applied to a non-component?
>
>>> 3) dispose() is public - but it could be protected, or even package 
>>> private.  I can't think of a reason why code outside this limited 
>>> scope (the cleanup method above) should be calling dispose() 
>>> directly, unless Woody has a use for it that I haven't found.
>>
>>
>> dispose() *needs* to be public, as Woody's upload widget calls it. 
>> This widget also calls setDisposeWithRequest(false).
>
>
> Interesting - I couldn't find where woody was using dispose().  Why 
> would it need to dispose instead of setDisposeWithRequest()?


The caller of setDisposeWithRequest(false) becomes responsible for 
disposing the Part, since the request won't do it. This is used by the 
UploadWidget to handle form redisplay without loosing uploaded files: 
when a file is uploaded, the corresponding part is detached from the 
request and is disposed whenever the user replaces the already uploaded 
file.

It's then the responsibility of the code using the form to dispose the 
Part held by the widget when it has finished with it. Note that some 
provision is made for applications that would forget this by deleting 
temporary files in PartOnDisk.finalize(), and also by marking uploaded 
files as deleteOnExit().

>>> 3) A public accessor is created called boolean disposeWithRequest(). 
>>> This seems better called isDisposeWithRequest() and I don't see why 
>>> it needs to be public.  It's intended use is in the cleanup method 
>>> above, unless woody has a use for it that I haven't found.
>>
>>
>> I agree with your naming concern, and you're right in saying it 
>> doesn't need formally need to be public, as it's used only by 
>> MultipartRequest.
>>
>>> Sorry I just noticed these things - I glanced at the cvs mail at the 
>>> time it went by but didn't notice the issues above until I dug 
>>> through tonight.
>>
>>
>> With the above comments, it seems to me the only problem is the 
>> disposeWithRequest() acessor. I guess it's too late now to change it 
>> (Carsten said "don't commit"), but this should not hurt much.
>
>
> I think you're right that it's too late.  The problem is that the 
> misnamed accessor sounds like it performs an action and so may be used 
> by people mistakenly.  Would you be in favor of deprecating it right 
> after release and creating a new method with the new name?  Normally I 
> am not so focused on naming issues, but this is a part of our API that 
> will be touched by a lot of users who already seem to have a hard time 
> with uploads and I'm anticipating a fresh round of questions...


Ok for deprecating, and I even think we could simply remame (and thus 
loose the current name) it a this method is very internal to 
MultiPartRequest and I doubt people will use it.

Sylvain

-- 
Sylvain Wallez                                  Anyware Technologies
http://www.apache.org/~sylvain           http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
Orixo, the opensource XML business alliance  -  http://www.orixo.com



Mime
View raw message