<Swing Dev> [9] Review request for 8132119 Provide public API for text related methods in SwingUtilities2

Mon Mar 21 21:09:37 UTC 2016

On 3/21/2016 1:38 PM, Sergey Bylokhov wrote:
> On 21.03.16 17:41, Alexander Scherbatiy wrote:
>>
>> On 18/03/16 19:49, 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.
>>
>>    I used JMH for an average time measuring of a test method which calls
>> text UI drawing class from a SynthLookAndFeel method:
>> ------------
>>      @Benchmark
>>      @BenchmarkMode(Mode.SampleTime)
>>      @OutputTimeUnit(TimeUnit.MICROSECONDS)
>>      public void testDrawMethod() throws Exception {
>>          int N = 10000;
>>          JComponent component = new JLabel(TEXT);
>>          SynthGraphicsUtils synthGrapicsUtils = new 
>> SynthGraphicsUtils();
>>          ...
>>          for (int i = 0; i < N; i++) {
>>              synthGrapicsUtils.computeStringWidth(synthContext, font,
>> fontMetrics, TEXT);
>>              synthGrapicsUtils.paintText(synthContext, g, TEXT, 10, 
>> 10, 0);
>>          }
>>      }
>>    ------------
>>
>>    Here is the full code of the tested method:
>> http://cr.openjdk.java.net/~alexsch/8132119/jmh/test.00/TextUIDrawingBenchmark.java 
>>
>>
>>
>>   I used 3 samples with different JDK:
>>   Sample 1: JDK without fixes
>>   Sample 2: JDK where instance of TextUIDrawing class is placed in base
>> ComponentUI class.
>>   Sample 3: JDK where instance of TextUIDrawing is loaded for each
>> necessary UI class from UIManager "uiDrawing.text" property.
>
> can you please provide the second patch as well?

   Sample 2: http://cr.openjdk.java.net/~alexsch/8132119/webrev.07
   Sample 3: http://cr.openjdk.java.net/~alexsch/8132119/webrev.08

   Thanks,
   Alexandr.
>
>>
>>   In the Sample 2 SynthGraphicsUtils retrieves a TextUIDrawing instance
>> from the provided JComponent.getUI() class.
>>   In the Sample 3 SynthGraphicsUtils gets a TextUIDrawing instance from
>> UIManager property.
>>
>>   The calculated average time in microseconds is:
>>   Sample 1: 20976.041 ± 60.181
>>   Sample 2: 21556.180 ± 89.476
>>   Sample 3: 23607.186 ± 62.319
>>
>>   If just roughly use a formula like:
>>     method_time = initialization_time + loop_count *
>> (time_for_same_operation + time_for different_operations)
>>   it is possible to get a time estimation for different operation like:
>> (method_time2 - method_time1) / loop_count
>>
>>   Such time difference calculation gives:
>>   0.08 microseconds for Sample 2
>>   0.26 microseconds for Sample 3
>>
>>   Thanks,
>>   Alexandr.
>>
>>>
>>>   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
>>>>
>>>
>>
>
>