Skip to content

Conversation

@scientificware
Copy link
Contributor

@scientificware scientificware commented Sep 16, 2022

This is referenced in Java Bug Database as

This is tracked in JBS as

Adds the 4 and 8 digits color hex notations to CSS.java, as described in :
CSS Color Module Level 4
W3C Candidate Recommendation Snapshot, 5 July 2022
6.2 The RGB Hexadecimal Notations: #RRGGBB

Designed from : ScientificWare JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8293776: Adds CSS 4 and 8 digits hex coded Color (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/10317/head:pull/10317
$ git checkout pull/10317

Update a local copy of the PR:
$ git checkout pull/10317
$ git pull https://git.openjdk.org/jdk.git pull/10317/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10317

View PR using the GUI difftool:
$ git pr show -t 10317

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10317.diff

Webrev

Link to Webrev Comment

 CSS level 4
 - defines color hex code as #[2 digits Red][2 digits Green][2 digits Blue][2 digits Alpha]. With digit 0 ... f.
 - allows, webpage passes 3, 4, 6 or 8 digit color code.
   - 3 digits #[R][G][B] ........ represents #[RR][GG][BB]FF
   - 4 digits #[R][G][B][A] ..... represents #[RR][GG][BB][AA]
   - 6 digits #[RR][GG][BB] ..... represents #[RR][GG][BB]FF
   - 8 digits #[RR][GG][BB][AA] . represents #[RR][GG][BB][AA]

 Becareful ! In java.awt.Color hex #[2 digits Alpha][2 digits Red][2 digits Green][2 digits Blue]
 Comme cette méthode est définie dans CSS, elle doit traiter uniquement le format CSS Level 4.

 According notes below the current OpenJDK implementation is
 - 3 digits #[R][G][B]    represents #[RR][GG][BB]FF
 - 6 digits #[R][G][B]    represents #[RR][GG][BB]FF
Typo in comment.
This implementation uses a Pattern instance and parses the string argument as an unsigned integer with radix 16.
- The pattern declaration and import are missing.
- The parseUnsignedInt method can send a NumberFormatException, its import is missing.
java.lang.NumberFormatException is imported by default.
Updatre Copyrigth date.
Removes unnecessary integer casts.
- This modifies CSS.java hexToColor method,
  - by adding two hex code formats,
  - by treating only 2, 3, 4 and 8 digits well formated hex code, all other string returns null.
Some spaces are missing or extra.
Adds a final modifier to hex Pattern.
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 16, 2022

👋 Welcome back scientificware! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 16, 2022
@openjdk
Copy link

openjdk bot commented Sep 16, 2022

@scientificware The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@mlbridge
Copy link

mlbridge bot commented Sep 16, 2022

Modifies the improper name of the recommendation with CSS Color Level 4.
CSS Level 4 doesn't exist. CSS is a set of Modules, one of them is Color.
- Adds final modifier to hex Pattern.
- corrects recommandation names CSS Level 4.
- Adds missing spaces.
Adds a regression for DK-8293776 : Adds CSS 4 and 8 digits hex coded Color #13
Removes author name for JDK main-line development repository.
scientificware and others added 3 commits September 18, 2022 09:08
Corrects typos.
And applies a better digit extraction as suggested in main-line review thread.
- Corrects typos.
- Applies a better digit extraction as suggested in main-line review thread.
  - Using charAt avoids creating temporary strings of length 1.
  - Also, using the %[argument_index$]conversion format specifier form allows halving the lengths of the vararg arrays.
- Corrects mistake about variable declaration for parseUnsignedInt(...) method returned value. Must be int and not Integer.

Hex3468DigitsColor.java :
- Ajout d'un test de régression pour JDK-8293776.
Comment on lines 1363 to 1366
if (digits.startsWith("#")) {
digits = digits.substring(1, Math.min(n, 9));
n--;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the method return null immediately if n < 3 || n > 8 after the if block at lines 1363-1366? It's already known the format is invalid.

I also propose adding hex.matcher(digits).matches() here to avoid running the matcher at each attempt.

In short, the next statement should be:

if (n < 3 || n == 7 || n > 8 || !hex.matcher(digits).matches()) {
    return null;
}

The following code deals with a valid color notation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aivanov-jdk, I suppose that most of color notations are correct.
Adding these tests in first place has a time cost.
With &&, hex.matcher(digits).matches() is evaluated only once.
That why I separated length test from hex test. Pattern usage has a significant time cost.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aivanov-jdk, I suppose that most of color notations are correct.

Likely, otherwise the colors wouldn't show up.

Adding these tests in first place has a time cost.

The time is rather negligible, in my opinion. Yet the code is clearer.

If you care about micro-optimisations, the n == 6 case should be the first one in the chain of if-blocks as the most common case.

With &&, hex.matcher(digits).matches() is evaluated only once. That why I separated length test from hex test.

Yes, you're absolutely right. I agree it is evaluated only once, which wasn't apparent to me from the start. Someone else may also consider it as inefficiency.

Pattern usage has a significant time cost.

Exactly. Is it even needed? The previous code did well without it.

I'm sure it's not needed, Integer.parseUnsignedInt validates the digits are valid, if not, it throws NumberFormatException. Since, as you say, it's rare that the color notation is invalid, I see no reason to avoid the exception using the pattern.

Comment on lines 1389 to 1399
if (n == 3 && hex.matcher(digits).matches()) {
final char r = digits.charAt(0);
final char g = digits.charAt(1);
final char b = digits.charAt(2);
digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$sff", r, g, b);
} else if (n == 4 && hex.matcher(digits).matches()) {
final char r = digits.charAt(0);
final char g = digits.charAt(1);
final char b = digits.charAt(2);
final char a = digits.charAt(3);
digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s", r, g, b, a);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to combine these two cases into one?

Suggested change
if (n == 3 && hex.matcher(digits).matches()) {
final char r = digits.charAt(0);
final char g = digits.charAt(1);
final char b = digits.charAt(2);
digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$sff", r, g, b);
} else if (n == 4 && hex.matcher(digits).matches()) {
final char r = digits.charAt(0);
final char g = digits.charAt(1);
final char b = digits.charAt(2);
final char a = digits.charAt(3);
digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s", r, g, b, a);
if (n == 3 || n == 4) {
final char r = digits.charAt(0);
final char g = digits.charAt(1);
final char b = digits.charAt(2);
final char a = n == 4 ? digits.charAt(3) : 'f';
digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s", r, g, b, a);

The only difference between these two case is in a which is hard-coded as ff in the format string for n == 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, the code is more concise but we double the number of tests ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, the code is more concise but we double the number of tests ?

For a rather rare case. I don't think it's a critical path which affects performance.

If you like, you can create a variable to store the result of n == 4 to avoid testing once again. I still don't think it's worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we reach a conciliation with this version. ? All cases are treated in 2 or 3 tests.
It incorporates all your suggestions and my single comment about the number of tests.

        if (n < 5) {
            if (n < 3) {
                return null;
            } else {
                final char r = digits.charAt(0);
                final char g = digits.charAt(1);
                final char b = digits.charAt(2);
                final char a = n == 4 ? digits.charAt(3) : 'f';
                digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s", r, g, b, a);
            }
        } else if (n < 7) {
            if (n == 5) {
                return null;
            } else {
                digits = String.format("%sff", digits);
            }
        } else if (n != 8 ) {
            return null;
        }
        try {
            int i = Integer.parseUnsignedInt(digits, 16);
            return new Color((i >> 24) & 0xFF, (i >> 16) & 0xFF, (i >> 8) & 0xFF, i & 0xFF);
        } catch (NumberFormatException nfe) {
            return null;
        }

Some articles suggest to avoid using exceptions as flow controls. I never tested this point. That's why I introduced Patterns in this method. May I run tests in this case ? With the code below :

        if (n < 5) {
            if (n < 3 || !hex.matcher(digits).matches()) {
                return null;
            } else {
                final char r = digits.charAt(0);
                final char g = digits.charAt(1);
                final char b = digits.charAt(2);
                final char a = n == 4 ? digits.charAt(3) : 'f';
                digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s", r, g, b, a);
            }
        } else if (n < 7) {
            if (n == 5 || !hex.matcher(digits).matches()) {
                return null;
            } else {
                digits = String.format("%sff", digits);
            }
        } else if (n != 8 || !hex.matcher(digits).matches()) {
            return null;
        }
        try {
            int i = Integer.parseUnsignedInt(digits, 16);
            return new Color((i >> 24) & 0xFF, (i >> 16) & 0xFF, (i >> 8) & 0xFF, i & 0xFF);
        } catch (NumberFormatException nfe) {
            return null;
        }

I also wish to test the version with bit rotation too :

        if (n < 5) {
            if (n < 3 || !hex.matcher(digits).matches()) {
                return null;
            } else {
                final char r = digits.charAt(0);
                final char g = digits.charAt(1);
                final char b = digits.charAt(2);
                final char a = n == 4 ? digits.charAt(3) : 'f';
                digits = String.format("%1$s%1$s%2$s%2$s%3$s%3$s%4$s%4$s", r, g, b, a);
            }
        } else if (n < 7) {
            if (n == 5 || !hex.matcher(digits).matches()) {
                return null;
            } else {
                digits = String.format("%sff", digits);
            }
        } else if (n != 8 || !hex.matcher(digits).matches()) {
            return null;
        }
        try {
            return new Color(Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),8),
                             true);
        } catch (NumberFormatException nfe) {
            return null;
        }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we reach a conciliation with this version. ? All cases are treated in 2 or 3 tests.

Your new versions introduce one more condition to each case and make the code more complicated and less clear. I am for concise, clear and readable code. I am against premature optimisation, and it hasn't been proved yet that there's a performance issue.

Combining the cases of n == 3 and n == 4 avoids code duplication by introducing one more comparison to both cases. If this becomes a critical path, JIT could optimise the code.

As such, I am still for combining these two cases. If you insist, I'm okay with your original code with code duplication. Yet I'd like to eliminate the duplicate code.

Some articles suggest to avoid using exceptions as flow controls. I never tested this point. That's why I introduced Patterns in this method.

It's true, creating an exception object is a somewhat expensive operation. Yet you say, “most of color notations are correct”, therefore an incorrect color which cannot be parsed is rare, so it's an exceptional situation, it is okay to use exceptions in this case. You also say, “Pattern usage has a significant time cost,” at the same time, you introduce a small time penalty for each and every correct color.

I think the hex pattern is redundant.

I also wish to test the version with bit rotation too :

            return new Color(Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),8),
                            true);

It could be not as clear, yet it works.

May I suggest wrapping the code for Color and rotateRight arguments?

            return new Color(
                    Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),
                                        8),
                    true);

It fits better into 80-column limit. Alternatively, wrap the second parameter of rotateRight only:

            return new Color(Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),
                                                 8),
                             true);

This way it's easier to see how many bits are rotated.

Copy link
Contributor Author

@scientificware scientificware Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aivanov-jdk My previous proposition perfectly respected all your suggestions and didn't introduce a new test.

But this discussion should be outdated if you validate this new approach. Performance results came from my repository I mentioned in the header.

  • The code before this PR ran in 230ms.
  • Our previous codes ran in 1 200ms to 1800 ms with String + formatted + %n$s usage.
  • They ran in 350ms to 380ms with String + formatted + %s usage.
  • And in 100ms to 110ms if we replace String + format with a string concatenation.
  • Now the code below gives the same results in 36ms and with all our expected behaviors. Since we control notation length we
    • can bypass some controls,
    • directly generate the color value,
    • without generate a new string,
    • and reject a wrong number format without generate any exception.
  /**
   * Convert a "#FFF", "#FFFF", "#FFFFFF" or "#FFFFFFFF" hex string to a Color.
   * If the color specification is bad, an attempt will be made to fix it up.
   */
  static final Color hexToColor(String digits) {
      // CSS Color level 4 allows webpage passes 3, 4, 6 or 8 digit color codes.
      //   - 3 digits #[R][G][B] ........ represents #[RR][GG][BB]FF
      //   - 4 digits #[R][G][B][A] ..... represents #[RR][GG][BB][AA]
      //   - 6 digits #[RR][GG][BB] ..... represents #[RR][GG][BB]FF
      //   - 8 digits #[RR][GG][BB][AA] . represents #[RR][GG][BB][AA]
      final byte[] iseq = digits.startsWith("#") ?
                               iseqmap.get(Integer.valueOf(digits.length())):
                               iseqmap.get(Integer.valueOf(-digits.length()));
      if (iseq == null) {
          // Rejects string argument with a wrong number length.
          return null;
      }
      // Only 3, 4, 6 and 8 digits notations.
      // Parses the string argument and build color value.
      int dv;
      int value = 0;
      for (byte i : iseq) {
          if ((dv = -i) != 15 && (dv = Character.digit(digits.charAt(i), 16)) < 0) {
              // Rejects string argument with not a valid digit in the radix-16.
              return null;
          }
          value = dv | value << 4;
      }
      return new Color(value, true);
  }

  // Map of Index Sequences. Index -15 means, use the default value 15.
  private static final Map<Integer, byte[]> iseqmap =
      Map.ofEntries(
          // Positive key, for # prefixed string, is associated with index from 1 to 8.
          // Negative key, for not # prefixed string, is associated with index from 0 to 7.
          Map.entry(Integer.valueOf(4), new byte[]{-15, -15, 1, 1, 2, 2, 3, 3}),
          Map.entry(Integer.valueOf(5), new byte[]{4, 4, 1, 1, 2, 2, 3, 3}),
          Map.entry(Integer.valueOf(7), new byte[]{-15, -15, 1, 2, 3, 4, 5, 6}),
          Map.entry(Integer.valueOf(9), new byte[]{7, 8, 1, 2, 3, 4, 6, 6}),
          Map.entry(Integer.valueOf(-3), new byte[]{-15, -15, 0, 0, 1, 1, 2, 2}),
          Map.entry(Integer.valueOf(-4), new byte[]{3, 3, 0, 0, 1, 1, 2, 2}),
          Map.entry(Integer.valueOf(-6), new byte[]{-15, -15, 0, 1, 2, 3, 4, 5}),
          Map.entry(Integer.valueOf(-8), new byte[]{6, 7, 0, 1, 2, 3, 4, 5})
      );

Copy link
Contributor Author

@scientificware scientificware Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is too strict ! Test with "#1fe56980 ", "#1fe56980 ". Made changes to be able to parse this kind of string.
Should I declare idseq as final for the same reasons a, b, c were declared final in the previous version.

Removes Classpath exception.
Suggested change, not to use `l` as an identifier because it could be confused with `1`.
This part of code could change and be replaced by bits right rotation after  performance tests.
Removes individual color tests and only compares the RGB value.
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 12, 2023

@scientificware This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@scientificware
Copy link
Contributor Author

Still waiting a review.

L'utilisation de la classe Patter n'est plus d'actualité.
Updates copyright date to 2023.
Updates copyright date to 2023.
@SWinxy
Copy link
Contributor

SWinxy commented Feb 23, 2023

I don't know if I like the use of Maps. I think I would prefer doing it via an enhanced switch. I've written a sketch of what I would write. Untested and probably very wrong, but I like it.

static Color hexToColor(String digits) {
    if (digits.startsWith("#")) {
        digits = digits.substring(1);
    }

    try {
        int a = Integer.parseInt(digits, 16);

        return switch (digits.length()) {
            case 3 -> new Color((a & 0xF00) << 24 | (a & 0xF0) << 16 | (a & 0xF) << 8 | 0xFF);
            case 4 -> new Color((a & 0xF000) << 24 | (a & 0xF00) << 16 | (a & 0xF0) << 8 | (a & 0xF));
            case 6 -> new Color((a & 0xFF0000) << 24 | (a & 0xFF00) << 16 | (a & 0xFF) << 8 | 0xFF);
            case 8 -> new Color(a);
            default -> null;
        }
    } catch (NumberFormatException e) {
        return null;
    }
}

I don't like when PRs languish. It sucks.

@prrace
Copy link
Contributor

prrace commented Feb 28, 2023

As noted there, this PR likely needs to be withdrawn and absorbed into #9825

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 28, 2023

@scientificware This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@scientificware
Copy link
Contributor Author

Delay due to the reorganization of my contribution schedule caused by new requests.

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 26, 2023

@scientificware This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@scientificware
Copy link
Contributor Author

Delay due to the reorganization of my contribution schedule caused by new requests.

@bridgekeeper
Copy link

bridgekeeper bot commented May 25, 2023

@scientificware This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@scientificware
Copy link
Contributor Author

Delay due to the reorganization of my contribution schedule caused by new requests.

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 22, 2023

@scientificware This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 20, 2023

@scientificware This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Jul 20, 2023
@scientificware
Copy link
Contributor Author

/open

@openjdk openjdk bot reopened this Jul 30, 2023
@openjdk
Copy link

openjdk bot commented Jul 30, 2023

@scientificware This pull request is now open

…ignments into separate line.

Merge master

# Conflicts:
#	src/java.desktop/share/classes/javax/swing/text/html/CSS.java
CSS.java :
Removes assigning values inside the if-condition
Extracts the assignments into separate line.

Hex3468DigitsColor.java :
Adds a message to result in case of failure only
@openjdk
Copy link

openjdk bot commented Aug 11, 2023

⚠️ @scientificware This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@prrace
Copy link
Contributor

prrace commented Aug 17, 2023

Back in Feb I wrote

As noted there, this PR likely needs to be withdrawn and absorbed into #9825

Nothing has changed on that front, so I don't know why you re-opened this.

@scientificware
Copy link
Contributor Author

@prrace Before withdraw it, I wanted to push all @aivanov-jdk's requested changes in this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client [email protected] rfr Pull request is ready for review

Development

Successfully merging this pull request may close these issues.

5 participants