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.
// Does not have elements, so returns true.
123 while (result && names.hasMoreElements()) {
MuxingAttributeSet.java
130 for (int i = 0; i < as.length; i++) {
Why we do not use foreach loop here?
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"?
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.
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.
