'Re: <Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingU'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-swing-dev
Subject:    Re: <Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingU
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2015-12-23 14:41:51
Message-ID: 567AB2AF.7040106 () oracle ! com
[Download RAW message or body]

+1
On 10/12/15 22:33, Alexey Ivanov wrote:
> 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.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>


-- 
Best regards, Sergey.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic