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

Fri Jan 15 21:37:09 UTC 2016

On 15/01/16 19:58, Semyon Sadetsky wrote:
>>> getStringWidth(): It is worth to add a note that the returning value
>>> is not the exact visual string width because rendering hints are not
>>> taken into account.

It depends, the current description is correct. Default font metrics of 
the component should take into account KEY_TEXT_ANTIALIASING property.
The width of the string will be calculated by the passed font metrics. 
If the font metrics differs from the metrics of the graphics object then 
results will be different, if the metrics will be the same then results 
will be the same.

If these methods will produce inconsistent result when we will get a 
bugs when the size of the component(which depends from the font) will 
differ from their appearance on the screen.

>>     I thought that rendering hints can affect a string quaility and
>> drawing performance. Do they really change the visual string width?
> Please look how the font anti-aliasing works:
> https://upload.wikimedia.org/wikipedia/commons/3/34/A_aliased_and_simple.jpg
>
> For example if user cut this width from a drawing artifacts may be
> remained.
>>>
>>> What was the reason for renaming clipStringIfNecessary() and
>>> stringWidth()?
>>>
>>> stringWidth() is a standard method name to get the advance in all
>>> other JDK interfaces.
>>     The 'get' prefix is used here not because it is JavaBeans
>> requirements but because of the method name conventions.
>>     For example, the same Utilities class contains
>> getTabbedTextWidth() method which also has the 'get' prefix.
>>
>>> clipStringIfNecessary method name better reflects the method
>>> operation than the proposed one.
>>     The 'necessary' word is vague in this case. Is there any need to
>> clip a string if it is not necessary?
>>     The method description gives the explanation in which case the
>> string will be clipped.
>>
>>  Thanks,
>>  Alexandr.
>>
>>> This is an utility static API and we don't need to follow JavaBeans
>>> naming conventions here.
>>>
>>> --Semyon
>>>
>>>
>>> On 1/13/2016 6:03 PM, Alexander Scherbatiy wrote:
>>>>
>>>>   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.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>

-- 
Best regards, Sergey.