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

Thu Dec 10 14:06:53 UTC 2015

  Hello,

  Could you review the updated fix:
    http://cr.openjdk.java.net/~alexsch/8132119/webrev.04/

On 12/10/2015 2:39 PM, Alexey Ivanov wrote:
> 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.

    The fix is updated according to the provided comments.

    Thanks,
    Alexandr.
>
>
> 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.
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>