<Swing Dev> [9] Review request for 8156217 Selected text is shifted on HiDPI display
Alexandr Scherbatiy
alexandr.scherbatiy at oracle.com
Mon Aug 15 11:13:32 UTC 2016
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/3bd87a14/attachment.html>
More information about the swing-dev
mailing list