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

[Alexander Scherbatiy]

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