<Swing Dev> [11] RFR: JDK-8199441: Wrong caret position in multiline text components on Windows with a screen resolution higher than 100%

Jayathirth D V jayathirth.d.v at oracle.com
Fri Jun 15 08:21:32 UTC 2018


Hello Phil & Prasanta,

Please find my observation:

I went through the history of webrev's to understand what we are trying to do with respect to "round" value.

It looks like the problem we have is : When we have floating point "view location" and "useFPAPI == true" whether to use round value as "true" or "false". 

I went through the logic of how round is used in Utilities.getTabbedTextOffset() and I agree with Phil's observation.

So I tried to find different use cases for "round" value:

1) In javax/swing/text/GlyphPainter1.getBoundedPosition() we call getTabbedTextOffset() with 
	a) floating values for viewLocation
	b) useFPAPI is true
	c) But "round" value is false.

2) In javax/swing/text/PlainView.viewToModel() we call getTabbedTextOffset() with
	a) one of viewLocation value is float(so by Widening conversion it will call getTabbedTextOffset(.. , .. , float, float, ....))
	b) useFPAPI is true
	c) But "round" value is true.

May be the difference in logic between above cases will help us to understand use case of "round" value and help us to decide "round value" in WrappedPlainView.java.

Thanks,
Jay

-----Original Message-----
From: Philip Race 
Sent: Monday, June 11, 2018 6:29 AM
To: Prasanta Sadhukhan
Cc: swing-dev at openjdk.java.net
Subject: Re: <Swing Dev> [11] RFR: JDK-8199441: Wrong caret position in multiline text components on Windows with a screen resolution higher than 100%

FWIW I looked at this comment in the getTabbedTextOffset method
                 // the length of the string measured as a whole may differ from
                 // the sum of individual character lengths, for example if
                 // fractional metrics are enabled; and we must guard from this.

I am not really sure what the author was trying to say here.
Fractional Metrics is NOT the same thing as using floating point to measure width.
You will need floating point for fractional metrics .. but that makes it a necessary condition, not the same thing.

Also I looked at the rounding code

The logic for round==true seems to be trying to find the offset that is closest to the target position, so it may be less, the same or greater.

For round=false, it is trying to find the largest offset that does not exceed it.

I am not sure who would use the true case, but at least we can be consistent in using false in both cases.

+1

-phil.
-phil

On 6/10/18, 8:42 AM, Prasanta Sadhukhan wrote:
>
>
> On 6/9/2018 3:53 AM, Philip Race wrote:
>> The oddity about using round=true at line 845 is the other case seems 
>> to be using round == false ?
>>
>> I think round==false was intended for use with fractional metrics but 
>> it is not clear.
>>
> Seems logical.
> Modified webrev
> http://cr.openjdk.java.net/~psadhukhan/8199441/webrev.03/
>
> Regards
> Prasanta
>> -phil
>>
>> On 6/7/18, 4:12 AM, Prasanta Sadhukhan wrote:
>>> Updated webrev (as you pointed) to modify WrappedPlainView to use FP 
>>> version
>>>
>>> http://cr.openjdk.java.net/~psadhukhan/8199441/webrev.02/
>>>
>>> I have used "round" to be true as
>>> public static final int getTabbedTextOffset(Segment s, FontMetrics 
>>> metrics,
>>>                                              int x0, int x, 
>>> TabExpander e,
>>>                                              int startOffset) { uses 
>>> "round" value as true.
>>> I did not find any other usage of this int version of the API other 
>>> than WrappedPlainView.
>>>
>>> static final int getTabbedTextOffset(View view, Segment s, 
>>> FontMetrics metrics,
>>>                                          int x0, int x, TabExpander e,
>>>                                          int startOffset,
>>>                                          int[] justificationData) {} 
>>> is used in GLyphPainter1#viewToModel but it was not needed for this 
>>> particular issue so I have not changed it for now.
>>>
>>> Regards
>>> Prasanta
>>> On 6/7/2018 3:33 AM, Phil Race wrote:
>>>> This is changing the internals of getTabbedTextOffset and using FP 
>>>> regardless of whether useFPAPI is true. This is not what I said.
>>>>
>>>> I wrote that you should call the FP version directly.
>>>>
>>>> Look at this
>>>> ---
>>>>      * @deprecated replaced by
>>>>      *     {@link #getTabbedTextOffset(Segment, FontMetrics, float, 
>>>> float,
>>>>      *                                 TabExpander, int, boolean)}
>>>>      */
>>>>     @Deprecated(since = "9")
>>>>     public static final int getTabbedTextOffset(Segment s, 
>>>> FontMetrics metrics,
>>>>                                              int x0, int x, 
>>>> TabExpander e,
>>>>                                              int startOffset) {
>>>>         return getTabbedTextOffset(s, metrics, x0, x, e, 
>>>> startOffset, true);
>>>> ----
>>>> If we had wanted what you proposed in the first webrev and I think 
>>>> this one is effectively same, we'd have altered the behaviour to 
>>>> and not deprecated and added new API
>>>>
>>>> So whatever code in our L&F is currently calling
>>>>
>>>> getTabbedTextOffset(Segment s,
>>>>                     FontMetrics metrics,
>>>>                     int x0, int x, TabExpander e,
>>>>                     int startOffset)
>>>>
>>>> should instead call
>>>>
>>>> getTabbedTextOffset(Segment s,
>>>>                     FontMetrics metrics,
>>>>                     float x0, float x, TabExpander e,
>>>>                     int startOffset,
>>>>                     boolean round)
>>>>
>>>> I am not sure if round should be true or false but calling the 
>>>> float version is the key.
>>>>
>>>> This (and no other change needed) does the trick for me :
>>>>
>>>> ---
>>>> a/src/java.desktop/share/classes/javax/swing/text/WrappedPlainView.
>>>> java
>>>>
>>>> +++ 
>>>> b/src/java.desktop/share/classes/javax/swing/text/WrappedPlainView.
>>>> java
>>>>
>>>> @@ -360,11 +360,11 @@
>>>>          int currentWidth = getWidth();
>>>>          if (wordWrap) {
>>>>              p = p0 + Utilities.getBreakLocation(segment, metrics,
>>>> -                                                tabBase, tabBase + 
>>>> currentWidth,
>>>> + (float)tabBase, (float)(tabBase + currentWidth),
>>>>                                                  this, p0);
>>>>          } else {
>>>>              p = p0 + Utilities.getTabbedTextOffset(segment, metrics,
>>>> -                                                   tabBase, 
>>>> tabBase + currentWidth,
>>>> + (float)tabBase, (float)(tabBase + currentWidth),
>>>>                                                     this, p0, false);
>>>>          }
>>>>          SegmentCache.releaseSharedSegment(segment);
>>>> @@ -847,8 +847,8 @@
>>>>                          Segment segment = 
>>>> SegmentCache.getSharedSegment();
>>>>                          loadText(segment, p0, p1);
>>>>                          int n =
>>>> Utilities.getTabbedTextOffset(segment, metrics,
>>>> -                                                   alloc.x, x,
>>>> - WrappedPlainView.this, p0);
>>>> + (float)(alloc.x), (float)x,
>>>> + WrappedPlainView.this, p0, false);
>>>> SegmentCache.releaseSharedSegment(segment);
>>>>                          return Math.min(p0 + n, p1 - 1);
>>>>                      }
>>>>
>>>> I may have missed something else but to verify I downloaded and 
>>>> used your test, which BTW has DOS line-endings !
>>>>
>>>> -phil.
>>>>
>>>> On 06/06/2018 12:04 AM, Prasanta Sadhukhan wrote:
>>>>>
>>>>>
>>>>> On 5/9/2018 12:41 AM, Phil Race wrote:
>>>>>> I am not sure this is the right fix.
>>>>>> If the intention had been to change all calls to
>>>>>> getTabbedTextOffset() to use the FP API it would have just been 
>>>>>> changed .. rather than adding a new API that provides the option 
>>>>>> to specify whether to use FP.
>>>>>>
>>>>>> So perhaps the problem is that internal code needs to be updated 
>>>>>> to call the FP version directly ..
>>>>> I have updated the internal code to use the FP version directly 
>>>>> http://cr.openjdk.java.net/~psadhukhan/8199441/webrev.01/
>>>>> There are no new regressions in jtreg and jck tests.
>>>>>
>>>>> Regards
>>>>> Prasanta
>>>>>> I'd like to read https://bugs.openjdk.java.net/browse/JDK-8168992
>>>>>> to see
>>>>>> what was said there but JBS just went down for 5 hours 
>>>>>> maintenance ..
>>>>>>
>>>>>> -phil.
>>>>>>
>>>>>> On 05/07/2018 11:54 PM, Prasanta Sadhukhan wrote:
>>>>>>> Hi Sergey,
>>>>>>>
>>>>>>> I have run javax/swing/text jtreg and jck tests and did not 
>>>>>>> observe any regressions.
>>>>>>>
>>>>>>> Regards
>>>>>>> Prasanta
>>>>>>> On 4/28/2018 5:38 AM, Sergey Bylokhov wrote:
>>>>>>>> Hi, Prasanta.
>>>>>>>> Please confirm that you run related jck/reg tests.
>>>>>>>>
>>>>>>>> On 26/04/2018 23:52, Prasanta Sadhukhan wrote:
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>> Please review a fix for an issue where it is seen that,
>>>>>>>>>   with a screen resolution higher than 100% and then clicking 
>>>>>>>>> in a JTextArea having setLineWrap(true) set, the caret 
>>>>>>>>> (insertion point) is not aligned with the cursor.
>>>>>>>>>
>>>>>>>>> The issue seems to stem from the fact that caret position 
>>>>>>>>> calculation in DefaultCaret class utilises API that uses 
>>>>>>>>> integer calculation than floating point calculations.
>>>>>>>>> The code flow as seen is
>>>>>>>>> DefaultCaret#positionCaret=>BasicTextUI#viewToModel=>BoxView.v
>>>>>>>>> iewToModel=>CompositeView#viewToModel=>WrappedPlainView#viewTo
>>>>>>>>> Model=>Utilities.getTabbedOffset
>>>>>>>>>
>>>>>>>>> Now, getTabbedOffset utilises FontMetrics.charsWidth() which 
>>>>>>>>> uses integer arithmetic to get the caret position.
>>>>>>>>> The same getTabbedOffset uses Font.getStringBounds() which 
>>>>>>>>> uses floating point arithmetic via Rectangle2D.Float.
>>>>>>>>>
>>>>>>>>> Proposed fix is to make sure getTabbedOffset uses floating 
>>>>>>>>> point calculations by using getStringBounds() instead of
>>>>>>>>> charsWidth() so that it calculates the character width(s) of 
>>>>>>>>> text present in JTextArea in floating point to align the 
>>>>>>>>> caret.
>>>>>>>>>
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8199441
>>>>>>>>> webrev: 
>>>>>>>>> http://cr.openjdk.java.net/~psadhukhan/8199441/webrev.00/
>>>>>>>>>
>>>>>>>>> Regards
>>>>>>>>> Prasanta
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>



More information about the swing-dev mailing list