8185261: Font fallback sometimes doesn't work in Swing text components

[JB-Dmitry]

The proposed fix makes fonts with and without fallback components distinguishable (in terms of equals method), so that
font metrics cache (and other similar code) can handle them separately. This is achieved by adding a new boolean field
to Font class, specifically denoting fonts with fallback components. The latter ones don't need to pretend to be
'created' fonts anymore, to preserve their Font2D handle.
It's not possible to use the existing createdFont field in equals implementation, as JCK requires a 'real' created
font (the one obtained using Font.createFont method) to be equal to the same font after serialization and
deserialization.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8185261: Font fallback sometimes doesn't work in Swing text components

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7313/head:pull/7313
$ git checkout pull/7313

Update a local copy of the PR:
$ git checkout pull/7313
$ git pull https://git.openjdk.java.net/jdk pull/7313/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7313

View PR using the GUI difftool:
$ git pr show -t 7313

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7313.diff

[bridgekeeper]

👋 Welcome back dbatrak! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@JB-Dmitry The following label will be automatically applied to this pull request:

• client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (e3fa744a)
• 00: Full (357ffd0c)

[mrserb]

Is the test depends on the headful libraries? Don't we need to test both headful and headless cases?

[JB-Dmitry]

Indeed, the test will work just as fine in a headless environment. Copied that line without fixing. Will remove it.

[mrserb]

It seems that different files use different style of "Copyright 2022 JetBrains s.r.o", is the current correct?

[JB-Dmitry]

To my knowledge, it's correct. It's also used in other JetBrains source code currently.

[mrserb]

I wonder how it will work if the Font was read from the system, then serialized/deserialized and then used?

[JB-Dmitry]

Not sure I fully understand the question. Font object cannot have both createdFont and withFallback fields set, at least in the current implementation.
If we're talking about 'created' fonts (created from a file), the fix doesn't change anything with regard to their behavior - after serialization/deserialization the font might not work, I guess, if it's not registered using GraphicsEnvironment.registerFont. In any case, it will lose it's 'created' status, and will change behavior correspondingly.
If we're talking about 'fonts with fallback' (like the ones JEditorPane creates under the hood), they will lose their fallback components after serialization/deserialization. But that's also how it worked before the proposed change. I'm not going to address that problem in this PR.

[prrace]

I don't know how much of the fix falls apart if this is changed, but I am not sure about adding this invisible attribute to equals(). I suppose font2DHandle would have the same issue .. but I didn't think that could be used in equals because it is assigned lazily so it would present even more problem.
I do see that it would be tricky to have some private extra equality testing in the code that finds the right fall back but I'd still like to see if that approach might work. Can you add in to the bug a more detailed explanation of where exactly you see the problem occur ?

I need some convincing and really would like to see if there is an alternative solution that I might prefer but that is something I can't switch on to right now since nothing is coming to me which means it is likely to take some time.

I didn't get the JCK comment createdFont is transient .. and nothing serializes the font data.
Serialization of Font objects is just serializing the fields liek name and style and size but there's no guarantee that it will behave the same .. I mean if I serialize Segoe UI on WIndows and try to deserialize it on Linux nothing in the world can fix the issue that Linux doesn't have that font.

[mrserb]

Serialization of Font objects is just serializing the fields liek name and style and size but there's no guarantee that it will behave the same .. I mean if I serialize Segoe UI on WIndows and try to deserialize it on Linux nothing in the world can fix the issue that Linux doesn't have that font.

Since the Font object will lose some fields during serialization its behavior will be changed even on the same platform.

[JB-Dmitry]

I've added some more details to the ticket about why exactly given test case isn't working as expected. Please let me know if I wasn't clear enough.

I don't know how much of the fix falls apart if this is changed, but I am not sure about adding this invisible attribute to equals().

That added line in equals() method is actually the main change in the fix. As you say, one could possibly also try to compare font2DHandle-s instead, but that would require forcing the population of that field in equals method, which seems to have larger potential for side effects.

As a font with fallback components can produce different rendering that the font without them, I believe, they shouldn't be considered 'equal'. Of course, there's an alternative method to fix this - make font metrics cache distinguish such fonts explicitly, without modifying Font.equals(). But font metrics cache isn't the only place comparing fonts using equals() method, and such a solution, I believe, would just mean waiting for other trouble in future.

The fact that equals would be using some private information isn't perfect of course. Ideally, this information (whether the font supports font fallback or not) should be public, and it should be possible to specify this property when creating a font object, but that needs introducing new API methods, and I don't want to go that far at the moment. Also, we're already living in a non-perfect world, where two Font objects with identical public properties might behave differently, and now we're just changing equals method to reflect that.

As for the comment about JCK, that relates to a yet another implementation option - do not introduce a new field, and just add comparing of createdFont fields in equals method. That's the way it was originally implemented in JBR 4 years ago, but after we found out it breaks JCK, the fix was changed to use a separate field. Corresponding JCK test, creates a font object from a file on filesystem, using Font.createFont method, serializes/deserializes it, and checks that the result is 'equal' to the initially created font. As createdFont field is transient, it's reset to false after serialization/deserialization, and the test obviously fails, if we try to include createdFont check into equals. Making createdFont field not transient also doesn't seem to be an option, as that is also considered an API change.

[prrace]

As you say, one could possibly also try to compare font2DHandle-s instead,
but that would require forcing the population of that field in equals method,
which seems to have larger potential for side effects.

Yes, I noted this would present even more problems.

Of course, there's an alternative method to fix this - make font metrics cache distinguish such fonts explicitly,
.... I believe, would just mean waiting for other trouble in future.

I was getting at this with my comment

I do see that it would be tricky to have some private extra equality testing in the code that finds the right fall back

But it wasn't (isn't) clear to me without tracking this down if some piece of the implementation is making
some assumptions it perhaps shouldn't. But yes, very likely not going to work exactly like that.

Taking a look I think the problem starts with getCompositeFontUIResource()

So another approach is to define a subclass of FontUIResource in which it over-rides hashCode() and equals()
and does the extra comparison needed.

It would only be equal to another CompositeFontUIResource with the same base font

sun.font.CompositeFontUIResource extends FontUIResource { ...

public void boolean equals(Object o) {
return (o instanceof CompositeFontUIResource && super.equals(o);
}

Then in sun.font.FontUtilities we add
if (compFont == null) {
compFont = new CompositeFont(physicalFont, dialog2D);
compMap.put(physicalFont, compFont);
}

• fuir = new CompositeFontUIResource(font);
    FontAccess.getFontAccess().setFont2D(fuir, compFont.handle);
  

Ah but this won't work if we are mixing Font and this sub-class since equals then won't be symmetric ..

The explanation about JCK indicates that JCK could also obtain a font from Swing that has fallbacks,
serialize it and on deserialization currently it would be equal but after this fix it would not.
So I don't see the difference except that there (presumably) happens not to be a JCK test for this scenario ...
However since JCK isn't part of OpenJDK I don't think we can discuss it any further here.

[JB-Dmitry]

That's true - after the fix, a font with fallback components will not be equal to the original one after serialization/deserialization. In my opinion, that's OK, as after that it will behave not in the way original font does - that's already the case now. Are you saying that is still a no-go? Then we either need to change the serialization format for Font by including a new non-transient field (btw, what's the procedure for that? is that possible? does it require CSR?) so that after deserialization it could 'restore' its fallback components. Or still make that 'more precise' font comparison elsewhere - in font metrics cache and other places.

[JB-Dmitry]

@prrace Any chance of looking at this?

[JB-Dmitry]

Still needs a review.

[prrace]

I know .. I know .. I just (a) don't like it, and (b) don't have a good alternative.

[JB-Dmitry]

@prrace So, how do we proceed from here? Should I just wait and ping you periodically?
I believe, the bug in question is quite important to fix. It's doesn't have a good workaround, at least in big projects where it's hard to have full control over the code base - a perfectly legitimate piece of code executed in one place will break the code in a different place. And it could be hard to investigate due to this reason.
I'm ready to re-do the fix in any acceptable way, but I'll need at least some directions about what is considered acceptable.

[JB-Dmitry]

Still waiting for a response.

If you're not comfortable with changing Font.equals behaviour, let's introduce some other public method, which can be used to distinguish fonts with and without fallback components. I believe, it should be in public API, as the issue isn't just about JEditorPane (etc) implementation. Even though it's not a documented feature, client code can easily construct a font with fallback components using existing public API - one should just use new javax.swing.text.StyleContext().getFont(family, style, size) instead of new java.awt.Font(family, style, size).

[JB-Dmitry]

Not having a feedback on PR for months is pretty discouraging. If any help in solving this issue isn't going to be accepted, let me know - I'll stop pinging then.

Just in case, let me re-iterate on the available options, as I see them:

• We don't modify Font.equals behaviour.
  In this case, modifications will need to be made in font metrics cache code (and, potentially, other affected places going forward as needed). That code would either access Font2D instance directly (making the approach available only for JDK-internal code), or we can introduce some public method in Font class, e.g. rendersEquallyTo(Font), which can perform Font2D comparison internally.

• We do modify Font.equals behaviour
  One option here is to use Font2D reference for comparison, but this needs to force font resolution before the comparison, and, I'm afraid, that would be too breaking a change. And if we cannot compare Font2D instances, we can only add some more state to Font object, either 'hidden' or made available via new API method(s).

[bridgekeeper]

@JB-Dmitry This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@JB-Dmitry This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.