RFR: 7083187: Class CSS.CssValue is missing implementations of equals() and hashCode() [v10]
Prasanta Sadhukhan
psadhukhan at openjdk.org
Wed May 31 03:11:59 UTC 2023
On Tue, 30 May 2023 17:32:10 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:
>> This is because font-size attribute can have units or percentage in it, it seems like as is being tested in the regression test, 42px, 42pt so "svalue" is being used which will have 42px/42pt so we can compare the string value of 42px or 42pt otherwise we need to parse to get the integer 42 and then the units px or pt and then compared separately
>
> We should not parse the string, it's already parsed.
>
> What if I put a space between value and unit? These two strings should produce equal attribute sets:
>
> {"font-size: 42px", "font-size: 42 px"},
>
>
> The easiest way is to implement `LengthUnit.equals` which you still need for comparing `CSS.LengthValue` anyway because that class has exactly the same problem. You also compare the raw string there:
>
> https://github.com/openjdk/jdk/blob/612d5eba4c83ab53c797b03a244879261a5faa2a/src/java.desktop/share/classes/javax/swing/text/html/CSS.java#L2678-L2681
>
> It doesn't look right to me.
>
> This is basically [a continuation of the same discussion](https://github.com/openjdk/jdk/pull/13405#discussion_r1202100159).
I am not sure I understand...Where the string is parsed...Earlier in my previous iteration I was using Scanner util class to parse the string to extract integer 42 and px from 42px and comparing separately which I thought you were referring to as we should not parse in `equals` so I removed and I optimized to compare the string value `svalue`
What are you suggesting then? I already told using your suggested code fails the regression test..
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13405#discussion_r1211039400
More information about the client-libs-dev
mailing list