<Swing Dev> [9] Review request for 8156217 Selected text is shifted on HiDPI display
Alexandr Scherbatiy
alexandr.scherbatiy at oracle.com
Fri Aug 19 08:03:46 UTC 2016
On 8/19/2016 2:25 AM, Philip Race wrote:
> OK .. I do not know enough about how modelToView is used by Swing
> to know what is really needed here. Someone with a bit more Swing
> background needs to chime in. I was encouraged that the *API* surface
> of the changes was much smaller than it had seemed from the webrev
> but maybe that is because you did not include everything. For example
> although they are just subclassing the method overrides in PasswordView
> since that is a public class would become part of the spec .. would
> they not ?
> Just like the "int" counterparts today :-
> https://docs.oracle.com/javase/8/docs/api/javax/swing/text/PasswordView.html
>
> Put another way I was looking for what the content of the CCC would be.
Here is the updated version of the public API change which includes
overridden deprecated methods:
http://cr.openjdk.java.net/~alexsch/8156217/webrev.05/public-api.01
Thanks,
Alexandr.
>
> -phil.
>
>
> On 8/15/16, 11:48 AM, Alexander Scherbatiy wrote:
>> On 15/08/16 21:43, Phil Race wrote:
>>> 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.
>>
>> The main change for the Caret public API (methods
>> Caret.getMagicCaretPosition2D()/setMagicCaretPosition2D(Point2D p))
>> is not included in the current fix. I only moved the new methods
>> JTextComponent.modelToView2D(int pos)/viewToModel2D(Point2D pt) from
>> the fix for the Caret to this fix. These methods are used not only
>> for caret but in other cases like mouse handling, text dragging and
>> others.
>>
>> Thanks,
>> Alexandr.
>>
>>
>>>
>>> -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/20160819/d591edd2/attachment.html>
More information about the swing-dev
mailing list