-
-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the ASCII character set optional in fonts #8932
Conversation
PR isn't finished. Editor changes needed |
for (int i = start; i <= end; i++) { | ||
ASCII_7BIT[i - start] = (char) i; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to have only one place where default symbols specified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly:
public static final char[] ASCII_7BIT = " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~".toCharArray();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that we usually don't do static initialization like that, but in this case, having it like this explains the logic behind this string. If you don't mind, I keep it this way.
for (int i = 0; i < chars.length(); i++) { | ||
char c = chars.charAt(i); | ||
characters.add((int) c); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic that can read both Old and New path. If new chars field is filled, then use this one, else use old path
} | ||
} | ||
Set<Integer> deDup = new HashSet<Integer>(characters); | ||
characters = new ArrayList<Integer>(deDup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a trick for deduplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use TreeSet because we need a sorted list? Otherwise HashSet is probably faster. Although I'm not sure it will have much impact on performance anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, Mats made a assumption that it would be nice to have consistent output between builds, and I agree with him:
However, you probably want to use a SortedSet here to ensure you get consistent ordering between builds?
} | ||
} | ||
Set<Integer> deDup = new HashSet<Integer>(characters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking with the de-duplication!
However, you probably want to use a SortedSet
here to ensure you get consistent ordering between builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense. Thank you! I replaced it with a TreeSet
Editor: Use explicit list of characters in .font files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I had a question and suggestion, but nothing to prevent this from being merged
The
Extra Characters
field has been deprecated and replaced with a new field calledCharacters
. This new field combines the default ASCII with any previously definedExtra Characters
. Using this new field, developers can specify exactly which symbols the font should include, e.g. numbers only, etc.Fix #8070
PR checklist
Example of a well written PR description:
### Technical changes
Technical changes:
Technical notes: