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

Semyon Sadetsky semyon.sadetsky at oracle.com
Mon Jan 18 07:15:50 UTC 2016 


On 1/16/2016 12:25 AM, Sergey Bylokhov wrote:
> On 15/01/16 19:31, Alexander Scherbatiy wrote:
>>    - the description of the getClippedString method is updated to
>> mention that ellipsis is added at the end of the clipped string
>
> It seems that we go far far away from the description of mission of 
> this method to the description of its implementation. The goal of the 
> method is possibility for clients to use the same draw functions which 
> are used by our L&F so the custom components will look the same as 
> standard components. Description that something is added to the end of 
> the clipped string, the exact list of client properties seems 
> unnecessary?
"goal of the method is possibility for clients to use the same draw 
functions which are used by our L&F"...
then this sentence should be added to those methods docs instead.
But their should not be described as simply "draw string" or "clip string".
> Moreover it seems that if the requested space is really small we can 
> return ellipses which will occupy more space than requested.(i guess 
> this is a separate bug)
>
>>
>> On 1/15/2016 1:16 PM, Semyon Sadetsky wrote:
>>> Hi,
>>>
>>> getClippedString() : It is worth to explain that 3dots are added at
>>> end or returned.
>>>
>>> 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.
>>      I thought that rendering hints can affect a string quaility and
>> drawing performance. Do they really change the visual string width?
>>>
>>> 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.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>
>

More information about the swing-dev mailing list