On Wed, 31 May 2023 03:09:12 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org> wrote:
> I already told using your suggested code fails the regression test.. Which regression test do you refer to? The one for `AttributeSet.equals` or another? I can't see how implementing `equals` could affect other tests… unless some tests already use `AttributeSet.equals`. If the test being developed fails, it means the implementation in `CSS` is wrong. I haven't been able to thinker with the code myself. > What are you suggesting then? Neither parsing the string nor using the raw string seems right. The object already contains the parsed data as a set of fields: `value`, `index`, `lu`. https://github.com/openjdk/jdk/blob/927a9ed68371597eba1609f97ac03dd1de812e26/src/java.desktop/share/classes/javax/swing/text/html/CSS.java#L2203-L2205 I think the fields should be used in the implementation of the `equals` method, and, as [I already noted](https://github.com/openjdk/jdk/pull/13405#discussion_r1200351364), it should look something like this: @Override public boolean equals(Object val) { return val instanceof CSS.FontSize size && value == size.value && index == size.index && Objects.equals(lu, size.lu); } Using `Objects.equals` handles `null` values in `lu` fields gracefully. At the same time, this requires that `LengthUnit` implements `equals`. I suggested an implementation without `LengthUnit.equals`, it was probably incorrect because I didn't test it; however, implementing `LengthUnit.equals` is a better solution. @Override public boolean equals(Object obj) { return obj instanceof LengthUnit ou && type == ou.type && value == ou.value && Objects.equals(units, ou.units); } (Okay, I thought `CSS.LengthValue` also uses `LengthUnit` class but it doesn't, or rather it doesn't have it as a field, units are stored as a `String` value there.) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1211318063
