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
