<Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Fri Apr 8 17:45:08 UTC 2016
On 07.04.16 17:07, Alexander Scherbatiy wrote:
> Could you review the updated fix:
> http://cr.openjdk.java.net/~alexsch/8132119/webrev.10
Looks fine to me, but I am curious how often we use "throws NPE" in
public API (especially w/o "@throws NPE" in javadoc).
>
> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>
--
Best regards, Sergey.
More information about the swing-dev
mailing list