<Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2
Kevin Rushforth
kevin.rushforth at oracle.com
Fri Apr 8 18:34:44 UTC 2016
Just my $0.02 (feel free to ignore).
Specifying "throws" on a method for an unchecked exception like NPE
seems odd. Typically, only checked exceptions are declared. Both checked
and unchecked exceptions can and should be documented with @throws.
-- Kevin
Sergey Bylokhov wrote:
> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
More information about the swing-dev
mailing list