<Swing Dev> [11] RFR: JDK-8199441: Wrong caret position in multiline text components on Windows with a screen resolution higher than 100%
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Wed Jun 13 11:21:28 UTC 2018
Can I please have a 2nd +1 from someone?
Regards
Prasanta
On 6/11/2018 6:29 AM, Philip Race wrote:
> 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.viewToModel=>CompositeView#viewToModel=>WrappedPlainView#viewToModel=>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