Do you want to make compatible or incompatible change?

I hope you want compatible one and as such apireviews are here to help you 
ensure that your change is compatible. That is why my answer is that you need 
review and a bunch of tests that will for example guarantee that everything 
shown in DataObject.getRegistry().getModifiedSet() is really shown during the 
shutdown.

I am not advicing any particular solution as a fix, but I agree that this is a 
needed change. People do request it often. Just keep in mind that it should 
probably work together with Save All action. So if you do Save All and then 
Exit, you will not get the dialog at all.

Ha, good that you mentioned SaveAll. We actually have our own versions of these
as well:
http://www.netbeans.org/source/browse/contrib/tool/src/org/netbeans/modules/tool/actions/

Maybe the issue would be to also update the original ones to include this
functionality ;-))

As for compatibility: I would of course do everything compatibly, calling the
original methods + the code that is in our actions. But it depends on what you
call compatibility: it will behave differently, now it does not do anything for
the save cookies in the top components lookups and with the new version it would
do something.

Little tricky will be how to avoid duplicates (data object modified, top
component opened with a save cookie). Can I check for the save cookie equality?
I am not sure ...

Duplicates are indeed a problem. 

Slight incompatibility by doing more seems inevitable. Better then doing less.

Equality of cookies might work. Needs a test.

Btw. A bit different approach is to move the registry of modified objects to 
openide/windows API and let the openide/loaders one delegate to them...

IMHO we need a proper API/SPI for finding "modified things that need to be
saved" that is independent of both openide/loaders and openide/windows. This
need was identified years ago as part of the investigation of "Loaders II".

The new API should be used from both SaveAllAction (which should be moved from
openide/loaders to openide/actions) and ExitDialog; and
DataObject.Registry.getModifiedSet() should be deprecated, with a bridge into
the new SPI for compatibility.

Maybe something like:

public abstract class Savable {
  public abstract void save() throws IOException;
  public abstract String displayName();
  public abstract Icon icon();
}
public class SavableRegistry {
  public static Collection<? extends Savable> savables();
  public static void add/removeChangeListener(ChangeListener l);
}
public interface SavableProvider {
  Collection<? extends Savable> savables();
  void add/removeChangeListener(ChangeListener l);
}

with a DataObjectSavableProvider in global lookup which scans
DO.Registry.modifiedSet and translates each DO (valid and with a SaveCookie) to
a Savable whose displayName and icon are taken from the nodeDelegate.

We will do planning for the next 5 weeks tomorrow. This looks like bigger task
than I originaly thought. But I am still willing to do that if agreed on the API
shape. Should I start with a fresh module? openide/saver maybe? with packages
org.netbeans.api.saver, org.netbeans.spi.saver, org.netbeans.modules.saver?

I have another rather big task with the shortcuts dialog (see
contrib/tool/optionsKeymap) I will have to decide which one will I do first. To
know a bit about 6.0 schedule would help me a bit here ... ok, I will try to get
funding also for this one ;-)

Also with a completely new API it would most probably require proper API review
(not the fast track). As soon as I will have time to do this, I will change the
status of this issue to started. And after I will have some code I will try to
ask for the review.

I doubt we need a new module just for this, but I am not quite sure where it
should go if not. Yarda any opinion? openide/util seems too low-level but
nothing else looks right (and LifecycleManager is already there).

I agree that this is the same category as LifecycleManager. Also agree that it
kind of pullutes util with something that is not really utility. And also agree
that it is too small to deserve its own module. So what now?

Hmm. projects/queries is the only place I can think of. Not ideal but could
possibly work.

Wherever it goes, perhaps LifecycleManager should be moved to be in the same
module (but keep the same package), using moduleautodeps for compat.

No, no, not project/queries. I don't inlclude it in our platform. I could change
that but I don't think we want to have anything related to "projects" ...

projects/queries actually isn't related to projects in any meaningful way,
that's just where it wound up in CVS. In fact it is already part of the platform
cluster, and used by relatively low-level modules like masterfs. It is small and
adds no overhead if unused.

1. I am not sure why openide/windows was rejected. I see the note, but I do 
not understand the reason. I can imagine possible digust, but given our 
current problems to find right module, I would consider it the cleanest 
solution. 

2. project/queries. If openide/masterfs depends on that module, then I 
disagree with putting there some visual-only support like list of saveables.

3. lifecycle manager could be moved into the same module as support for 
saveables, but it keeping it in its old package is not reasonable. Better to 
deprecate the old interface, and create one brand new.

4. this is probably too early advice, but please guys, any API shall imho 
follow split of client vs. provider API. Actually maybe the API can really be 
just LifecycleManager.saveAll() and LifecycleManager.exit(), and some listener 
support.

5. It would desirable for the new API to replace the private communication 
channel between core/execution, openide/progressapi and ExitDialog.

6. I really like the idea of SavableProvider. Such class would perfectly 
complement LifecycleManager, it could be the channel for openide/loaders, 
core/execution and openide/progressapi and any other module wishing to provide 
modify all functionality including core/windows doing that on nodes of opened 
components(of course beware of duplicates).

Re. #1: -1 on openide/windows. Saving modified things has nothing to do with the
window system. I would rather create a fresh module than have it there.

Re. #2: openide/fs already includes all kinds of APIs that are even more visual,
e.g. FileSystem.HtmlStatus. Anyway I'm not tied to project/queries, it was just
an idea.

Re. #5/#6: can you elaborate on how core/progress relates to this? I guess you
are talking about the list of pending tasks active when the app shuts down. That
should probably be its own related but separate API/SPI, I suppose.

Maybe we could have a new module openide/lifecycle covering:

- LifecycleManager (or a replacement)
- SavableRegistry etc.
- TaskRegistry etc. (for ExecutionEngine task, progress handles, ...)
- in the future, startup/shutdown state transitions and hooks, a la proposed
"Desktop" JSR

Now that I think about it a bit, openide/modules is sounding attractive for
this. Related to ModuleInstall.clos* methods, for example. What do you think
about putting this stuff into org.openide.modules.*?

If we want to put it to some existing module I think that openide/modules is the
best place from the proposed options because:

1. It already contains ModuleInstall.clos* and ModuleInstall.restored() which
are in fact also lifecycle management from module perspective

2. The modules API is kind of small

I personally prefer a new module since this functionality is not strictly
necessary when using just the module system. We would add unneccessary classes
for people that would be interested in pure module system. But my opinion is not
really strong in this respect, I will leave the decision to others (Jarda) ;-)

I like Jesse's idea and do not have strong opinion about right place where to
put this. project/queries can be OK as well as openide/modules.

If we want to fix this it would be good to get rid of code in projects/ui that
scans DO registry for modified files and adds DOs from opened TopComponents and
have only one impl of {Exit|Save}Dialog. Also profiler had problems with current
implementation - snapshot is not backed by DO until it is saved.

Jesse wrote:
>Maybe we could have a new module openide/lifecycle covering:
>
>- LifecycleManager (or a replacement)
>- SavableRegistry etc.
>- TaskRegistry etc. (for ExecutionEngine task, progress handles, ...)
>- in the future, startup/shutdown state transitions and hooks, a la proposed
>"Desktop" JSR
>
>Now that I think about it a bit, openide/modules is sounding attractive for
>this. Related to ModuleInstall.clos* methods, for example. What do you think
>about putting this stuff into org.openide.modules.*?

I would think LifecycleManager and this API would be in a module lifecycle as
that is what both things are suited for.  Saving is part of the lifecycle of
data in any many instances (sometimes you just have readyonly data).  It is
opened, it is edited, it is saved, possibly commited, and it is closed.  

Then in cases where someone might actually write a small application such as
some type of a simple monitoring application for instance where lifecycle
doesn't matter so much to them then they don't get extra classes.  All the
classes add up, and I am for being able to create as small applications as
possible, so I would think this module being able to be broken completely out of
the platform would make sense.  Allows for smaller footprints.

Just a side note on my thoughts on this (reason if you will): To me this is one
of the issues with the JRE/JDK currently.  Too many classes have made it into
the JVM which most are not used in many simple applications, so if one wants to
embed a JVM in a simple application their application is still at a minimum the
size the the full JRE.  I'm for having the JRE/JDK completely module based as
well where we can choose to not include different APIs we may never even touch.
 If the the env supported dependencies such as the NB module system does that
would be possible, and the meta-data could be inspected to tell us what we can
ommit.  Even for the native code.

That's my opinion on this.

See also suggested interface in

http://www.netbeans.org/nonav/issues/show_bug.cgi?id=146377#desc21

which adds isSavable (and a listener) so there is no need to add and remove a cookie from a DataObject or node
dynamically. That interface is missing display name / icon information which would be needed for use by the Exit dialog
and maybe also for status line updates of SaveAction.

Forthcoming org.netbeans.core.startup.ModuleLifecycleManager.saveAll could benefit. (It cannot access SaveCookie or
DataObject.Registry.)

Here is the proposed API and test to show how to use it:
http://hg.netbeans.org/core-main/rev/128b270ddffd
if there is a basic consensus that this can solve problems we have, I can implement the rest (save and save all actions and exit dialog).

Couple of comments.
It could have been simple proposal - just one factory method taking a callback with some extra details together with dispose/destroy(). Unfortunately it is not easy to see this pattern. It adds more than people need to see and seems little bit awkward to me.

RK1.Why do you introduce new interface and then tell us not to implement it? What does it mean for all existing implementations of SaveCookie?

RK2. Savables is not only utility class to create instance but also a registry. Can we split these two functions.

RK3. Savables.create - looks to me like a method to create mutable (CharSequence) instance of Savable interface without actually implementing the interface. Isn't it simpler to let people implement Savable as outlined by Jesse in comment #4? Let them handle object identity in equals().

These three are related and maybe originally suggested Savable can do. Additionally you can add something simpler to understand AbstractSavable with ctor
AbstractSavable(SavableRegistry r)
and 
public void save() th IOE { doSave(); r.unregister(); }
Because mix of identity object, Callback from concurrency utilities and CharSequences to avoid method returning String does not lead to readable code.

RK4. Why saveMessage? Tests do not cover how this should be used.

RK5. How does it work if save() throws an IOException (not covered by test)? I suppose it remains in registry and will still be shown in save on exit dialog.

RK6. What exactly is the rest - fix 'exit dialog' and 'save all' to operate on modified DataObjects (to preserve compatibility) and everything registered in this new support. Fix save action to enable saving of Savable instances. Is this correct?

RK7. Can you document destroy()? In javadoc and by test? Should client code call it? From saving callback? When undo invalidates savable? 

RK8. If you reject my previous comments and for some reason stick with factory method: can you get rid of identity? Think about client code that has to create identity object to call factory method and it can drop returned Savable immediately. Merge identity and Savable.

[JG01] RK2 is a good point; SavableRegistry might be a better name.


[JG02] Also agreed with RK3; this pattern with the opaque identity and the callback is rather confusing, with no compelling advantage. Would be more intuitive for people to supply an arbitrary implementation of Savable. For compatibility reasons SaveCookie could not extend an interface which required additional methods, but you could add a String getDisplayName() method in LabeledSavable extends Savable, and probably supply an abstract base class which could have a protected handleSave method and a cancel method. Then the registry would just have the obvious Set<Savable> methods: add, remove, get all.


[JG03] [for the record, though I expect you will reject it anyway] Still prefer to have a listenable boolean attribute indicating whether the object currently needs saving or not. (Makes SaveAction slightly more complicated, but this is just one piece of code, compared to the many providing Savable.) This would simplify usage from e.g. DataObject.lookup. Currently you need to use CookieSet methods to insert and remove a SaveCookie dynamically, which prevents a standard support class from implementing all of the logic itself. SimpleES is able to do so only by requiring a reference to the original CookieSet, granting a powerful capability that it ought not require (and forcing the DataObject to use CookieSet instead of a plain Lookups.fixed or similar).


[JG04] Placing the API in openide.awt prevents org.netbeans.core.startup.ModuleLifecycleManager.saveAll from making use of the registry. openide.modules would be a better location for this reason, despite the attraction of keeping "Something-able" interfaces in one package.


[JG05] To really evaluate, need to see how DataObject.Registry.getModifiedSet() can be deprecated. (JG02 would likely make this easier, since it could just rely on the real Savable registry to manage state, and filter the results to give only its private implementation type.)


[JG06] Refer to comment #14. Would be good for a proposed patch to at least attempt to simplify current code in projectui. And would be good to demonstrate that the new API finally makes it possible to create a persist-on-save editor component such as profiler snapshots, as this is a common request.

Thanks for your comments. I reflected the most important ones in
http://hg.netbeans.org/core-main/rev/5ffab5291dab
If the new API looks better, I'll proceed with its implementation and usage in here-in indicated places (DataObject.Registry, exit dialog, SaveAction, SaveAllAction, lifecycle-manager.saveAll).

Many things can have display names - it is a natural consequence of localization.  It makes no sense to me that DisplayName should extend Savable.

Also, wouldn't "Localized" be a more accurate name?

Probably this should have no connection to save behavior and sit next to NbBundle.  If possible backward-compatibly, Node should implement it.


Next...Why in <insert diety>'s name is equals() and hashCode() part of the signature of AbstractSavable?  

I seem to remember a certain Jarda Tulach looking at the signatures inside JavaMail in disbelief, in my hotel room in Portland, and spontaneously exclaiming "Wow!  Somebody was learning object oriented programming!" when seeing those same method signatures in JavaMail's (awful) API.  What could cause such a change of heart?


Documentation should explicitly state what are the consequences of calling register() or unregister() twice or out of sequence and those consequences should be enforced.  If garbage collection counts as a call to unregister(), that should also be documented.  There is actually an interesting, slightly-backwards yet elegant potential use of Java locks possible, if you allow register and unregister to have an argument of type Object, if you can see it - but it would be a little bit strange to most people.

Update. New diff available as
http://hg.netbeans.org/core-main/rev/23633ae6fad5

Build is available at
http://deadlock.netbeans.org/hudson/job/prototypes-savable-77210/

Javadoc is available at
http://deadlock.netbeans.org/hudson/job/prototypes-savable-77210/javadoc/


I have not implemented the "Exit & Save" dialog yet (due to unresolved issue with Savable.DisplayName), but otherwise I believe this is the API we were seeking for.

Please note the way "Save All" action is implemented now: it is plain "Save" action, but acting on different context...

Exit dialog is fixed now. I've resolved the problem with DisplayName by removing it and using toString() instead (I'd like to create the DisplayName interface, but it is too Looks-like and I don't feel I want to go that path now):
http://hg.netbeans.org/core-main/rev/3133a04929e6

I've solved another problem with Icon as well:
http://hg.netbeans.org/core-main/rev/1694762d50b2

I don't want to integrate this change for 7.0.1 (just for 7.1), but as soon as it is know which branch goes where, I feel it is time to integrate this long awaiting change.

Created attachment 107152 [details]
Sample project in Mercurial bundle format

*** Bug 40427 has been marked as a duplicate of this bug. ***

Time to integrate. Please finish last round of review.

Savable.REGISTRY is a Lookup which (presumably) only will ever contain objects of type Savable.  This seems like an abuse of Lookup.  Wouldn't Lookup.Result<Savable> make more sense?

(In reply to comment #28)
> Savable.REGISTRY is a Lookup which (presumably) only will ever contain objects
> of type Savable.  

Right.

> Wouldn't
> Lookup.Result<Savable> make more sense?

Many APIs accept Lookup. Like createContextAwareAction(Lookup). Replacing Lookup with Lookup.Result would complicate reuse in such situations. I'd rather keep it.

Looks accepted, I'll integrate tommorow.

Merged as ergonomics#6cd9c57bddfa

(In reply to comment #20)
> [JG04] Placing the API in openide.awt prevents
> org.netbeans.core.startup.ModuleLifecycleManager.saveAll from making use of the
> registry.

I guess this was ignored, so MLM.saveAll will continue to be broken. Please at least update the comment in that method explaining why it cannot be fixed (without using reflection).


[JG07] Asking a Savable to implement Icon and then delegate all methods is awkward. Would be better to just have a method @CheckForNull Icon getIcon().


Check spec versions. openide.awt/manifest.mf says 7.33 yet sources use @since 7.31 and similar in apichanges.xml and some deps. Ditto openide.nodes 7.21 vs. 7.23. Just grep the diff for 7.21 and 7.31.

Created attachment 108461 [details]
LifecycleManager patch, OK?

I can fix JG04 by herein attached patch, I think.

Thanks for the 7.31 vs. 7.33 observation. Fixed.

Re. JG07. I cannot add method to Savable, as SaveCookie extends it. I want to leave the contract as it is now.

(In reply to comment #33)
> LifecycleManager patch, OK?

No, I think this is dangerous. A prioritized LM impl should be solely responsible for all methods. NbLifecycleManager.saveAll has to be in control.

> Re. JG07. I cannot add method to Savable, as SaveCookie extends it.

I see.