Created attachment 69208 [details]
Proposed patch (missing apichanges)

Please review this patch. Should simplify writing of data objects.

Y01 This is an incompatible change with huge impact for already existing code. The benefit includes three lines 
shorter code in the generated template (which can easily be achieved in different and compatible way). The negatives 
are innumerable and unknown in advance. If your only goal is to simplify the template, then add new constructor (or 
choose similar compile time trick) to allow people consciously decide that they want the new behaviour. Such compile 
time change would imho qualify for api review fast. However I consider current patch non-compatible, controversial, 
and not suitable for fast review. If you insist on it, let's have standard review next week. If you prepare different 
patch, feel free to re-ask for fast review.

> The benefit includes three lines shorter code in the generated template

Actually, the benefit is that a lot of our users don't have to wonder why all their DataObject's need three lines of
boilerplate code. Most Java developers would expect that kind of code to be in a superclass.

> The negatives are innumerable and unknown in advance.

http://en.wikipedia.org/wiki/Fear,_uncertainty_and_doubt

Would you please enumerate at least the first few of these negatives and elaborate about them?

I personally quite like this change - certainly better than forcing everyone to adjust their DOs (by overriding
getLookup method [wasn't its addition also an incompatible change?]).

I don't insist on anything. But the change would mean that getLookup would work correctly for nearly all DOs by default;
currently it produces the correct contents but goes through getNodeDelegate which is too dangerous to leave enabled in
production code. The simplification of newly generated DO subclasses is nice but a secondary benefit; the main purpose
is to avoid going through gND for old DO subclasses.

Just for reference, please see issue 62707 for background on the original API change. 

"the main purpose is to avoid going through gND for old DO subclasses" - this is something that will have been 
addressed by two approaches:
- a warning is printed to explain why the code shall be upgraded
- we shall fix all data objects we have under our control, e.g. in the NetBeans IDE

> > It is heavily incompatible change.
> To whom? Just in principle, or do you have specific examples in mind?

For everyone who overrides DataObject.getCookie. As far as my could investigate this used to be very common technique 
and it is the main reason which motivated the decision made in issue 62707.

Keep in mind that no new code created for NetBeans 6.0 and later suffers from any of "the main purpose" problems.

If you think there are likely to be subclasses overriding getCookie rather than using getCookieSet and which have not
already overridden getLookup then we cannot do this.

Yes, overriding getCookie was quite common in the days before 6.0, thanks for your understanding.

So the only thing that is left is the task of "beautifying" the code generated by our File Type wizard. I've already 
talked about adding new constructor option. There is another possible approach: we can create "MultiDataObject2 
extends MultiDataObject" class with correctly overriden getLookup method and change our File Type wizard to extend 
this new class. I do not consider the "beautification" important enough to justify the new class, but as this approach 
is fully backward compatible, I am not going to block anyone to do it.

MDO2 sounds confusing, and a new constructor option would not work very well - you would have to pass some dummy
parameter like int _ = 0, which would just be confusing.

> So the only thing that is left is the task of "beautifying" the code generated by our File Type wizard. 

I actually think that striving for straightforward and understandable code is important. You could just as well argue
that EJB 2.1 is just fine and we don't need EJB 3, "because there are already tools such as NetBeans and XDoclet that
will generate correct EJB 2.1 code for you". But reading code is just as important as writing it, which is why EJB 3.0
(where 1-2 classes are equivalent to 4-5 classes in EJB 2.1) is so much better than EJB 2.1. So this is not just the
issue of beauty, it is a very practical and real problem that needs attention.

Yes, sir, those are exactly my words. In case you ever need short way to express that, just call it "beautifying". 

Btw. instead of MDO2 you could use BaseDataObject...

"BaseDataObject" (or, more consistently, "AbstractDataObject") would be OK except that it is not clearly associated with
MultiFileLoader, which you will need to accept in its constructor and pass to super (even if using
DataLoaderPool.factory). I'm not sure if introducing a new base class just for this purpose is worth it, but perhaps.

>For everyone who overrides DataObject.getCookie. As far as my could investigate this used to be very common technique 
>and it is the main reason which motivated the decision made in issue 62707.

IMHO any reference to "COOKIE" shall have been gone since like 5.0.. When exactly was it superceded and rendered
obsolete by Lookup? 3.3? 

This just shows that the trash pile is higher than we can actually see..

Unfortunately DO.lookup was only introduced in 6.0.

Also note that Cookie-related methods have never been formally deprecated. Lookup-related methods were introduced and
their use encouraged, but that is it.

Yes, I know it's not been deprecated. I believe that was a mistake that needs to be corrected and going forward we need
to have an "end of life" policy to strike a balance between backward compatibility and usability of APIs.

Created attachment 69748 [details]
DO's overriding getCookie()

The attached diff contains the changed needed to get the full distribution to build after making DO.getCookie() final.
It shows that only 20 of the 109 files named *DataObject.java in main override this method.

Not sure yet how to interpret this result. But I definitely agree with Milos in that the current state is not right. We
should not deprecate a whole concept (moreover, in the fuzzy manner that was used to deprecate cookies -- why is
Node.Cookie not marked as deprecated?), while still happily using it in our own (albeit old) code.

Now that we have DO.getLookup() I think it would make sense to move towards the state where the DO lookup is the main
source of the capabilities of both the DO and its node. The current state must be quite chaotic for a new user: the main
source of capabilities is either the node delegate (in the default implementation) or the CookieSet (a deprecated
concept). Not to mention that the recommended way to put things in the DO lookup is AFAIK CookieSet.assign()!

Agreed, the current state is a mess. We should not suggest that newly written (Multi)DataObject's override getLookup to
use a CookieSet when we do not want them to use "cookies" at all. Probably the better suggestion would be

public @Override Lookup getLookup() {
  return Lookup.EMPTY; // Lookups.fixed(...);
}

but then we also have templates for DataEditorSupport which expect a CookieSet to insert/remove a SaveCookie. We should
also deprecate SaveCookie, as I have long suggested, and replace with

public interface Savable {
  boolean isSavable();
  /** should be a no-op in case isSavable currently false */
  void save() throws IOException;
  void addChangeListener(ChangeListener l);
  void removeChangeListener(ChangeListener l);
}

so that you do not need to be constantly inserting and deleting a cookie every time the file is modified or saved.
DataEditorSupport could implement Savable directly, or offer a Savable instance from a method. For compatibility we
could have a new method somewhere:

public static Savable findSavable(Lookup l) {
  Savable s = l.lookup(Savable.class);
  if (s != null) {
    return s;
  } else {
    return new SavableBridge(l);
  }
}
private static class SavableBridge implements Savable, LookupListener {
  private final LookupResult<SaveCookie> r;
  private final ChangeSupport cs = new ChangeSupport(this);
  SavableBridge(Lookup l) {
    r = l.lookupResult(SaveCookie.class);
    r.addLookupListener(this);
  }
  public boolean isSavable() {
    return r.allInstances().iterator().hasNext();
  }
  public void save() throws IOException {
    SaveCookie c = r.allInstances().iterator().next();
    if (c != null) {
      c.save();
    }
  }
  public void addChangeListener(ChangeListener l) {
    cs.addChangeListener(l);
  }
  public void removeChangeListener(ChangeListener l) {
    cs.removeChangeListener(l);
  }
  public void resultChanged(LookupEvent ev) {
    cs.fireChange();
  }
}

and of course SaveAction would need to be modified to use findSavable.

Regarding SaveCookie vs. Savable, see issue #77210.