<Swing Dev> [8] Review request for 8001423 javax.swing.text.StyleContext.SmallAttributeSet breaks hashcode/equals contract

[Denis S. Fokin]

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.
>