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 37779 - Compatibility between Node.Property <-> PropertyModel
Summary: Compatibility between Node.Property <-> PropertyModel
Status: VERIFIED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Explorer (show other bugs)
Version: 3.x
Hardware: PC Linux
: P1 blocker (vote)
Assignee: _ tboudreau
URL:
Keywords:
Depends on:
Blocks: 37626
  Show dependency tree
 
Reported: 2003-12-05 10:28 UTC by Jaroslav Tulach
Modified: 2008-12-22 17:56 UTC (History)
0 users

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Scatch of compatibility test (4.90 KB, patch)
2003-12-05 10:29 UTC, Jaroslav Tulach
Details | Diff
Diff with javadoc expanded to explain PropertyModel/Property relationship and situation wrt firing changes (5.23 KB, patch)
2003-12-05 18:14 UTC, _ tboudreau
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2003-12-05 10:28:29 UTC
During review of issue 37626 I was desperatelly
searching to find out how the compatibility
between old (using PropertyModel) and new (using
Node.Property) is fullfilled. I have not found
anything that would relate that and that is why I
wrote a basic test (will attach) to check it. I
have found that the direction from PropertyModel
-> Node.Property is probably fine (because the
code inside is build around Node.Property), but
the opposite does not work. 

Please make it work, include my test into some
testsuite and enhance it to where necessary for
example to test ExPropertyModel. I could not test
much, as the functionality is really missing.

Btw. I would also like to know why ExPropertyModel
and DefaultPropertyModels are deprecated and
PropertyModel is not?
Comment 1 Jaroslav Tulach 2003-12-05 10:29:09 UTC
Created attachment 12432 [details]
Scatch of compatibility test
Comment 2 _ tboudreau 2003-12-05 12:49:12 UTC
Jarda, no, the test you wrote cannot work - how could it?  Consider:

 - You create a PropertyPanel using Node.Property
     - Node.Property has no notion of listening, and no way to access
       the Node it belongs to, to attach listeners
 - Then you call PropertyPanel.getModel() and start listening on a 
     generated property model that wraps the original Node.Property.  

How should I detect a change in the Node.Property when I don't know
the Node it belongs to?  I could fire changes *if* they are changes
made by the property panel, but there's no way to detect changes done
from outside.

I would prefer to document that if you use setProperty or one of the
Property constructors, that the model received from
PropertyPanel.getModel will not fire changes.  That would also be
compatible.  After all, if you are passing a Property object, you also
know where you got it.  If you want to listen to it, you can, and tell
the property panel to refresh itself.  Most of the time that's not
what anybody wants.  For the second case, that is...

the reason PropertyPanel is not deprecated but others are:
 - There is a valid use case for PropertyModel, where someone wants to
write an implementation it will fire changes the property panel should
detect.  That is the only remaining use case.
 - There is no valid use case for DefaultPropertyModel - if you are
using DefaultPropertyModel, then you know the object you are creating
a model for - you need it for the constructor.  Use BeanNode if you
want changes, or PropertySupport.Reflection by itself if you don't
expect the value to change on its own.
 - ExPropertyModel exists to support a similar use case - BeanNode can
be substituted more effectively.

Part of the point of the two deprecations is that UI components whose
values change while the UI is on screen due to events beyond their
control is usually *bad* UI.  This kind of magic should be
discouraged, and the person writing such a UI should be encouraged to
take responsibility (as in, listen to the BeanNode themselves if
unexpected changes are interesting and likely to happen) or don't if
its not wanted.

PropertyModel is not deprecated, because as Jesse pointed out on
nbdev, there are cases where you have a UI with a property that is
composed from other properties in the UI and should change to reflect
them.  It is right for someone to write an implementation of
PropertyModel to automate that, and that is a case of the developer
taking responsibility for the fact that changes will happen to the
property displayed due to events outside the property panel's control.

The point is that I do not want a property panel that assumes the
common case is a UI where unexpected changes will happen and should
magically updated in the UI, I want a property panel that assumes that
the common case is the opposite - that *is* the common case in UI
design.  If that kind of magic is wanted, that is a use case for
PropertyModel, and should be an explicit design decision of the
developer, so they have to know what they're asking for and take
responsibility for its behavior.


So I propose instead to document that if you use Node.Property to
instantiate a PropertyPanel, or use setProperty to set its model, that
the PropertyModel instance you get from getModel will not fire
changes;  that if you want the PropertyPanel to automatically reflect
changes made from outside, you should use PropertyModel.  Since the
Node.Property methods did not exist before the change, it should be
compatible.

Longer term, a more consistent approach would be to do as follows:
 - Deprecate PropertyModel too
 - Produce a new Property interface to retro-implement on 
   Node.Property
 - Produce a subinterface Property.Mutable with listener methods for 
   such cases.  Or use something like Observable.

That work should be part of repackaging property sheet/openide
separation II.
Comment 3 _ tboudreau 2003-12-05 13:01:56 UTC
BTW, if you are interested in what I would propose as a replacement
for PropertyPanel, have a look at the package-private interface
PropertyDisplayer in the revised package.  It is implementations of
these that the new PropertyPanel embeds.  
Comment 4 _ tboudreau 2003-12-05 18:13:27 UTC
Note there was only one constructor in the old PropertyPanel which
would take a Node.Property instance, and it was package-private -
presumably it was used by the old property sheet:

/** Uses an instance of NodePropertyModel as model.*/
PropertyPanel(Node.Property p, Object []beans) {
    this(new NodePropertyModel(p, beans), 0);
}

Therefore, it is a compatible change if listening on the generated
PropertyModel from PropertyPanel.getModel() does not fire changes that
originate in the Property instance driving the PropertyPanel.  I have
modified the documentation on the branch to include that information
and will attach a diff.
Comment 5 _ tboudreau 2003-12-05 18:14:28 UTC
Created attachment 12442 [details]
Diff with javadoc expanded to explain PropertyModel/Property relationship and situation wrt firing changes
Comment 6 _ tboudreau 2003-12-05 22:38:00 UTC
Marking this issue as fixed - the documentation changes satisfy the
requirements for backward compatibility.
Comment 7 Jaroslav Tulach 2003-12-08 08:56:44 UTC
Goal of this request is to ensure that the compatibility will be
tested, the attached test is just an example how it could be done.
Your point about listener not working is accepted, ok, remove the
listener code from the test. 

"So I propose instead to document that if you use Node.Property to
instantiate a PropertyPanel, or use setProperty to set its model, that
the PropertyModel instance you get from getModel will not fire
changes"

ok, but at least execute the test to find out that if you use
Node.Property constructor, getModel returns null and fix it. So the
minimal request is 1. add some test based on hereattached example into
testsuite 2. make it work. The real request is 3. write a better test
to ensure, that we minimize the amount of unimplemented features.
Comment 8 _ tboudreau 2003-12-08 16:50:05 UTC
Sure, give it a shot.  It requires, though, that the client (TaskList,
whatever) know when it's going to start, and finish fiddling with
nodes,  grab the tree lock before its starts, and release it when it's
done.  So something on the client end still is needed.
Comment 9 _ tboudreau 2003-12-08 18:25:39 UTC
I've implemented generating a PropertyModel on demand, and integrated
your compatibility test (removing Listener.assertChanged() as agreed).

PropertyPanelTest.java 1.4.6.3
Comment 10 Jaroslav Tulach 2003-12-12 10:07:49 UTC
Good.