Bug 98206

Created attachment 39622 [details]
proposed implementation

the proposal is to add a SaveAsCookie and its default implementation that will
be added to DataObject's CookieSet in DataEditorSupport

note: unitests will be provided before integration to cvs.

[JG01] Is DataObject.copyRename intentionally not public?


[JG02] Shouldn't the new location and name be arguments to SaveAsCookie.saveAs?
Then the dialog prompting for this information would be part of SaveAsAction,
rather than DataEditorSupport.

Y01 You should not add new methods into interfaces that others implement - 
e.g. you cannot just add method to OperationListener.

Y02 Re. JG01 - I guess I suggested for it to not be public. Imho the less we 
expose the more we can change in future, but I can accept public as well

Y03 Re. JG02 - I like Jesse suggestion a bit more than current state, however 
that implies the SaveAsCookie cannot be in openide/nodes as it will reference 
FileObject. As such it would have to be moved to openide/loaders and 
potentially renamed and changed to not implement Node.Cookie anymore - we 
probably do not want more Node.Cookies outside of org.openide.cookies package. 
Maybe AsSaveable? Then you need own implementation of SaveAsAction - btw. 
please do not expose the class, expose just factory method: ContextAwareAction 
SaveAction.saveAsAction() would be ok, I guess.

Y04 Tests, please.

Y05 I have found out that SaveAsCookie is added/removed to the CookieSet, 
probably it would be better if it was there all the time, so the 
DataEditorSupport should have method saveAs() - this means all its subclasses 
known to us would have to be modified to also implement 
SaveAsCookie/AsSaveable.

Created attachment 40073 [details]
new implementation

i've attached patch for improved implementation:
- no Cookies, generic SaveAsCapable interface instead
- file browser is opened from the action instead of editor support
- no changes to OperationListener interface

if there are no more objections, i'll add some tests and integrate the patch ver
2 tomorrow

the patch is integrated to trunk, see #20147 for cvs commit log

Sorry for the late comments, but I just did not have time to look at this before
the commit:


[JG03] saveAs(FileObject) is strange. It means that the new file must already
exist! Which forces SaveAsAction to create an empty file and then call
saveAs(newFileObj). I don't understand why you don't use the style used
everywhere else, e.g. in createFromTemplate: saveAs(FileObject folder, String name).


[JG04] SaveAsAction either should be a factory method (mentioned in Y03), or at
least not expose API-irrelevant interfaces in its signature (LookupListener and
PropertyChangeListener).


Now some comments regarding the SAA impl:


[JG05] Use FileUtil.createData(File). (If JG03 is ignored, else createFolder.)


[JG06] Enclose everything in an atomic action to prevent race conditions when
e.g. saving XML files whose MIME resolvers pay attention to file contents (again
only relevant if JG03 is ignored).


[JG07] Do not use the broken basename + extension methods in the Filesystems API
(use just createData(String) if not using FileUtil.createData, delete the method
getFileName).


[JG08] Do not log a localized message from Logger.log(Level,String,Exception),
but rather attach using Exceptions.attachLocalizedMessage and then call
log(Level,null,Exception).


[JG09] When setting a default directory in the file chooser be sure to use
FileUtil.preventFileChooserSymlinkTraversal.


[JG10] For readability and stylistic consistency with other code it is best use
the idiom 'if (x == null)' rather than 'if (null == x)' etc.


[JG11] Do not refer to a bundle in a different package than the current class;
when using NbBundle.getMessage always use the enclosing top-level class as a
reference (here SaveAsAction.class).


[JG12] Any particular reason why enablement logic will refuse to enable the
action when you select a file e.g. in the Projects tab?

jg03 - is it ok to change the method signature now that it's already committed
to cvs? (perhaps if i'll do it over the weekend nobody will notice:)

jg04 - jg11 - ok, i'll fix that

jg12 - it's the result of ui review, hie team thought it was strange to call
saveAs on non-open files

Re. JG03 - you can change the signature before release without many 
compatibility issues, I agree that Jesse's signature is better as it 
potentially makes sense even for multidataobjects. 

jg04 - SaveAsAction is now package private with public static create method.
there's no suitable action in that package where the factory method could be added.

jg11 - SaveAsAction is in package org.openide.actions so it would mean adding
the message to a bundle in a different module which doesn't make sense, imo