Bug 98206 - save-as API review
save-as API review
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Data Systems
6.x
All All
: P2 (vote)
: 6.x
Assigned To: Stanislav Aubrecht
issues@platform
: API, API_REVIEW_FAST
Depends on:
Blocks: 20147
  Show dependency treegraph
 
Reported: 2007-03-19 14:32 UTC by Stanislav Aubrecht
Modified: 2008-12-22 10:57 UTC (History)
4 users (show)

See Also:
Issue Type: TASK
:


Attachments
proposed implementation (36.66 KB, patch)
2007-03-19 14:34 UTC, Stanislav Aubrecht
Details | Diff
new implementation (32.46 KB, patch)
2007-03-28 10:39 UTC, Stanislav Aubrecht
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stanislav Aubrecht 2007-03-19 14:32:03 UTC
add an option to save opened document under a different file name and/or extension
Comment 1 Stanislav Aubrecht 2007-03-19 14:34:38 UTC
Created attachment 39622 [details]
proposed implementation
Comment 2 Stanislav Aubrecht 2007-03-19 14:38:08 UTC
the proposal is to add a SaveAsCookie and its default implementation that will
be added to DataObject's CookieSet in DataEditorSupport
Comment 3 Stanislav Aubrecht 2007-03-19 14:38:49 UTC
note: unitests will be provided before integration to cvs.
Comment 4 Jesse Glick 2007-03-21 21:52:52 UTC
[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.
Comment 5 Jaroslav Tulach 2007-03-27 15:53:18 UTC
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.
Comment 6 Jaroslav Tulach 2007-03-27 16:53:17 UTC
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.
Comment 7 Stanislav Aubrecht 2007-03-28 10:39:43 UTC
Created attachment 40073 [details]
new implementation
Comment 8 Stanislav Aubrecht 2007-03-28 10:41:55 UTC
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
Comment 9 Stanislav Aubrecht 2007-03-29 09:21:03 UTC
if there are no more objections, i'll add some tests and integrate the patch ver
2 tomorrow
Comment 10 Stanislav Aubrecht 2007-03-30 14:30:55 UTC
the patch is integrated to trunk, see #20147 for cvs commit log
Comment 11 Jesse Glick 2007-03-31 00:56:05 UTC
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?
Comment 12 Stanislav Aubrecht 2007-03-31 07:21:14 UTC
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
Comment 13 Jaroslav Tulach 2007-04-02 13:33:12 UTC
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. 
Comment 14 Stanislav Aubrecht 2007-04-04 09:38:43 UTC
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


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo