<Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Thu Apr 7 14:07:25 UTC 2016
Hello,
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8132119/webrev.10
On 07/04/16 14:09, Sergey Bylokhov wrote:
> On 06.04.16 23:23, Alexander Scherbatiy wrote:
>> Could you review the updated fix:
>> http://cr.openjdk.java.net/~alexsch/8132119/webrev.09
>
> There is tiny typos
> TextUIDrawing.getClippedString():
> "@param c the component"
> I guess it should be:
> "@param c the component, may be null"
Updated.
> BasicTextUIDrawing:
> There is no dots at the end of the description of the class.
>
> I am not sure but will javadoc automatically copy the @throws tag from
> the parent tag? For example @throws NullPointerException to
> BasicTextUIDrawing.drawString().
I added throws NPE to the BasicTextUIDrawing class methods. It
forces copying @throws tag from the parent javadoc.
Thanks,
Alexandr.
>
>>> 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.
>
> I am fine, just one note. Now we have the interface, and the class
> without any state, and
> - If the users need to implement the only one method, then it will be
> necessary to subclass a BasicTextUIDrawing, which will limit the
> possible class hierarchy.
> - If the user most of the time will need to implement all methods to
> make them in sync he also can override BasicTextUIDrawing, then why we
> need the interface?
>
>>>
>>> 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