<Swing Dev> [9] Review request for 8156217 Selected text is shifted on HiDPI display
Phil Race
philip.race at oracle.com
Mon Aug 15 17:43:45 UTC 2016
Why is the caret support added in here ? Same for the modelToView
That will just hold this up as the reasoning behind needing those
changes is not something
I have yet been able to convince myself about - even after reading your
last email.
-phil.
On 08/15/2016 04:13 AM, Alexandr Scherbatiy wrote:
>
> Hello,
>
> Could you review the updated fix?
> webrev which contains only change in public API:
> http://cr.openjdk.java.net/~alexsch/8156217/webrev.05/public-api
> webrev with contains all changes:
> http://cr.openjdk.java.net/~alexsch/8156217/webrev.05/all
>
> - methods with int coordinates are deprecated
> - public isUseFloatingPointAPI()/setUseFloatingPointAPI() methods
> are added to the PlainView and WrappedPlainView classes
> - JTextComponent.modelToView2D(int pos)/viewToModel2D(Point2D pt)
> public methods from fix JDK-8163124 Add floating point API support to
> javax.swing.text.Caret
> are added
> - some @implSpec descriptions are removed from the new text drawing
> methods with floating point arguments
> - Built-in L&Fs are updated to use floating point API in standard
> Java text components
>
> Thanks,
> Alexandr.
>
> On 7/28/2016 5:38 PM, Alexandr Scherbatiy wrote:
>>
>> See comments inline.
>>
>> On 7/26/2016 11:57 PM, Phil Race wrote:
>>> I have a lot of doubts about this as well as trouble getting my head
>>> around all of it.
>>>
>>> Given that apps need to 'buy in' to the floating point I am not sure
>>> what we are gaining
>>> but I need to make sure I understand the problem.
>>>
>>> It affects only the methods that the 3rd party code can over-ride
>>> in subclasses and that are called by the JDK internal code.
>>>
>>> There are just two protected methods that matter :-
>>> PlainView.drawSelectedText(..)
>>> and
>>> PlainView.drawUnselectedText(..)
>>>
>>> The hidpi precison matters since they are drawing a sub-range of the
>>> text.
>>> Is there any other method that matters / is used in this way ?
>> I have found the following methods which relate to text drawing,
>> can be overridden and could have floating point coordinates:
>>
>> javax.swing.text.PlainView.drawLine(...)
>> javax.swing.text.PlainView.lineToRect(...)
>> javax.swing.text.PasswordView.drawEchoCharacter(...)
>>
>> javax.swing.plaf.TextUI.modelToView(...)
>> javax.swing.plaf.TextUI.viewToModel(...)
>> javax.swing.plaf.TextUI.getToolTipText(...)
>>
>> There is also a method which relates to a caret position in a text:
>> javax.swing.text.DefaultCaret.setMagicCaretPosition(Point p)
>> This requires additional investigation because DefaultCaret extends
>> Rectangle and so its coordinates can't be float.
>>
>>>
>>> Since 3rd party code is not over-riding these they will get the JDK
>>> super-class version, thus losing any customisation they might have done
>>> in the no-longer-called int version.
>>>
>>> Assuming that is correct, what customisation would be lost and how
>>> much does it matter?
>>
>> The example is javax.swing.text.PasswordView class which overrides
>> drawSelectedText(...)/drawUnselectedText(...) methods and draws echo
>> chars instead of text.
>> The similar can be done in a custom component:
>> --------
>> public class CustomPasswordField extends FieldView {
>>
>> @Override
>> protected int drawSelectedText(Graphics g, int x, int y, int p0,
>> int p1) throws BadLocationException {
>> // draw echo chars
>> }
>> }
>> --------
>>
>> Switching to support new methods with floating point coordinates will
>> lead that real text will be shown for old applications in password
>> fields.
>>>
>>> My prefernce is to deprecate the int versions and always call the
>>> float versions
>>> rather than the opt-in approach.
>>>
>>> Actually my real preference would be to come up with something that does
>>> not involve drawing the text in chunks like this.
>>>
>>> ie Swing should use AttributedCharacterIterator .. it looks like
>>> the code to
>>> do this might already be there !
>>>
>>> 106 private float drawElement(int lineIndex, Element elem, Graphics g,
>>> 107 float x, float y, boolean fractionalCharBounds)
>>> 108 throws BadLocationException
>>> 109 {
>>> 110 int p0 = elem.getStartOffset();
>>> 111 int p1 = elem.getEndOffset();
>>> 112 p1 = Math.min(getDocument().getLength(), p1);
>>> 113
>>> 114 if (lineIndex == 0) {
>>> 115 x += firstLineOffset;
>>> 116 }
>>> 117 AttributeSet attr = elem.getAttributes();
>>> 118 if (Utilities.isComposedTextAttributeDefined(attr)) {
>>> 119 g.setColor(unselected);
>>> 120 x = Utilities.drawComposedText(this, attr, g, x, y,
>>> 121 p0-elem.getStartOffset(),
>>> 122 p1-elem.getStartOffset());
>>> 123 } else {
>>>
>>> In fact what *that* illustrates is that applications already cannot
>>> expect
>>> their over-ridden methods to be called, so this fix is trying to fix
>>> something
>>> that can't be fixed.
>> The javadoc for the "protected PlainView.drawLine(...)" method is:
>> ---------
>> /**
>> * Renders a line of text, suppressing whitespace at the end
>> * and expanding any tabs. This is implemented to make calls
>> * to the methods {@code drawUnselectedText} and
>> * {@code drawSelectedText} so that the way selected and
>> * unselected text are rendered can be customized.
>> ---------
>>
>> Applications can rely on this behaviour and stopping to call the
>> drawSelectedText(...)/drawUnselectedText(...) methods with int
>> coordinates will be incompatible change.
>>
>>>
>>> So why can't we do that ? Just deprecate those int methods, don't add
>>> the float methods and use ACI ..
>> New float methods allow to easily migrate on new API for
>> applications without significant changes.
>>
>>> BTW getTabSize() is supposed to be a character count isn't it ? Not
>>> a pixel
>>> count. So why does it need a float version.
>> Could you review the updated fix:
>> http://cr.openjdk.java.net/~alexsch/8156217/webrev.04
>>
>> - methods with int coordinates which can be overridden are deprecated
>> - getFractionalTabSize() method is removed
>>
>> Thanks,
>> Alexandr.
>>>
>>> -phil
>>>
>>> On 06/30/2016 08:50 AM, Alexandr Scherbatiy wrote:
>>>> On 6/28/2016 8:14 PM, Alan Snyder wrote:
>>>>> Suppose an application is only partially fixed to use/override the
>>>>> floating point methods. Perhaps it uses a library that has not
>>>>> been fixed.
>>>>>
>>>>> Is there a more fine grained way to detect programmer awareness or
>>>>> lack of awareness of the new methods?
>>>>
>>>> Here is a slightly updated version which adds public
>>>> isUseFloatingPointAPI()/setUseFloatingPointAPI() methods to the
>>>> PlainView and WrappedPlainView classes:
>>>> http://cr.openjdk.java.net/~alexsch/8156217/webrev.02
>>>>
>>>> Using the floating point API is disabled by default and enabled
>>>> for standard Swing text component classes. This has advantage that
>>>> selection will work for text component in users applications on
>>>> HiDPI display.
>>>>
>>>> But it still has the same problem. Applications which use custom
>>>> View classes needs to updated them to implement corresponding text
>>>> drawing methods with floating point arguments and enable the
>>>> floating point API usage.
>>>>
>>>> Thanks,
>>>> Alexandr.
>>>>
>>>>>
>>>>> Alan
>>>>>
>>>>>
>>>>>> On Jun 28, 2016, at 9:59 AM, Alexandr Scherbatiy
>>>>>> <alexandr.scherbatiy at oracle.com
>>>>>> <mailto:alexandr.scherbatiy at oracle.com>> wrote:
>>>>>>
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> I tried to merge this fix with the 8132119 Provide public API
>>>>>> for text related methods in SwingUtilities2
>>>>>> and found a flow in the used algorithm.
>>>>>>
>>>>>> For each method that uses integer coordinates the fix adds a
>>>>>> pair with floating point arguments.
>>>>>> The fix 8156217 uses only methods with floating point values to
>>>>>> correctly handle a selected text.
>>>>>> This leads that overridden method with integer arguments in user
>>>>>> code is not called anymore.
>>>>>>
>>>>>> I think that this can be handled in the following way:
>>>>>> - Add a property that enables to use methods with floating point
>>>>>> arguments in Swing.
>>>>>> By default it is false and all work as before. The issue with
>>>>>> selected text is reproduced.
>>>>>> An application with enabled property does not have issue with
>>>>>> the selected text but a user should override
>>>>>> all methods with floating point values if he uses
>>>>>> corresponding methods with integer values.
>>>>>>
>>>>>> Here is a proposed solution where new public system property
>>>>>> "javax.swing.floatingPoints.enabled" is added:
>>>>>> http://cr.openjdk.java.net/~alexsch/8156217/webrev.01
>>>>>>
>>>>>> - Fix the enhancement JDK-8157461 Glyph image rendering for HiDPI
>>>>>> displays
>>>>>>
>>>>>> Thanks,
>>>>>> Alexandr.
>>>>>>
>>>>>> On 6/16/2016 6:07 PM, Alexandr Scherbatiy wrote:
>>>>>>> On 6/16/2016 4:47 PM, Alexandr Scherbatiy wrote:
>>>>>>>>
>>>>>>>> I tried to look deeper in the code and it seems there is a
>>>>>>>> rounding issue when float values are summed up.
>>>>>>>>
>>>>>>>> Suppose a transform with scale 1.5 is used and the 'a' char
>>>>>>>> advance is 10 in a dev space.
>>>>>>>> The 'a' char has advance 10 / 1.5 = 6.666666666666667 as double
>>>>>>>> value and 6.6666665 when it is cast to float in user space.
>>>>>>>> The width of a string which consists of 15 'a' chars is 15 *
>>>>>>>> 6.6666665 = 100.000000.
>>>>>>>> But the same width calculated as sum of each glyph advance in
>>>>>>>> StandardGlyphVector.initPositions() method is 99.999992.
>>>>>>>> --------------
>>>>>>>> double scale = 1.5;
>>>>>>>> float advance = (float) (10 / scale);
>>>>>>>> int N = 15;
>>>>>>>> System.out.printf("%d * %f = %f\n", N, advance, N *
>>>>>>>> advance);
>>>>>>>> float sum = 0;
>>>>>>>> for (int i = 0; i < N; i++) {
>>>>>>>> sum += advance;
>>>>>>>> }
>>>>>>>> System.out.printf("sum: %f\n", sum);
>>>>>>>> --------------
>>>>>>>>
>>>>>>>> Because of this a string drawn from float position 99.999998 is
>>>>>>>> shifted one pixel left which affects the text selection code in
>>>>>>>> Swing:
>>>>>>>> ------------------------
>>>>>>>> g.scale(1.5, 1.5);
>>>>>>>> String TEXT = "aaaaaaaaaaaaaaaa";
>>>>>>>> Rectangle2D rect = font.getStringBounds(TEXT, 0, index,
>>>>>>>> g.getFontMetrics().getFontRenderContext());
>>>>>>>> float selectedTextPosition = (float) rect.getWidth();
>>>>>>>> // 99.999992
>>>>>>>> g.drawString(TEXT.substring(0, index), x, y); //
>>>>>>>> non-selected text
>>>>>>>> g.drawString(TEXT.substring(index, TEXT.length()), x +
>>>>>>>> selectedTextPosition, y); // selected text is shifted to one
>>>>>>>> pixel left
>>>>>>>> ------------------------
>>>>>>> The last step is how coordinates are scaled in
>>>>>>> Graphics2D.drawString() method.
>>>>>>> If the graphics has scale 1.5 and zero translate the
>>>>>>> transformed coordinates are:
>>>>>>> (99.999992 + 0) * 1.5 = 149.999985
>>>>>>> (100.000000 + 0) * 1.5 = 150.000000
>>>>>>>
>>>>>>> Both of them are rounded to the same value.
>>>>>>>
>>>>>>> If the translate is set to integer 1 value:
>>>>>>> (99.999992 + 1) * 1.5 = 151.499989 // shifted to one pixel left
>>>>>>> (100.000000 + 1) * 1.5 = 151.500000
>>>>>>>
>>>>>>> A position 99.999992 in user space is rounded to 151 in dev space.
>>>>>>> A position 100.000000 in user space is rounded to 152 in dev space.
>>>>>>>
>>>>>>> And this difference can depend on the translate even it has
>>>>>>> integer value in user space because it is multiplied on the
>>>>>>> graphics scale.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alexandr.
>>>>>>>>
>>>>>>>> On 6/2/2016 11:41 PM, Alexandr Scherbatiy wrote:
>>>>>>>>> On 5/31/2016 10:40 PM, Phil Race wrote:
>>>>>>>>>>
>>>>>>>>>> I applied this and it is *much* better but there still seem
>>>>>>>>>> to be some tiny quirks.
>>>>>>>>>> When I drag the mouse to select text down and then up again,
>>>>>>>>>> as I pass the
>>>>>>>>>> original mouse click point vertically, repaint seem to jiggle
>>>>>>>>>> vertically by a pixel.
>>>>>>>>>> Perhaps a rounding issue in the repaint code's calculation of
>>>>>>>>>> the location of
>>>>>>>>>> the target y. I think I may see the same in left/right
>>>>>>>>>> dragging along a line too.
>>>>>>>>>> So I think this is repaint and not text related. Can you take
>>>>>>>>>> a look.
>>>>>>>>>
>>>>>>>>> I am able to reproduce this only using a floating point scale.
>>>>>>>>> It looks like 2d issue. I used a test which draws a text
>>>>>>>>> in two pieces. The second piece of the text is shifted from
>>>>>>>>> the first piece by the floating point size of the the first
>>>>>>>>> piece of the text.
>>>>>>>>> -----------
>>>>>>>>> Rectangle2D rect = font.getStringBounds(TEXT, 0, index,
>>>>>>>>> g.getFontMetrics().getFontRenderContext());
>>>>>>>>> float selectedTextPosition = (float) rect.getWidth();
>>>>>>>>> g.drawString(TEXT.substring(0, index), x, y);
>>>>>>>>> g.drawString(TEXT.substring(index, TEXT.length()), x +
>>>>>>>>> selectedTextPosition, y);
>>>>>>>>> -----------
>>>>>>>>>
>>>>>>>>> The second piece of the text can be shifted in the 2 cases:
>>>>>>>>> a) graphics scale is 1.5 and translation is 1.
>>>>>>>>> b) graphics scale is 2.25 without applied translation
>>>>>>>>>
>>>>>>>>> I have filed an issue on it:
>>>>>>>>> JDK-8158370 Text drawn from float pointing position and
>>>>>>>>> with float pointing scale is shifted
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8158370
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Alexandr.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -phil.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 05/06/2016 12:31 PM, Alexandr Scherbatiy wrote:
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> Could you review the fix:
>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8156217
>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~alexsch/8156217/webrev.00
>>>>>>>>>>>
>>>>>>>>>>> This is the second part of the fix related to the fact
>>>>>>>>>>> that char width can be fractional in user space.
>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/swing-dev/2016-May/005814.html
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> The Font.getStringBounds(...) method is used for the
>>>>>>>>>>> fractional string width calculation by Swing in user space.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Alexandr.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160815/1f523234/attachment.html>
More information about the swing-dev
mailing list