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

Mon Jan 18 07:09:41 UTC 2016


On 1/16/2016 12:37 AM, Sergey Bylokhov wrote:
> 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.
Then, this: 
https://docs.oracle.com/javase/tutorial/2d/text/measuringtext.html is 
incorrect?
>
>
>>>     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.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>
>