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

Wed Jan 13 15:03:12 UTC 2016

   Hello,

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

   - the methods description are updated to mention a component numeric 
shaper and non-print graphics context
   - @since tag value is updated to 9

  Thanks,
  Alexandr.

On 1/12/2016 10:42 PM, Philip Race wrote:
> @since needs to be just "9" as of the updated verona versioning scheme.
>
> -phil.
>
> On 1/12/16, 4:25 AM, Semyon Sadetsky wrote:
>> Hi Alexander,
>>
>> I see that you've added the next clarifications to the methods specs:
>>
>> > draws string/returns width ... using text properties and 
>> anti-aliasing hints from the provided component
>>
>> It still seems too brief and even incorrect for getStringWidth().
>>
>> For drawString():
>>
>> For non-printing case I would write:
>>
>> Sets the anti-aliasing rendering hints to the Graphics object if 
>> those are specified in the component properties. Then the string is 
>> drawn using the provided Graphics object with the numeric shaper 
>> taken into account if it is defined for the component.
>>
>> Please consider the printing case...
>>
>> For getStringWidth():
>>
>> Did not get which anti-aliasing hints it takes into account while 
>> there is no Graphics in the params list...
>> On the contrary, it does not take into account the rendering context.
>> My suggestion:
>>
>> Returns string pixel width by the provided font metrics and according 
>> to the numeric shaper if it is defined for the component.
>>
>> --Semyon
>>
>> For
>> On 12/10/2015 5:06 PM, 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.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>