<Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2
Semyon Sadetsky
semyon.sadetsky at oracle.com
Thu Apr 7 07:20:26 UTC 2016
Looks good.
--Semyon
On 4/6/2016 11:23 PM, Alexander Scherbatiy wrote:
>
> Hello,
>
> Could you review the updated fix:
> http://cr.openjdk.java.net/~alexsch/8132119/webrev.09
>
> - TextUIDrawing interface and its default implementaion
> BasicTextUIDrawing class are added
> - font metrics argument description is updated
>
> On 31/03/16 23:23, Phil Race wrote:
>> Another webrev where you have to slip past 40_ files to get to the
>> two that really matter :-)
>> I would have put SwingUtilities2.java and TextUIDrawing.java as the
>> first files.
> Updated.
>>
>>
>> Some of what I have to say here is more along the lines of things to
>> think
>> about rather than things that are wrong .. but there are also maybe
>> some things
>> that need to be fixed.
>>
>> Is javax.swing.plaf really the right package for the new class ?
>> I suppose it is for the use by the UI classes so maybe its right.
>>
>> Should the methods be taking "double" instead of "int" for location ?
>> This means the measurement APIs too.
>> None of the JDK 1.2 text APIs use ints. That is all 1.0 legacy.
>> So if Swing internally wants to use ints that is OK but maybe the API
>> should be floating point (double).
> The provided methods use Graphics as argument which only has
> drawString(String, int, int) method.
> If it is possible it is better to add the methods with float
> arguments and Graphics2D later.
>
>> Would that help hi-dpi at all ?
> The hi-dpi support mostly does not require changes in Swing. What it
> does just scales graphics using default transform from graphics
> configuration.
>
>> I suppose it would add over-head since all the existing code uses int
>> and we are no worse off and can add double methods later if we want to.
>>
>>
>> Regarding FontMetrics we need to add a caution that is must be a
>> FontMetrics
>> *obtained from the correct font and graphics*.
> Updated.
>>
>>
>> i.e what about attributes on the font such as "tracking" ?
>> or on the graphics such as FRACTIONALMETRICS
>> It looks like Swing might already fail if that were used.
>>
>> Look at this code :-
>>
>> public static int stringWidth(JComponent c, FontMetrics fm, String
>> string){
>> if (string == null || string.equals("")) {
>> return 0;
>> }
>> boolean needsTextLayout = ((c != null) &&
>> (c.getClientProperty(TextAttribute.NUMERIC_SHAPING) != null));
>> if (needsTextLayout) {
>> synchronized(charsBufferLock) {
>> int length = syncCharsBuffer(string);
>> needsTextLayout = isComplexLayout(charsBuffer, 0,
>> length);
>> }
>> }
>> if (needsTextLayout) {
>> TextLayout layout = createTextLayout(c, string,
>> fm.getFont(),
>> fm.getFontRenderContext());
>> return (int) layout.getAdvance();
>> } else {
>> return fm.stringWidth(string);
>> }
>> }
>>
>> The only thing Swing is looking at is one TextAttribute and whether
>> we have complex text.
>> That is not enough. This is an existing implementation issue but one
>> we should fix here.
>> You need to examine all the methods for similar issues.
> I created an enhancement for this:
> JDK-8153662
> SwingUtilities2.drawString()/getStringWidth()/clipString() should use
> more text attributes
> https://bugs.openjdk.java.net/browse/JDK-8153662
>>
>> The usage of default methods is worth thinking about.
>> There is a significant subjective element to this and I've really
>> thought only myself about
>> using them for the case where default methods are useful when you
>> need to extend an
>> existing interface.
>>
>> But I can imagine they may also be handy when someone would typically
>> want to implement
>> only one or two out of a larger number in an interface and the
>> default and you for some
>> reason don't want to do that with an abstract class (notably you want
>> an other class to
>> be able to implement the interface).
>>
>> So what do I think about the case where the interface is brand new and
>> what you've done is more or less implement a concrete class, but it's
>> one that
>> can be mixed in to another class ? Is that an appropriate use ?
>>
>> Maybe the test to pass in that case is whether the default
>> implementation
>> is going to be satisfactory for 90% of uses. If they are frequently
>> over-ridden
>> it would not be an appropriate use.
> Using this argument it seems if someone wants to implement the
> TextUIDrawing it needs to override all its methods to keep them in
> sync. Because of this I moved the TextUIDrawing implementation to the
> separate javax.swing.plaf.basic.BasicTextUIDrawing class.
>>
>> Based on that criterion I think it is OK to use here.
>>
>> Another thought:
>> When you add default implementations you should also be on the hook
>> for explaining what that does. It is a deeper contract than you would
>> otherwise have as an interface and maybe needs to be an @implNote
>> or you need to call out the default implementation.
>> ie there is what someone must code to satisfy the contract of the
>> interface and what is a behaviour in the default method ?
> I added the addition "using text properties and anti-aliasing hints
> from the provided component" description to the BasicTextUIDrawing
> class. There is always a question how many details a method
> description should contain. It is not clear for me should, for
> example, the used numeric shaper be listed in the description or not?
> Especially when there is an enhancement that the methods
> implementation should take more text attributes into account.
>
>> Also here is a link to some comments by Brian Goetz on default methods :
>>
>> http://stackoverflow.com/questions/28681737/java-8-default-methods-as-traits-safe/28684917#28684917
>>
>>
>> 75 * No character is underlined if the index is negative or
>> greater
>> 76 * than the string length {@code (index < 0 || index >=
>> string.length())}
>> 77 * or if the char value specified at the given index
>> 78 * is in the low-surrogate range.
>>
>>
>> I suppose if you point at the last char and it is a hi-surrogate
>> nothing is underlined in that case either.
>>
>> But I find the whole writing of this a bit inadequate as if you are
>> going to
>> this kind of detail you perhaps also need to say what happens in a
>> complex
>> script where what happens is two unicode characters end up as a
>> ligature,
>> and/or perhaps you aren't even pointing to a base character.
>> Maybe it is in fact over-specified. I see that the implementation draws
>> the underline itself rather than delegate to TextLayout. This might
>> make sense for performance reasons where it is simple text but some day
>> this maybe should be re-examined and so I would not over-specify it.
>>
>> How about :
>> "The underline will be positioned at the base glyph which
>> represents the valid char indicated by the index.
>> If the char index is not valid or is not the index of a
>> valid unicode code point then no underline is drawn"
> Updated.
>
> Thanks,
> Alexandr.
>>
>> -phil.
>>
>>
>>
>> On 03/24/2016 07:22 AM, Sergey Bylokhov wrote:
>>> On 24.03.16 16:52, Alexander Scherbatiy wrote:
>>>> On 24/03/16 10:36, Semyon Sadetsky wrote:
>>>>> Hi Alexander,
>>>>>
>>>>> Could you answer one question:
>>>>> Why did you choose default interface methods to implement
>>>>> TextUIDrawing and not implement them in DefaultTextUIDrawing having
>>>>> declarations only in the interface?
>>>>> AFAIK the common point of view is default methods should be used
>>>>> rarely because they may lead to unreadable code.
>>>>>
>>>> The only problem which I know about default methods is multiple
>>>> inheritance which has its own solution.
>>>
>>> What kind of problems? The benefit is obvious: it will not be
>>> necessary to implement all methods if only one of them should be
>>> tweaked.
>>>
>>>>
>>>> Could you give links to discussion or provide use cases where
>>>> default
>>>> methods leads to the unreadable code and show how does it relate to
>>>> the
>>>> TextUIDrawing implementation?
>>>>
>>>> Thanks,
>>>> Alexandr.
>>>>> --Semyon
>>>>>
>>>>> On 3/18/2016 6:49 PM, Alexander Scherbatiy wrote:
>>>>>>
>>>>>> Could you review the updated fix:
>>>>>> http://cr.openjdk.java.net/~alexsch/8132119/webrev.08/
>>>>>>
>>>>>> - Public TextUIDrawing interface is added to the javax.swing.plaf
>>>>>> package
>>>>>> - TextUIDrawing methods description does not mention component
>>>>>> properties to be more general
>>>>>> - TextUIDrawing methods are made default
>>>>>> - L&F sets an instance of the TextUIDrawing to look and feel
>>>>>> defaults using "uiDrawing.text" property
>>>>>> - ComponentUI class is not changed
>>>>>> - Each ComponentUI reads TextUIDrawing from UI defaults
>>>>>> - There is an interesting issue described in
>>>>>> http://mail.openjdk.java.net/pipermail/swing-dev/2016-March/005509.html
>>>>>>
>>>>>> which is related to the fact that MetalLabelUI returns a static
>>>>>> field from createUI() method.
>>>>>> TitleBorder creates a JLabel but does not put it to any
>>>>>> component
>>>>>> hierarchy. In this case SwingUtilities.updateComponentTreeUI()
>>>>>> method
>>>>>> calls MetalLabelUI.uninstallDefaults() on the static metalLabelUI
>>>>>> field and sets a new LabelUI for ordinary labels. The TitleBorder
>>>>>> label UI is not changed in this case and it still uses the
>>>>>> metalLabelUI field which is not initialized.
>>>>>> It seems that other applications can also use components just
>>>>>> for
>>>>>> drawing and have the same issue.
>>>>>> For this case the textUIDrawing field is not cleared in the
>>>>>> uninstallDefaults but just set to a static default value which
>>>>>> should
>>>>>> not lead to memory leaks.
>>>>>>
>>>>>> Thanks,
>>>>>> Alexandr.
>>>>>>
>>>>>> On 29/01/16 19:51, Alexander Scherbatiy wrote:
>>>>>>> On 25/01/16 13:44, Andrej Golovnin wrote:
>>>>>>>> Hi Alexandr,
>>>>>>>>
>>>>>>>>> Could you review the updated fix:
>>>>>>>>> http://cr.openjdk.java.net/~alexsch/8132119/webrev.07/
>>>>>>>> ....
>>>>>>>>> - public TextUIDrawing interface is added to the javax.swing.plaf
>>>>>>>>> package
>>>>>>>>> - public "TextUIDrawing getTextUIDrawing()" method is added to
>>>>>>>>> the
>>>>>>>>> ComponentUI class
>>>>>>>>> - L&F sets an instance of the TextUIDrawing to look and feel
>>>>>>>>> defaults using
>>>>>>>>> "uiDrawing.text" property
>>>>>>>>> - Look and Feel delegates use the instance of the TextUIDrawing
>>>>>>>>> for text
>>>>>>>>> drawing and measuring
>>>>>>>> Some thoughts on the current design/implementation:
>>>>>>>>
>>>>>>>> By adding a field to the ComponentUI class the current
>>>>>>>> implementation increases
>>>>>>>> memory consumption for all Swing applications. And you get the
>>>>>>>> feeling that
>>>>>>>> there are different implementations of TextUIDrawing per
>>>>>>>> ComponentUI instances.
>>>>>>>> Personally I can't imagine to have different implementations of
>>>>>>>> TextUIDrawing for
>>>>>>>> a given LookAndFeel. If I would design/implement it, then I would
>>>>>>>> implement it as
>>>>>>>> a property of the LookAndFeel class (similar to LayoutStyle)
>>>>>>>> and not
>>>>>>>> the ComponentUI.
>>>>>>>> Developers can use then the following code to obtain the
>>>>>>>> instance of
>>>>>>>> TextUIDrawing:
>>>>>>>>
>>>>>>>> UIManager.getLookAndFeel().getUIDrawing() // or
>>>>>>>> UIManager.getLookAndFeelUIDrawing() // use this static method as a
>>>>>>>> short cut for the line above.
>>>>>>> LayoutStyle keeps its instance per App context. The same is for
>>>>>>> the LookAndFeel
>>>>>>> when it is got through UIManager.getLookAndFeel() call.
>>>>>>> It means that accessing an instance of a TextUIDrawing will leads
>>>>>>> to a time consumption.
>>>>>>>
>>>>>>> There are 3 main ways of the SwingUtilities2.drawString(...)
>>>>>>> usage:
>>>>>>> 1. ComponentUI classes
>>>>>>> 2. Components created in UI (like BasicInternalFrameTitlePane)
>>>>>>> 3. Public utilities methods (like
>>>>>>> WindowsGraphicsUtils.paintText())
>>>>>>>
>>>>>>> For the cases 1 and 2 it is possible to load and store the
>>>>>>> UIDrawing instance during installUI()/updateUI() calls to
>>>>>>> decrease a
>>>>>>> time access to it.
>>>>>>>
>>>>>>> For the case 3 it is necessary to get LookAndFeel instance each
>>>>>>> time (which is taken from an App context)
>>>>>>> or use the passed JComponent object. It requires to have a public
>>>>>>> method and the associated variable for each instance of
>>>>>>> JComponent/ComponentUI/... class.
>>>>>>>> You can use this methods then in JDK too.
>>>>>>>>
>>>>>>>> And maybe rename the TextUIDrawing class to just UIDrawing and add
>>>>>>>> more useful methods,
>>>>>>>> e.g. a method to create a composite font, a method to convert DLUs
>>>>>>>> to pixels.
>>>>>>> UIDrawing name may look like it should be used for any UI
>>>>>>> drawing,
>>>>>>> not only for text ones. I am afraid that it can be misleading.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Andrej Golovnin
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
More information about the swing-dev
mailing list