On 4/11/2016 4:29 PM, Philip Race wrote:
On 4/6/16, 1:23 PM, Alexander Scherbatiy wrote:
Hello,
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8132119/webrev.09
- TextUIDrawing interface and its default implementaion
BasicTextUIDrawing class are added
- font metrics argument description is updated
On 31/03/16 23:23, Phil Race wrote:
Another webrev where you have to slip past 40_ files to get to the
two that really matter :-)
I would have put SwingUtilities2.java and TextUIDrawing.java as the
first files.
Updated.
Some of what I have to say here is more along the lines of things to
think
about rather than things that are wrong .. but there are also maybe
some things
that need to be fixed.
Is javax.swing.plaf really the right package for the new class ?
I suppose it is for the use by the UI classes so maybe its right.
Should the methods be taking "double" instead of "int" for location ?
This means the measurement APIs too.
None of the JDK 1.2 text APIs use ints. That is all 1.0 legacy.
So if Swing internally wants to use ints that is OK but maybe the API
should be floating point (double).
The provided methods use Graphics as argument which only has
drawString(String, int, int) method.
If it is possible it is better to add the methods with float
arguments and Graphics2D later.
Would that help hi-dpi at all ?
The hi-dpi support mostly does not require changes in Swing. What
it does just scales graphics using default transform from graphics
configuration.
Yes, but in another bug you are dealing with a problem positioning
the caret because of (somewhat) similar issues where coordinates have
been
rounded to an integral. A floating point value allows you to say that
this is 25.5 in user-space, even it if is 51.0 in device space.
It needs some more investigation.
What I have now is Swing uses font metrics to calculate a string
width (FontMetrics.charsWidth(...)) which sums up float char values.
The difference between font metrics used by Swing and font metrics
from graphics passed to paint method is that the fist has null frc.tx
matrix and the second one has a matrix with scales 2 on HiDPI display.
The returned char width by the font metrics with null transform has
value 7 for char 'a' (linear advance is 6.67 and xAdvance is 7).
The char width for the font metrics with scaled transform is 6.5 for
the same font and char. FileFontStrike requests glyph metrics and gets
linear advance 13.35 (dev transform is taken into account) xAdvance 13
- and apply the reverse transform. The result is 13 / 2 = 6.5.
And this bothers me because a result for applying the tx transform
and inverting it is different than just use the identity transform.
There are definitely problems with advance rounding but it seems they
are placed out of the Swing area.
I suppose it would add over-head since all the existing code uses int
and we are no worse off and can add double methods later if we want to.
Regarding FontMetrics we need to add a caution that is must be a
FontMetrics
*obtained from the correct font and graphics*.
Updated.
i.e what about attributes on the font such as "tracking" ?
or on the graphics such as FRACTIONALMETRICS
It looks like Swing might already fail if that were used.
Look at this code :-
public static int stringWidth(JComponent c, FontMetrics fm,
String string){
if (string == null || string.equals("")) {
return 0;
}
boolean needsTextLayout = ((c != null) &&
(c.getClientProperty(TextAttribute.NUMERIC_SHAPING) != null));
if (needsTextLayout) {
synchronized(charsBufferLock) {
int length = syncCharsBuffer(string);
needsTextLayout = isComplexLayout(charsBuffer, 0,
length);
}
}
if (needsTextLayout) {
TextLayout layout = createTextLayout(c, string,
fm.getFont(),
fm.getFontRenderContext());
return (int) layout.getAdvance();
} else {
return fm.stringWidth(string);
}
}
The only thing Swing is looking at is one TextAttribute and whether
we have complex text.
That is not enough. This is an existing implementation issue but one
we should fix here.
You need to examine all the methods for similar issues.
I created an enhancement for this:
JDK-8153662
SwingUtilities2.drawString()/getStringWidth()/clipString() should use
more text attributes
https://bugs.openjdk.java.net/browse/JDK-8153662
you mean bug ? I upgraded it to P3 because it matters a lot more now
with this public API
I do not think that it is a bug because the main request was to have
methods which draw strings in the same way as it is done by Swing L&Fs.
This will allow to have custom UI component which mimic to the
standard L&Fs.
It is also can be considered from the following point of view: is the
proposed request to use more text attributes more important for standard
Swing L&F or for custom L&F.
It seems is not the first case because Swing lives with the current
implementation for the long time.
For the second case we provide the public TextUIDrawing interface
which a developer can override and use any text attributes that are
necessary.
The usage of default methods is worth thinking about.
There is a significant subjective element to this and I've really
thought only myself about
using them for the case where default methods are useful when you
need to extend an
existing interface.
But I can imagine they may also be handy when someone would
typically want to implement
only one or two out of a larger number in an interface and the
default and you for some
reason don't want to do that with an abstract class (notably you
want an other class to
be able to implement the interface).
So what do I think about the case where the interface is brand new and
what you've done is more or less implement a concrete class, but
it's one that
can be mixed in to another class ? Is that an appropriate use ?
Maybe the test to pass in that case is whether the default
implementation
is going to be satisfactory for 90% of uses. If they are frequently
over-ridden
it would not be an appropriate use.
Using this argument it seems if someone wants to implement the
TextUIDrawing it needs to override all its methods to keep them in
sync. Because of this I moved the TextUIDrawing implementation to the
separate javax.swing.plaf.basic.BasicTextUIDrawing class.
Fair enough although I wonder if by the same argument there is any point
in exposing BasicTextUIDrawing as a public class ? I don't have a strong
objection to exposing it, except that the name "Basic..." seems to
invite subclassing ..
'Basic' in this case refers to the BasicLookAndFeel. All UI
components for the Basic L&F has the Basic prefix.
Basic L&F usually provides some basic implementation which can be
used by others L&F.
Thanks,
Alexandr.
-phil
Based on that criterion I think it is OK to use here.
Another thought:
When you add default implementations you should also be on the hook
for explaining what that does. It is a deeper contract than you would
otherwise have as an interface and maybe needs to be an @implNote
or you need to call out the default implementation.
ie there is what someone must code to satisfy the contract of the
interface and what is a behaviour in the default method ?
I added the addition "using text properties and anti-aliasing hints
from the provided component" description to the BasicTextUIDrawing
class. There is always a question how many details a method
description should contain. It is not clear for me should, for
example, the used numeric shaper be listed in the description or not?
Especially when there is an enhancement that the methods
implementation should take more text attributes into account.
Also here is a link to some comments by Brian Goetz on default
methods :
http://stackoverflow.com/questions/28681737/java-8-default-methods-as-traits-safe/28684917#28684917
75 * No character is underlined if the index is negative or
greater
76 * than the string length {@code (index < 0 || index >=
string.length())}
77 * or if the char value specified at the given index
78 * is in the low-surrogate range.
I suppose if you point at the last char and it is a hi-surrogate
nothing is underlined in that case either.
But I find the whole writing of this a bit inadequate as if you are
going to
this kind of detail you perhaps also need to say what happens in a
complex
script where what happens is two unicode characters end up as a
ligature,
and/or perhaps you aren't even pointing to a base character.
Maybe it is in fact over-specified. I see that the implementation draws
the underline itself rather than delegate to TextLayout. This might
make sense for performance reasons where it is simple text but some day
this maybe should be re-examined and so I would not over-specify it.
How about :
"The underline will be positioned at the base glyph which
represents the valid char indicated by the index.
If the char index is not valid or is not the index of a
valid unicode code point then no underline is drawn"
Updated.
Thanks,
Alexandr.
-phil.
On 03/24/2016 07:22 AM, Sergey Bylokhov wrote:
On 24.03.16 16:52, Alexander Scherbatiy wrote:
On 24/03/16 10:36, Semyon Sadetsky wrote:
Hi Alexander,
Could you answer one question:
Why did you choose default interface methods to implement
TextUIDrawing and not implement them in DefaultTextUIDrawing having
declarations only in the interface?
AFAIK the common point of view is default methods should be used
rarely because they may lead to unreadable code.
The only problem which I know about default methods is multiple
inheritance which has its own solution.
What kind of problems? The benefit is obvious: it will not be
necessary to implement all methods if only one of them should be
tweaked.
Could you give links to discussion or provide use cases where
default
methods leads to the unreadable code and show how does it relate
to the
TextUIDrawing implementation?
Thanks,
Alexandr.
--Semyon
On 3/18/2016 6:49 PM, Alexander Scherbatiy wrote:
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8132119/webrev.08/
- Public TextUIDrawing interface is added to the javax.swing.plaf
package
- TextUIDrawing methods description does not mention component
properties to be more general
- TextUIDrawing methods are made default
- L&F sets an instance of the TextUIDrawing to look and feel
defaults using "uiDrawing.text" property
- ComponentUI class is not changed
- Each ComponentUI reads TextUIDrawing from UI defaults
- There is an interesting issue described in
http://mail.openjdk.java.net/pipermail/swing-dev/2016-March/005509.html
which is related to the fact that MetalLabelUI returns a static
field from createUI() method.
TitleBorder creates a JLabel but does not put it to any
component
hierarchy. In this case SwingUtilities.updateComponentTreeUI()
method
calls MetalLabelUI.uninstallDefaults() on the static metalLabelUI
field and sets a new LabelUI for ordinary labels. The TitleBorder
label UI is not changed in this case and it still uses the
metalLabelUI field which is not initialized.
It seems that other applications can also use components
just for
drawing and have the same issue.
For this case the textUIDrawing field is not cleared in the
uninstallDefaults but just set to a static default value which
should
not lead to memory leaks.
Thanks,
Alexandr.
On 29/01/16 19:51, Alexander Scherbatiy wrote:
On 25/01/16 13:44, Andrej Golovnin wrote:
Hi Alexandr,
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8132119/webrev.07/
....
- public TextUIDrawing interface is added to the
javax.swing.plaf
package
- public "TextUIDrawing getTextUIDrawing()" method is added
to the
ComponentUI class
- L&F sets an instance of the TextUIDrawing to look and feel
defaults using
"uiDrawing.text" property
- Look and Feel delegates use the instance of the TextUIDrawing
for text
drawing and measuring
Some thoughts on the current design/implementation:
By adding a field to the ComponentUI class the current
implementation increases
memory consumption for all Swing applications. And you get the
feeling that
there are different implementations of TextUIDrawing per
ComponentUI instances.
Personally I can't imagine to have different implementations of
TextUIDrawing for
a given LookAndFeel. If I would design/implement it, then I would
implement it as
a property of the LookAndFeel class (similar to LayoutStyle)
and not
the ComponentUI.
Developers can use then the following code to obtain the
instance of
TextUIDrawing:
UIManager.getLookAndFeel().getUIDrawing() // or
UIManager.getLookAndFeelUIDrawing() // use this static method
as a
short cut for the line above.
LayoutStyle keeps its instance per App context. The same is for
the LookAndFeel
when it is got through UIManager.getLookAndFeel() call.
It means that accessing an instance of a TextUIDrawing will
leads
to a time consumption.
There are 3 main ways of the SwingUtilities2.drawString(...)
usage:
1. ComponentUI classes
2. Components created in UI (like BasicInternalFrameTitlePane)
3. Public utilities methods (like
WindowsGraphicsUtils.paintText())
For the cases 1 and 2 it is possible to load and store the
UIDrawing instance during installUI()/updateUI() calls to
decrease a
time access to it.
For the case 3 it is necessary to get LookAndFeel instance each
time (which is taken from an App context)
or use the passed JComponent object. It requires to have a
public
method and the associated variable for each instance of
JComponent/ComponentUI/... class.
You can use this methods then in JDK too.
And maybe rename the TextUIDrawing class to just UIDrawing and
add
more useful methods,
e.g. a method to create a composite font, a method to convert
DLUs
to pixels.
UIDrawing name may look like it should be used for any UI
drawing,
not only for text ones. I am afraid that it can be misleading.
Thanks,
Alexandr.
Best regards,
Andrej Golovnin
