This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 36916 - PersistenceType should be better documented, preferably API method
Summary: PersistenceType should be better documented, preferably API method
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Window System (show other bugs)
Version: 3.x
Hardware: All All
: P2 blocker (vote)
Assignee: mslama
URL:
Keywords: API, API_REVIEW_FAST
Depends on:
Blocks:
 
Reported: 2003-10-29 20:23 UTC by Jesse Glick
Modified: 2008-12-22 15:54 UTC (History)
5 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
Diff for this issue (2.84 KB, patch)
2003-12-15 16:03 UTC, mslama
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2003-10-29 20:23:50 UTC
See T10 & Y06 in issue #36122 for background.

If the 'PersistenceType' client property on
TopComponent is left as is, it should at least be
listed in the arch description as stability level
'devel' or stronger (not 'private'), and also
mentioned in the TopComponent class Javadoc.

Consider however making it a method in
TopComponent, e.g.

pu st fi int PERSISTENCE_NEVER = 0;
pu st fi int PERSISTENCE_ONLY_OPENED = 1;
pu st fi int PERSISTENCE_ALWAYS = 2;
/**
 * Declare when this TC should be stored to disk.
 * Return value should be constant
 * over a given TC's lifetime.
 * @return one of P_X constants
 */
public int getPersistenceType() {
    // first check for 'PersistenceType' client
    // prop for compat
    // else default to P_ALWAYS?
}

Ideally (IMHO) the default should be P_O_O (to
make it easiest to write code that does not leave
junk settings files in the user dir) but that may
cause compatibility problems for old modules
expecting even closed components to be persisted.
TBD whether that is really a serious enough
incompatibility to prevent the default from being
changed.

Should also document that for P_N, you need not
override read/writeExternal.
Comment 1 Jesse Glick 2003-10-29 20:24:12 UTC
Probably suitable for fast-track review.
Comment 2 Peter Zavadsky 2003-12-04 15:24:03 UTC
How is it with this?... is it really necessary now? Marek, it's your
area, passing to you so I don't forget about this.
Comment 3 mslama 2003-12-05 14:28:02 UTC
What do other think about it? Shall we start FASTREC for this?
Regarding default value I think it should stay unchanged ie. P_A.
settings files are deleted anyway when they are not used ie.
referenced from wstcref or wstcgrp. (So TC must be member of some
group or be docked opened or closed to some mode to survive in winsys.)
Comment 4 Tomas Pavek 2003-12-05 15:46:49 UTC
I think this should be done, fast-track review seems to be quite
appropriate. You may assign the issue to apireviews@netbeans.org and
add API_REVIEW_FAST keyword.
Comment 5 mslama 2003-12-15 16:03:46 UTC
Created attachment 12584 [details]
Diff for this issue
Comment 6 mslama 2003-12-15 16:05:55 UTC
Diff is attached. To keep backward compatibility with client property
I chose PERSISTENCE_ALWAYS as default persistence type.
Comment 7 mslama 2004-01-06 09:47:51 UTC
Going to commit given diff tomorrow if there are no objections.
Comment 8 Jesse Glick 2004-01-06 16:29:52 UTC
Need also to patch
openide/arch/arch-openide-windowsystem.xml#exec-component,
openide/api/doc/changes/apichanges.xml,
openide/openide-spec-vers.properties, perhaps
openide/api/doc/org/openide/doc-files/upgrade.html. Don't forget
@since in patch.

Also consider printing a brief runtime warning if the method is not
overridden to encourage people to specify the behavior using the
method; e.g.

private static final Set/*<Class>*/ warnedClasses = new WeakSet();
public int getPersistenceType() {
    if (warnedClasses.add(getClass())) {
        ErrorManager.getDefault().log(ErrorManager.WARNING, "Note - "
+ getClass().getName() + " ought to override getPersistenceType()
rather than using the client property or accepting the default.");
    }
    // as before...
}
Comment 9 mslama 2004-01-07 14:48:31 UTC
I would print warning only if client property PersistenceType is not
null. I think using default behaviour of TopComponent (not overriding
getPersistenceType) is ok.
Comment 10 Jesse Glick 2004-01-07 16:58:37 UTC
Re. the previous comment: Yes, that's OK; I only suggested keeping the
warning even for those TC subclasses which do nothing special, because
the default value (ALWAYS) is a somewhat dangerous default (even if
necessary for backwards compatibility). You need to do plenty of
special work to make persistence really work correctly, which many
TC's do not need at all (NEVER is fine); and WHEN_OPEN is usually a
better choice than ALWAYS since ALWAYS contributes to pollution of the
user directory and is rarely of any value to the user. Probably many
existing TC's (esp. in third-party modules) are using ALWAYS simply
because the authors were not aware of any other possibilities. Keeping
the warning on unless the method is overridden simply ensures that
people who really wanted ALWAYS will show that explicitly in their code.
Comment 11 mslama 2004-01-08 15:34:19 UTC
Ok. I did it as Jesse suggested. Only I do not print warning when it
is instance of TopComponent not subclass.

I also updated documentation.
Comment 12 Jesse Glick 2004-01-08 15:41:11 UTC
Looks great, thanks.