Created attachment 19067 [details]
new isDefaultValue() implementation

Sorry. Mistyped the issue number to be fixed. The right one is 45871.

I think this change is unnecessary. If you override
supportsDefaultValue() to return true, of course you have to also
override isDefaultValue() to return something meaningful. The default
return value of isDefaultValue() is completely irrelevant because it
should never be called unless it is also overridden. In fact I would
support having it be

public boolean isDefaultValue() {
    if (supportsDefaultValue()) {
        throw new IllegalStateException("You must override the
isDefaultValue method!");
    } else {
        // Should never get here if using default property sheet.
        // Irrelevant what this is.
        return false;
    }
}

You proposal implementation is ok for modules which have been
developed after adding isDefaultValue() method. But the main reason to
make this change is to be "compatible" with old modules. Authors of
these modules didn't have to be aware of isDefaultValue() because such
method didn't exist when they were writing theirs Node.Property
implementations (as it was said in issue 45871 discussion). I'm not
able to estimate how much of such modules could exist (and are
actively used) today and how much we should help them with this
compatibility. So if you still insist on you proposal and nobody else
will add an opinion I'll go your way. Thanks for opinion.

I don't understand why you don't simply disable the Restore Default
Value button when !supportsDefaultValue() - in such cases the property
does not understand default values so clearly the button should be
disabled, right? Why are you even calling isDefaultValue() in such cases?

The exact behaviour as you described is already in trunk in
PropertySheet.java:

FeatureDescriptor fd = table.getSelection();
defaultValueItem.setEnabled(fd != null &&
        fd instanceof Node.Property &&
        ((Node.Property) fd).supportsDefaultValue());

But if we want to achieve the behaviour demanded by the 45871 issue we
need to change the code to:

FeatureDescriptor fd = table.getSelection();
defaultValueItem.setEnabled(fd != null &&
        fd instanceof Node.Property &&
        ((Node.Property) fd).supportsDefaultValue() &&
        !((Node.Property) fd).isDefaultValue());

so the "Restore To Default" will be enabled ONLY when the property
supports defaultValue but currently doesn't contain defaultValue
value, otherwise not.

And as I described above the only problem could be with "bad"
Node.Property implementations - which can be case with old ones
because... described above.

I hope it's clear now :)

PS: code above is for RDV popup item (the same would applies to RDV
property editor button)

"And as I described above the only problem could be with "bad"
Node.Property implementations - which can be case with old ones
because... described above." - sorry, what? If you don't override
supportsDefaultValue(), then the button is already disabled, fine. If
you do, then you have to override isDefaultValue(), in which case the
button will be enabled or not as appropriate, fine. And if you
overrode only supportsDefaultValue() for some bizarre reason, then
your N.P code is obviously in error, so why don't you just fix it?
What problem are you trying to solve here?

Obviously defaultValueItem.setEnabled needs to check both sDV() and
iDV(), so if it doesn't currently, then clearly it needs to be fixed.
But that is just a bugfix, not an API change; the default return value
is iDV() is irrelevant because it will only be called in case sDV().

Here is explanation for "if you overrode only supportsDefaultValue()
for some bizarre reason"
- supportsDefaultValue() is there since version 1.0
- isDefaultValue() is there since version 3.19
so the bizarre reason is that somebody wrote a module before version
3.19 and wrote it correctly - e.g. did not override isDefaultValue, as
there was no such method - and the module worked correctly, the action
SetDefaultValue was enabled. What Martin wants to do right now, is to
enable/disable the state of the action based on value of
isDefaultValue. But that means it will break modules written before
version 3.19, as they will always return isDefaultValue true and the
action will be disabled, which breaks the contract they were written
against.

Thanks for the help Jarda. This is exactly the point. So we can:

1) Fixed the PropertySheet.java with:

 FeatureDescriptor fd = table.getSelection();
 defaultValueItem.setEnabled(fd != null &&
         fd instanceof Node.Property &&
         ((Node.Property) fd).supportsDefaultValue() &&
+        !((Node.Property) fd).isDefaultValue());

and don't touch the current API which could hurts those old modules.

2) Change the API that N.P.iDV() will return false and fix the
PropertySheet.java as in first case. So for old modules which overrode
supportsDV but don't override iDV the button will be always enabled -
which is better than always disabled

3) won't fix the issue 45871

Neither is perfrect and this was the reason why I set up this issue. I
don't know if there are enough of such modules. Thanks again Jarda and
sorry for confusion Jesse :)

Yarda's explanation makes sense (nothing in this issue or #45871
previously mentioned that isDefaultValue() was added after
supportsDefaultValue()), so I guess #2 is the only option.

How is it that this problem was missed when the isDefaultValue()
method was introduced?? Probably that method was not designed
correctly; should have been

/** return true, false, or null if unknown */
public Boolean isDefaultValue() {
    return supportsDefaultValue() ? Boolean.TRUE : null;
}

with supportsDefaultValue deprecated.

"nothing in this issue or #45871 previously mentioned that
isDefaultValue() was added after supportsDefaultValue()"

I tried to mention it in the original comment with

"....But this is problem for older modules because the
isDefaultValue() method was added after the supportsDefaultValue()
method....."

But probably other explanation was somehow confusing and Jarda's one
is surely the first one which makes clear sense. Doesn't matter now :) 

"...so I guess #2 is the only option..."

Ok, I will do so next week with leaving note in the api-changes. BTW I
found the following there (added by dstrupl):

"Method public boolean isDefaultValue() has been added to class
Node.Property. The idea behind this is to visually mark modified
properties in the property sheet. If the method returns false it means
that the value has been modified by the user and visual feedback will
be shown. The reason why the default impl is returning true is to make
the old properties (properties using previous version of the API) look
the same as they did before the change."

I'm not sure if the property sheet is implemented in the way the
comment describes - probably not. I took a look at the Form Editor
module which provides such "marking" and it seems they handle the
marking in their own way. I didn't found the iDV() method usage at any
other place. I will communicate the exact wanted behaviour with HIE.
And it is probably off-topic for this thread.

Thanks for opinions.

So I assume it's evaluated now and the target milestone can be changed
to 4.1. Correct?

Whoah, careful. dstrupl's explanation of the current behavior makes
sense - if you have some UI which is supposed to display whether the
current value is the default or not, then of course you want the
default value of iDV to be true, since old properties should not have
this marking (should not be boldfaced). Perhaps any such UI should be
disabled (never boldface) in case supportsDefaultValue() is false, and
we break compatibility for properties which overrode sDV but not iDV.

Given this new information, I would be more inclined to do #3 - i.e.,
nothing; old properties have to be fixed. (I consider the effects of
your suggested fix to be perhaps worse than the current situation.)
Having a sDV method with no iDV method simply did not make much sense,
and any properties which override sDV but not iDV cannot be expected
to work properly in general and will need to be rewritten. We can at
least do a runtime check: make the default impl of iDV be

private static final Set/*<String>*/ warnedNames = new HashSet();
public boolean isDefaultValue() {
    String name = getClass().getName();
    if (supportsDefaultValue() && warnedNames.add(name)) {
        ErrorManager.getDefault().log(ErrorManager.WARNING, "Class " +
name + " must override isDefaultValue() since it overrides
supportsDefaultValue() to be true");
    }
    return false; // or whatever
}

and use Find Usages to fix all of them that we know of.

"Given this new information, I would be more inclined to do #3 - i.e.,
nothing; ........We can at least do a runtime check: make the default
impl of iDV be ........."

Ok, I will use your proposed iDV implementation which is nice since it
will warn us once per every "bizzare" N.P. impl. per IDE session and I
will change the resolution of 45871 issue to LATER or REMIND with
Target M. to future. Although if we use "return false; // or whatever"
we can fix the 45871 issue for all our modules (however not fully for
external modules) - so it could be probably marked as fixed.

We don't use resolutions of LATER or REMIND - either it is open, or it
is FIXED / WONTFIX / INVALID / WORKSFORME.

Loggin a warning seems to me like a good idea. But just to be sure we
know what we want, here is a bit of evalution from a module writer
point of view. We have modules:

#1 - does not override supportsDefaultValue()
#2 - old module written without knowledge of isDefaultValue
#3 - newer module overwriting just supportsDefaultValue and relying on
default
#4 - new module that overrides both methods (otherwise there would be
the warning)

We want following behaviours (correct me if I am wrong). Btw. I am
asumming that there is a request to mark modified properties in sheet
that is going to be implemented someday as well):

#1 - no set default value action, no indication in property sheet that
the value is modified, no warnings (if isDefaultValue is not called
directly by someone else than the infrastructure)
#2 - set default value action enabled, properties are marked as
unmodified, warning printed (this is what we want, let's make old
modules work unchanged and warn them about necessary changes)
#3 - same as #2 (but this case is broken since the start,
isDefaultValue should have been overriden in first)
#4 - set default value always enabled, properties marked as
appropriate, no warning printed

If this is acceptable state, I believe issue 49031 should be WONTFIX.
If someone however insists on changing:

#4 - set default value action enabled and properties marked as
appropriate, no warning printed

we can simulate the Jesse's isDefaultValue method returning
Boolean[true,false,null] by using reflection to find out from the
explorer whether the method is Node.Property.isDefaultValue() is
overriden or not. If it is then call it, otherwise just leave the
action enabled. Just write a test for this fragile behaviour and make
sure it is documented in arch*explorer.xml and arch*nodes.xml.

Yes, nice summary.

Thanks for the review. I will go the last way proposed by Yarda (using
reflection). I will post a patch here when it is done.

Hello again.

1) I reimplemented Node.Property.isDefaultValue() as was proposed by
Jesse.
2) PropertySheet will use the reflection to decide whethere RDV
(RestoreDefaultValue action) should be enabled or not as proposed by
Yarda. Implemented in new (package-private) method in
PropUtils.shallBeRDVEnabled()
3) There are tests for all of this  - new NodeProperty51907Test.java
to tests if Warning is (not) logged appropriately and
PropUtilsTest.testRestoreDefaultValueBehaviour() to test if reflection
in shallBeRDVEnabled() works correctly in misc. cases

I will commit attached patches (follows) in a few days this week if
there are no any complains.

Created attachment 19179 [details]
NodeProperty51907Test warning test

Created attachment 19180 [details]
51907-patch.diff

Created attachment 19205 [details]
arch diff

Closing issues, thanks for the review.

Changes performed:

Checking in openide/src/org/openide/nodes/Node.java;
/cvs/openide/src/org/openide/nodes/Node.java,v  <--  Node.java
new revision: 1.90; previous revision: 1.89
done
Checking in openide/src/org/openide/explorer/propertysheet/PropUtils.java;
/cvs/openide/src/org/openide/explorer/propertysheet/PropUtils.java,v 
<--  PropUtils.java
new revision: 1.43; previous revision: 1.42
done
RCS file:
/cvs/openide/test/unit/src/org/openide/nodes/NodeProperty51907Test.java,v
done
Checking in
openide/test/unit/src/org/openide/nodes/NodeProperty51907Test.java;
/cvs/openide/test/unit/src/org/openide/nodes/NodeProperty51907Test.java,v
 <--  NodeProperty51907Test.java
initial revision: 1.1
done
Checking in openide/arch/arch-openide-propertysheet.xml;
/cvs/openide/arch/arch-openide-propertysheet.xml,v  <-- 
arch-openide-propertysheet.xml
new revision: 1.21; previous revision: 1.20
done
Checking in
openide/test/unit/src/org/openide/explorer/propertysheet/PropUtilsTest.java;
/cvs/openide/test/unit/src/org/openide/explorer/propertysheet/PropUtilsTest.java,v
 <--  PropUtilsTest.java
new revision: 1.2; previous revision: 1.1
done

Verified.