<Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2
Alexandr Scherbatiy
alexandr.scherbatiy at oracle.com
Tue Apr 19 08:08:07 UTC 2016
On 4/11/2016 4:29 PM, Philip Race wrote:
>
>
> On 4/6/16, 1:23 PM, Alexander Scherbatiy wrote:
>>
>> Hello,
>>
>> Could you review the updated fix:
>> http://cr.openjdk.java.net/~alexsch/8132119/webrev.09
>>
>> - TextUIDrawing interface and its default implementaion
>> BasicTextUIDrawing class are added
>> - font metrics argument description is updated
>>
>> On 31/03/16 23:23, Phil Race wrote:
>>> Another webrev where you have to slip past 40_ files to get to the
>>> two that really matter :-)
>>> I would have put SwingUtilities2.java and TextUIDrawing.java as the
>>> first files.
>> Updated.
>>>
>>>
>>> Some of what I have to say here is more along the lines of things to
>>> think
>>> about rather than things that are wrong .. but there are also maybe
>>> some things
>>> that need to be fixed.
>>>
>>> Is javax.swing.plaf really the right package for the new class ?
>>> I suppose it is for the use by the UI classes so maybe its right.
>>>
>>> Should the methods be taking "double" instead of "int" for location ?
>>> This means the measurement APIs too.
>>> None of the JDK 1.2 text APIs use ints. That is all 1.0 legacy.
>>> So if Swing internally wants to use ints that is OK but maybe the API
>>> should be floating point (double).
>> The provided methods use Graphics as argument which only has
>> drawString(String, int, int) method.
>> If it is possible it is better to add the methods with float
>> arguments and Graphics2D later.
>>
>>> Would that help hi-dpi at all ?
>> The hi-dpi support mostly does not require changes in Swing. What
>> it does just scales graphics using default transform from graphics
>> configuration.
>
> Yes, but in another bug you are dealing with a problem positioning
> the caret because of (somewhat) similar issues where coordinates have
> been
> rounded to an integral. A floating point value allows you to say that
> this is 25.5 in user-space, even it if is 51.0 in device space.
It needs some more investigation.
What I have now is Swing uses font metrics to calculate a string
width (FontMetrics.charsWidth(...)) which sums up float char values.
The difference between font metrics used by Swing and font metrics
from graphics passed to paint method is that the fist has null frc.tx
matrix and the second one has a matrix with scales 2 on HiDPI display.
The returned char width by the font metrics with null transform has
value 7 for char 'a' (linear advance is 6.67 and xAdvance is 7).
The char width for the font metrics with scaled transform is 6.5 for
the same font and char. FileFontStrike requests glyph metrics and gets
linear advance 13.35 (dev transform is taken into account) xAdvance 13
- and apply the reverse transform. The result is 13 / 2 = 6.5.
And this bothers me because a result for applying the tx transform
and inverting it is different than just use the identity transform.
There are definitely problems with advance rounding but it seems they
are placed out of the Swing area.
>
>>
>>> I suppose it would add over-head since all the existing code uses int
>>> and we are no worse off and can add double methods later if we want to.
>>>
>>>
>>> Regarding FontMetrics we need to add a caution that is must be a
>>> FontMetrics
>>> *obtained from the correct font and graphics*.
>> Updated.
>>>
>>>
>>> i.e what about attributes on the font such as "tracking" ?
>>> or on the graphics such as FRACTIONALMETRICS
>>> It looks like Swing might already fail if that were used.
>>>
>>> Look at this code :-
>>>
>>> public static int stringWidth(JComponent c, FontMetrics fm,
>>> String string){
>>> if (string == null || string.equals("")) {
>>> return 0;
>>> }
>>> boolean needsTextLayout = ((c != null) &&
>>> (c.getClientProperty(TextAttribute.NUMERIC_SHAPING) != null));
>>> if (needsTextLayout) {
>>> synchronized(charsBufferLock) {
>>> int length = syncCharsBuffer(string);
>>> needsTextLayout = isComplexLayout(charsBuffer, 0,
>>> length);
>>> }
>>> }
>>> if (needsTextLayout) {
>>> TextLayout layout = createTextLayout(c, string,
>>> fm.getFont(),
>>> fm.getFontRenderContext());
>>> return (int) layout.getAdvance();
>>> } else {
>>> return fm.stringWidth(string);
>>> }
>>> }
>>>
>>> The only thing Swing is looking at is one TextAttribute and whether
>>> we have complex text.
>>> That is not enough. This is an existing implementation issue but one
>>> we should fix here.
>>> You need to examine all the methods for similar issues.
>> I created an enhancement for this:
>> JDK-8153662
>> SwingUtilities2.drawString()/getStringWidth()/clipString() should use
>> more text attributes
>> https://bugs.openjdk.java.net/browse/JDK-8153662
>
> you mean bug ? I upgraded it to P3 because it matters a lot more now
> with this public API
I do not think that it is a bug because the main request was to have
methods which draw strings in the same way as it is done by Swing L&Fs.
This will allow to have custom UI component which mimic to the
standard L&Fs.
It is also can be considered from the following point of view: is the
proposed request to use more text attributes more important for standard
Swing L&F or for custom L&F.
It seems is not the first case because Swing lives with the current
implementation for the long time.
For the second case we provide the public TextUIDrawing interface
which a developer can override and use any text attributes that are
necessary.
>>>
>>> The usage of default methods is worth thinking about.
>>> There is a significant subjective element to this and I've really
>>> thought only myself about
>>> using them for the case where default methods are useful when you
>>> need to extend an
>>> existing interface.
>>>
>>> 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.
>
> Fair enough although I wonder if by the same argument there is any point
> in exposing BasicTextUIDrawing as a public class ? I don't have a strong
> objection to exposing it, except that the name "Basic..." seems to
> invite subclassing ..
'Basic' in this case refers to the BasicLookAndFeel. All UI
components for the Basic L&F has the Basic prefix.
Basic L&F usually provides some basic implementation which can be
used by others L&F.
Thanks,
Alexandr.
>
> -phil
>>>
>>> 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