<Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2

[Alexey Ivanov]

Hi Alexandr,

I suggest using {@code underlinedIndex} in this sentence:

* The {@code underlinedIndex} parameter points to a char value (Unicode 
code unit) in the
* given string.

in the Javadoc for drawStringUnderlineCharAt().


And I suggest using "fits" instead of "can fit" in @return for 
getClippedString() and rephrasing the conditions where empty string is 
returned:

* @return the clipped string that fits in the provided space, an empty
*         string if the given string argument is {@code null} or empty.


Regards,
Alexey

On 10.12.2015 5:23, Alexander Scherbatiy wrote:
> Hello,
>
> Could you review the updated fix:
>   http://cr.openjdk.java.net/~alexsch/8132119/webrev.03/
>
> - methods description is updated to mention used text properties and 
> anti-aliasing hints from the provided component
> - the drawStringUnderlineCharAt method description is updated
> - @since tag is added for the drawString() method
> - the description that some parameters may/must not be null is added
> - the test is updated to call the methods on EDT
> - the test is updated to check passed null arguments
>
> On 09/12/15 14:40, Alexey Ivanov wrote:
>> Hi Alexandr,
>>
>> Shouldn't drawString() also have @since tag?
>>
>>
>> Could you please also clarify whether underlinedIndex in 
>> drawStringUnderlineCharAt refers to the char index in the string?
>>
>> The statement
>> * The underlined index refers to char values (Unicode code units).
>> makes it unclear: underlinedIndex is an *index* or a *char* (Unicode 
>> code point).
>
>  I updated it to "The underlined index points to a char value (Unicode 
> code unit) in the given string."
>  The 'refers' word was used for a value at the given index.
>  However, I am not sure that the new variant is better.
>>
>>
>> * Nothing is drawn for null string. No character is underlined for the
>> * index {@code < 0}, {@code >=} than the string width or if the char 
>> value
>> * specified at the given index is in the low-surrogate range.
>>
>> I think it will be better to spell comparison operators, I mean to 
>> use  "greater than" rather than ">=". And "length" must be used 
>> instead of "width".
>>
>> I propose the following text:
>>
>> No character is underlined if the index is negative or greater than 
>> the string length or if the char value specified at the given index 
>> is in the low-surrogate range.
>>
>> For the first part of condition, you can add clarification in 
>> parenthesis: {@code index < 0 || index >= string.length()}.
>  Updated.
>>
>>
>> For consistency, please remove the full stop in @return tag in 
>> description of getClippedString.
>
> Updated.
>
> Thanks,
> Alexandr.
>
>>
>> Regards,
>> Alexey
>>
>> On 25.11.2015 18:28, Alexandr Scherbatiy wrote:
>>>
>>>
>>> Hello,
>>>
>>> Could you review the updated fix:
>>> http://cr.openjdk.java.net/~alexsch/8132119/webrev.02
>>>
>>> The javadoc references for the #drawStringUnderlineCharAt and 
>>> #getClippedString methods are moved after parameters description.
>>>
>>> Thanks,
>>> Alexandr.
>>>
>>>
>>> 14.09.2015 17:39, Alexander Scherbatiy пишет:
>>>>
>>>> Hello,
>>>>
>>>> Could you review the updated fix:
>>>> http://cr.openjdk.java.net/~alexsch/8132119/webrev.01/
>>>>
>>>> I tried to use Utilities.drawStringUnderlineCharAt(...) with chars 
>>>> that have
>>>> - 1 character:2 glyphs mapping (U+00E1) and ligature (U+FB01)
>>>> The whole glyph is underlined.
>>>> - 2 characters: 1 glyph mapping (supplementary char U+10400)
>>>>
>>>> The char value specified the the underlined index should point to 
>>>> the high-surrogate range of a supplementary character.
>>>> I updated the javadoc for the 
>>>> Utilities.drawStringUnderlineCharAt(...) method to:
>>>> -----------------------------
>>>> /**
>>>> * Draws the given string at the specified location underlining
>>>> * the specified character.
>>>> * <p>
>>>> * The underlined index refers to char values (Unicode code units).
>>>> * If the char value specified at the given underlined index is in
>>>> * the high-surrogate range and the char value at the following 
>>>> index is in
>>>> * the low-surrogate range then the supplementary character 
>>>> corresponding
>>>> * to this surrogate pair is underlined.
>>>> * <p>
>>>> * Nothing is drawn for null string. No character is underlined for the
>>>> * index {@code < 0}, {@code >=} than the string width or if the 
>>>> char value
>>>> * specified at the given index is in the low-surrogate range.
>>>> -----------------------------
>>>>
>>>> Thanks,
>>>> Alexandr.
>>>>
>>>> On 9/7/2015 12:27 PM, Alexander Scherbatiy wrote:
>>>>> On 9/2/2015 8:09 PM, Phil Race wrote:
>>>>>> I don't remember or know how Swing resolves this but the 
>>>>>> measurement ones
>>>>>> are not reliable since they do not take a Graphics context, so 
>>>>>> you cannot
>>>>>> measure the string properly. You need a FontRenderContext to 
>>>>>> measure.
>>>>>
>>>>> The provided methods use 
>>>>> SwingUtilities2.getFontRenderContext(JComponent) method which 
>>>>> returns the FontRenderContext associated with the component.
>>>>>
>>>>>>
>>>>>> So as it stands these APIs do not appear suitable to be made 
>>>>>> public as they
>>>>>> are not reliable.
>>>>>>
>>>>>> Whilst I could look at the code, if I instead just look at the 
>>>>>> API, I am scratching my
>>>>>> head over :-
>>>>>>
>>>>>> public static void drawString(JComponent c, Graphics g, String 
>>>>>> text, int x, int y)
>>>>>>
>>>>>> Here you provide the Graphics *and* the Component.
>>>>>> And it says the JComponent may be null.
>>>>>> So I am supposing that there is optional information that may be 
>>>>>> pulled from the
>>>>>> JComponent regarding rendering mode ?
>>>>>
>>>>> The optional information provided by the component is:
>>>>> - java.awt.font.NumericShaper
>>>>> - java.awt.font.FontRenderContext
>>>>> - antialiasing hints
>>>>>
>>>>>>
>>>>>> drawStringUnderlineCharAt(..) probably needs to explain if the 
>>>>>> index is code point
>>>>>> or UTF16 char index and what happens if there is not 1:1 code 
>>>>>> point:glyph mapping.
>>>>> I will update this.
>>>>>>
>>>>>> Are we sure that (any of) these really ought/need to be public - 
>>>>>> particularly given the
>>>>>> resolution of https://bugs.openjdk.java.net/browse/JDK-6302464
>>>>>
>>>>> These methods are used by JDK L&Fs to draw text. The initial 
>>>>> request was to provide public methods that can be used by a custom 
>>>>> L&F to draw strings consistently with other L&Fs.
>>>>>
>>>>> They are also designed to properly render text for printing. To do 
>>>>> that they use call to internal ProxyPrintGraphics class to obtain 
>>>>> the print graphics context.
>>>>>
>>>>> Even if printing staff will be public, these methods are just 
>>>>> utility methods (in the same way as other text methods in the 
>>>>> javax.swing.text.Utilities class) that help easily to draw and 
>>>>> print text in the same way as JDK L&Fs do that.
>>>>>
>>>>> Thanks,
>>>>> Alexandr.
>>>>>
>>>>>>
>>>>>> -phil.
>>>>>>
>>>>>> On 09/02/2015 08:28 AM, Alexander Scherbatiy wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you review the fix:
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8132119
>>>>>>> webrev: http://cr.openjdk.java.net/~alexsch/8132119/webrev.00
>>>>>>>
>>>>>>> The suggested drawString, drawStringUnderlineCharAt, 
>>>>>>> clipStringIfNecessary, and stringWidth methods are
>>>>>>> added to the javax.swing.text.Utilities class.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>