RFR: JDK-8292276 : Add named colors from CSS Color Module Level 4 [v35]

ScientificWare duke at openjdk.org
Thu Aug 17 22:01:37 UTC 2023


On Thu, 17 Aug 2023 18:59:40 GMT, Phil Race <prr at openjdk.org> wrote:

>> As mentioned earlier in this review, you need to absorb the changes in 
>> https://github.com/openjdk/jdk/pull/10317/
>> into THIS PR and withdraw 10317.
>
>> @prrace
>> 
>> Do I absorb the changes in #15262 too ?
> 
> I hadn't even seen that one until you just mentioned it !
> I was puzzled because it was 8 months old .. but I then realised you've had it in draft all that time and just made it RFR a few days ago.
> 
> Having a third concurrent PR making changes to the same file is making things nearly insane.
> 
> The formal answer to your question is that
> (1) If these changes are unrelated in anyway to the other changes they can go separately, either before or after
> (2) If these changes require that the other changes be pushed first then they can go EITHER in the same PR or after
> (3)  if these changes are interdependent, meaning they cannot go before the other change and the other change also depends on them, then they should all be in one PR
> 
> If its (1) or (2) then I suggest we completely ignore this new PR until after the issues in the first one are pushed.

@prrace and @aivanov-jdk https://github.com/openjdk/jdk/pull/15262, https://github.com/openjdk/jdk/pull/10317 and https://github.com/openjdk/jdk/pull/9825 (this PR) are independant. That's why I planed to treat them separatly. It is also easier to test performance.

Yes, all affect the result of the `stringToColor` method but they are unrealated except for the stringToColor comments which need to be updated.

`stringToColor` is public only in `StyleSheet.java` but not in `CSS.java`. Using it directly with an unparsed string can be problematic if the string contains extra spaces. In fact this method is waiting for a parsed string.

In jshell, compare the following results :


import javax.swing.text.html.StyleSheet;
StyleSheet ss = new StyleSheet();

ss.stringToColor("#fe1ab5"); \\ rgb(254,26,181)
ss.stringToColor(" #fe1ab5"); \\ null
ss.stringToColor("rgb(254,26,181)"); \\ rgb(254,26,181)
ss.stringToColor(" rgb(254,26,181)"); \\ null
ss.stringToColor("rgb (254,26,181)");  \\ null

import javax.swing.text.AttributeSet;

AttributeSet as = ss.getDeclaration("color: rgb (254,26,181);"); \\ null


May be we can write it like that : 

    static public Color stringToColor(String string) {
        StyleSheet ss = new StyleSheet();
        AttributeSet as = ss.getDeclaration("color: %s;".formatted(string));
        return CSS.stringToColor(as.getAttribute(COLOR).toString());
    }

-------------

PR Comment: https://git.openjdk.org/jdk/pull/9825#issuecomment-1683033052


More information about the client-libs-dev mailing list