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

[Alexey Ivanov]

Hi Alexandr,

The updated code looks fine to me.

Thanks,
Alexey

On 10.12.2015 17:06, Alexander Scherbatiy wrote:
>
>  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.
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>