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

Semyon Sadetsky semyon.sadetsky at oracle.com
Fri Jan 15 16:58:36 UTC 2016 


On 1/15/2016 7:31 PM, Alexander Scherbatiy wrote:
>
>  Hello,
>
>  Could you review the updated fix:
>    http://cr.openjdk.java.net/~alexsch/8132119/webrev.06
>
>   - the description of the getClippedString method is updated to 
> mention that ellipsis is added at the end of the clipped string
>
> 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?
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.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>>
>

More information about the swing-dev mailing list