<Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Fri Apr 8 19:00:58 UTC 2016
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