<Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2
Semyon Sadetsky
semyon.sadetsky at oracle.com
Mon Apr 11 06:33:12 UTC 2016
Looks good.
--Semyon
On 4/8/2016 10:00 PM, Alexander Scherbatiy wrote:
>
> Hello,
>
> Could you review the updated fix:
> http://cr.openjdk.java.net/~alexsch/8132119/webrev.11/
>
> - @throws tag is added in javadoc for methods in BasicTextUIDrawing
> class
> - unchecked exceptions are removed from methods throws declaration
>
> Thanks,
> Alexandr.
>
> On 08/04/16 22:34, Kevin Rushforth wrote:
>> 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