Hi, Alexander.
Looks like this code in StyleContext:
903 if (attr instanceof SmallAttributeSet) {
904 return attr == this;
905 }
is unnecessary since you add the same chech in:
SwingUtilities2.isAttributeSetEqual
22.11.2012 19:28, Alexander Scherbatiy wrote:
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8001423/webrev.02/
- The isAttributeSetEqual method that compares two attribute sets is
added to the SwingUtilities2 class
- The isDefined() method is used to know that an attribute is local
- The attributes from the sets are compated with the equals
method. That leads that parents are also compared by the equals
method. The same was before the fix.
Is it possible to add some comments to the method about this?
Thanks.
- All asymmetrically redefined equals and isEqual methods now call
SwingUtilities2.isAttributeSetEqual() method.
Thanks,
Alexandr.
On 11/15/2012 2:01 PM, Alexander Scherbatiy wrote:
On 11/14/2012 10:31 PM, Denis S. Fokin wrote:
Hi Alexander,
my thoughts are below
1. I see the next scenario
// 'this' contains 3 attributes
// 'attr' does not contain attributes
In this scenario the code below is unreachable because of the
previous check:
if (getAttributeCount() != attr.getAttributeCount()) {
return false;
}
925 boolean result = true;
926
927 Enumeration names = attr.getAttributeNames();
// 'names' is an empty enumeration
// by this reason we do not get into the loop
928 while (result && names.hasMoreElements()) {
929 Object name = names.nextElement();
930 result =
attr.getAttribute(name).equals(getLocalAttribute(name));
931 }
// three attributes is not the same as no attributes
// but we have not changed the default value of the
// result variable, so return true.
932
933 return result;
2. You have removed the next code form the SimpleAttributeSet.java file
305 if (this == obj) {
306 return true;
307 }
I suggest the next checks in the beginning of each equals method
I have removed this check because the equals method calls
isEqual() method that has this check.
Using this check in both methods equals and isEqual is code
duplication.
a.) return false if the parameter is null
According to the implementation before the fix it should throw
NPE. This is a correct behavior. I do not think that it should be
changed.
b.) return false if a class of the parameter differs from expected
one (in general case is not an instance of AttributeSet)
Yes. This check is always implemented in the equals method.
c.) return true if the objects are equal by reference
This is implemented in all overridden equals and isEqual method
(directly or from the isEqual call from equals).
4.
By contrast with StyleContext.SmallAttributeSet.equals() we do not
change anything in StyleContext.NamedStyle.equals()
StyleContext.java
1431 /**
1432 * Checks whether two attribute sets are equal.
1433 *
1434 * @param attr the attribute set to check against
1435 * @return true if the same
1436 * @see AttributeSet#isEqual
1437 */
1438 public boolean isEqual(AttributeSet attr) {
1439 return attributes.isEqual(attr);
1440 }
It was decided by design that these classes should be compared
only by reference (so the equals method is not overridden). We do not
change this behavior in the current fix.
The current fix only makes 'equals' and 'isEqual' method
symmetrically for those cases there they were not symmetrically.
5.
In you current implementation we have a code duplication. This code
is common for StyleContext.SmallAttributeSet, SimpleAttributeSet,
MuxingAttributeSet (and may be for future NamedStyle).
921 if (getAttributeCount() != attr.getAttributeCount()) {
922 return false;
923 }
924
925 boolean result = true;
926
927 Enumeration names = attr.getAttributeNames();
928 while (result && names.hasMoreElements()) {
929 Object name = names.nextElement();
930 result =
attr.getAttribute(name).equals(getLocalAttribute(name));
931 }
932
933 return result;
May be we should somehow reuse our code here?
There is no a code duplication because every time we check local
attributes from the given set with the local attributes from the
current set.
Getting local attributes from the current set depends on the set
class implementation. For example, the SimpleAttributeSet contains
it's attributes in table and the SmallAttributeSet in an array.
So the only code duplication can be checking attribute counts. It
will be new public API that should be processed from the CCC request.
Is it really necessary?
6.
In the next places we have different approaches for the comparison
of attributes.
- comparation of local attributes (StyleContext)
- comparation of local attributes 'Approach #2' (MuxingAttributeSet)
- comparation of attributes
Is it intentional or it should be implemented the same way?
All these cases compares only local attributes.
The last case gets attributes from the table. The table
contains only local attributes.
Thanks,
Alexandr.
StyleContext.java
920 private boolean isAttributeSetEqual(AttributeSet attr) {
921 if (getAttributeCount() != attr.getAttributeCount()) {
922 return false;
923 }
924
925 boolean result = true;
926
927 Enumeration names = attr.getAttributeNames();
928 while (result && names.hasMoreElements()) {
929 Object name = names.nextElement();
930 result =
attr.getAttribute(name).equals(getLocalAttribute(name));
931 }
932
933 return result;
934 }
795 Object getLocalAttribute(Object nm) {
796 if (nm == StyleConstants.ResolveAttribute) {
797 return resolveParent;
798 }
799 Object[] tbl = attributes;
800 for (int i = 0; i < tbl.length; i += 2) {
801 if (nm.equals(tbl[i])) {
802 return tbl[i+1];
803 }
804 }
805 return null;
806 }
MuxingAttributeSet.java
167 public boolean isEqual(AttributeSet attr) {
168 if (this == attr) {
169 return true;
170 }
171
172 if (getAttributeCount() != attr.getAttributeCount()) {
173 return false;
174 }
175
176 boolean result = true;
177
178 Enumeration names = attr.getAttributeNames();
179 while (result && names.hasMoreElements()) {
180 Object name = names.nextElement();
181 Object localAttribute = getLocalAttribute(name);
182 result = localAttribute != null &&
attr.getAttribute(name).equals(localAttribute);
183 }
184
185 return result;
186 }
188 private Object getLocalAttribute(Object name) {
189 for (AttributeSet as : getAttributes()) {
190 Enumeration<?> names = as.getAttributeNames();
191 while (names.hasMoreElements()) {
192 Object attributeName = names.nextElement();
193 if (attributeName.equals(name)) {
194 return as.getAttribute(name);
195 }
196 }
197 }
198 return null;
199 }
SimpleAttributeSet.java
115 public boolean isEqual(AttributeSet attr) {
116 if (this == attr) {
117 return true;
118 }
119
120 if (getAttributeCount() != attr.getAttributeCount()) {
121 return false;
122 }
123
124 boolean result = true;
125
126 Enumeration names = attr.getAttributeNames();
127 while (result && names.hasMoreElements()) {
128 Object name = names.nextElement();
129 result =
attr.getAttribute(name).equals(table.get(name));
130 }
131
132 return result;
133 }
Thank you,
Denis.
On 11/12/2012 5:31 PM, Alexander Scherbatiy wrote:
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8001423/webrev.01/
See my comments below:
On 11/6/2012 6:26 PM, Denis S. Fokin wrote:
Hi Alexander,
I found several places that could be improved, in my opinion.
SimpleAttributeSet.java
120 boolean result = true;
I would not assign 'true' as a default value. If the 'names'
enumeration does not have any elements, we return true even not
trying
to compare anything.
We always check before that both attribute sets have the same
count. So
if the second attribute set is empty that means that the first set
also
empty.
// Does not have elements, so returns true.
123 while (result && names.hasMoreElements()) {
That is ok. Comparing two empty attribute sets should return true
because they are equal to each other.
MuxingAttributeSet.java
130 for (int i = 0; i < as.length; i++) {
Why we do not use foreach loop here?
Fixed.
AbstractDocument.java
1874 && !(obj instanceof AbstractElement)) {
I am not an expert in Swing, so this check is not quite clear to me.
Why we check AbstractDocument.AbstractElement and do not check, for
instance, SimpleAttributeSet? Is it only because of "the old
AbstractElement behaviour"?
There is a theory how to implement equals/hashCode methods
correctly in
Java.
For example, If there is a class Point{ double x; double y; } the
equals
method can be:
--------------------------------------------
public boolean equals(Object obj) {
if (obj instanceof Point) {
Point point = (Point) obj;
return x == point.x && y == point.y;
}
return false;
}
--------------------------------------------
And there is a child class
--------------------------------------------
class ColorPoint extends Point{
Color color;
}
--------------------------------------------
Point point = new Point(3,4);
ColorPoint c = new ColorPoint(3, 4, Color.GREEN);
ColorPoint bluePoint = new ColorPoint(3, 4, Color.BLUE);
What we want that the greenPoint was different from the bluePoint.
And we know that the check point.equals(greenPoint) returns true
because
Point 'equals' method checks only coordinates.
So the ColorPoint equals method could look like:
-----------------------------------------------
public boolean equals(Object obj) {
if (obj instanceof ColorPoint) {
return (((ColorPoint) obj).color == this.color) && super.equals(obj);
} else if (obj instanceof Point) {
return super.equals(obj);
}
return false;
}
-----------------------------------------------
The idea is that a child class should add an additional check for
objects that have the same instance.
The check 'if ((obj instanceof AttributeSet) && !(obj instanceof
AbstractElement))' in the AbstractDocument has been added for the same
reason.
If an object is an instance of AbstractDocument we return just
(this ==
obj) in other case if the object is and instance of AttributeSet we
return isEqual((AttributeSet) obj).
The theory also says that it is better to add 'isEqual' rather to
override the 'equals' method because in the second case
the hashCode method should be overriden accordingly and for mutable
object it could have troubles.
I decided to changed the fix to:
- make the equals and isEqual method symmetrically
- updated the hashCode method only if they were implemented
And leave all others as is to not break the backward compatibility.
By the way, I would say that we should check for equality by
reference
in the first place to get rid of comparing attributes if we call
equals method for the same object.
Fixed.
Thanks,
Alexandr.
Thank you,
Denis.
On 11/1/2012 5:30 PM, Alexander Scherbatiy wrote:
Hello,
Could you review the fix:
bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8001423
webrev: http://cr.openjdk.java.net/~alexsch/8001423/webrev.00
The 'equals' method from the SmallAttributeSet class compares local
attributes from the current set with all attributes (local and
parent)
from the second set if local counts are equal.
So if we check the following text attributes:
attr1: [ elem1 = value1] // getAttributeCount = 1
attr2: [ Parent = [ elem1 = value1] ] // getAttributeCount = 1
The results are:
attr1.equals(attr2) = false // attr1 does not contain element
with name
Parent
attr2.equals(attr1) = true // attr2 has element elem1 in the
parent set
All other classes that implement AttributeSet interface has the same
problems with the hashcode/equals contract.
The fix compares all local attributes from one set with all local
attributes from the another set in the equals method.
If both sets have parents, they will be compared automatically
(in the
same way as it was before).
SimpleAttributeSet: Hashtable returns hashcode based on both keys
and
values. The fix redefines it to return a hash code only for values.
MuxingAttributeSet: There is no direct way to get all local
attributes.
The getLocalAttribute method returns a value only if name is local.
AbstractElement: If both attribute sets are AbstractElement they are
compared by reference. In other case they are compared by
checking local
attributes. This preserves the old AbstractElement behaviour.
The javax/swing/text/AttributeSet/8001423/bug8001423.java test is
added.
I run the javax/swing/text tests and there is no new failures
after the
fix.
Thanks,
Alexandr.
--
Best regards, Sergey.
