On Thu, 17 Aug 2023 18:59:40 GMT, Phil Race <[email protected]> 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
