QtCurve: work around new QFont::setStyleName() side-effects (WIP/testing required)
ClosedPublic

Authored by rjvbb on Dec 8 2017, 4:38 PM.

Details

Summary

QFont::setStyleName() has begun introducing side-effects in recent Qt versions. It now overrides the other font properties, which are also no longer updated. Calling setStyleName("Bold Italic") on a regular font will thus result in a QFont instance which

  • renders correctly (= Bold Italic)
  • has inconsistent toString() and key() representations
  • will ignore setBold(false) and setItalic(false)

QtCurve doesn't use the style name itself, but works with fonts set by the user and can thus have to deal with fonts that do have a style name set. The QFontDialog does this systematically (in this case the internal representation will be correct and complete though).

This patch introduces a workaround that strips the style name in a reliable manner, i.e. by recreating the input QFont instance:

  • if no style name is set (or an empty stylename), a quick copy is made
  • if the stylename equals the styleString() returned by the font database the font most likely has a consistent and complete internal representation, so a new font can be created from the traditional attributes
  • otherwise, a database lookup is performed to find the most appropriate match.

The fontStripStyleName() method and the font database instance are provided in a new helper class into which I have also moved the setBold(Widget*) and unSetBold(Widget*) functions.

See QTBUG-65037 (among others)

Test Plan

Works as expected with Qt 5.8 on Mac (Cocoa and X11) and Linux/X11 (testing with 5.9/Mac pending).

I have benchmarked the strip algorithm in a separate utility (http://github.com/RJVB/fontweightissue-qt5.git).

It would be good if this patch were tested by other QtCurve users to be sure it doesn't introduce behaviour or performance regressions.

Diff Detail

Repository
R626 QtCurve
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rjvbb requested review of this revision.Dec 8 2017, 4:38 PM
rjvbb created this revision.
rjvbb edited the summary of this revision. (Show Details)
yuyichao accepted this revision.Dec 8 2017, 4:59 PM

It feels like the API could have hidden this completely and do all the dirty work when setting the style name... Anyway, the change looks right given the description of the problem and if this is what we have to deal with so be it........

This revision is now accepted and ready to land.Dec 8 2017, 4:59 PM
rjvbb added a comment.Dec 8 2017, 6:25 PM
It feels like the API could have hidden this completely and do all the dirty work when setting the style name...

Apparently "they" don't agree; several discussions are open about it, including the ticket referenced in the summary.

Anyway, the change looks right given the description of the problem and if this is what we have to deal with so be it........

Thanks, did you test?

R.

Thanks, did you test?

No. I don't have any special font setting and more importantly my main computer is being repaired in the next 2 weeks and I don't have everything setup on the slow temporary laptop... ;(....

rjvbb added a comment.Dec 8 2017, 8:39 PM

You might already notice the effects if your system is otherwise up-to-date (Qt at least 5.8.0, Plasma 5.11). Just confirming your font choices in the fonts KCM should add the style names to your kdeglobals and even without that the plasma-integration plugin will probably do the same. See D9070, which I created after noticing that I had lost the bold text in my GUI.

This revision was automatically updated to reflect the committed changes.