Replace obsolete QFontMetrics::width with QFontMetrics::horizontalAdvance
ClosedPublic

Authored by gengisdave on Jul 13 2019, 12:03 PM.

Details

Reviewers
nmel
asensi
Group Reviewers
Krusader
Summary

In Qt 5.11 was introduced horizontalAdvance() to replace width(). In version 5.13, the old method is marked as obsolete.

In a far future, when the minimal Qt version will be updated (now 5.9), the old methods will be removed safely.

Test Plan

Code must compile on Qt < 5.11 and Qt >= 5.11. No visual changes should be noticed.

Diff Detail

Repository
R167 Krusader
Lint
Lint Skipped
Unit
Unit Tests Skipped
gengisdave requested review of this revision.Jul 13 2019, 12:03 PM
gengisdave created this revision.

Isn't it be easier to use boundingRect(text).width() as a replacement of horizontalAdvance(text) which does not require Qt 5.11 and is not deprecated?

Thanks in advance for your answer.

@yurchor : I follwed the guidelines from the documentation (https://doc.qt.io/qt-5/qfontmetrics-obsolete.html#width); I haven't tried to see where the difference really is, I think there could be some pixel overlapped or some extra space.

nmel requested changes to this revision.Jul 20 2019, 6:35 AM
nmel added a subscriber: nmel.

I agree we can substitute the width method with the horizontalAdvance, however I propose we use compat.h to eliminate code duplication here.

This revision now requires changes to proceed.Jul 20 2019, 6:35 AM
gengisdave updated this revision to Diff 62219.EditedJul 21 2019, 6:36 PM

Changed with macro, as suggested by @nmel; the code now is cleaner (and QFONTMETRICS_HORIZONTAL_ADVANCE seems a bit long)

I checked the macro expansion with gcc -E

nmel accepted this revision.Jul 22 2019, 6:26 AM

Thanks Davide! It's cleaner and macro name is good. No issues with fonts were discovered when I tested.

krusader/compat.h
40 ↗(On Diff #62219)

(optional) I'd use QFontMetrics::horizontalAdvance(A) and QFontMetrics::width(A) for additional type check.

This revision is now accepted and ready to land.Jul 22 2019, 6:26 AM
asensi accepted this revision.Jul 23 2019, 10:11 PM
asensi added a subscriber: asensi.

(optional) I'd use QFontMetrics::horizontalAdvance(A) and QFontMetrics::width(A) for additional type check.

Even if those changes are not made, the new code works using Kubuntu 18.04, and improves the current situation. So thanks, Davide! (and Nikita and Yuri :-) )

asensi added a subscriber: cfeck.Aug 8 2019, 10:20 PM

An article about it has been published recently:

About deprecation of QFontMetrics::width()
https://kdepepo.wordpress.com/2019/08/05/about-deprecation-of-qfontmetricswidth/

by @cfeck (Christoph Feck, who has commited some changes to the repository of Krusader).

asensi removed a subscriber: cfeck.Aug 9 2019, 5:42 AM
gengisdave added a subscriber: cfeck.EditedAug 12 2019, 7:49 PM

An article about it has been published recently:

About deprecation of QFontMetrics::width()
https://kdepepo.wordpress.com/2019/08/05/about-deprecation-of-qfontmetricswidth/

by @cfeck (Christoph Feck, who has commited some changes to the repository of Krusader).

I also read https://tsdgeos.blogspot.com/2019/07/beware-of-some-of-qt-513-deprecation.html by @aacid. That's why I wanted to wait to test more this patch. In D22504, for example, the changes were closer to a method rename.

While on my tests both methods work fine, I haven't found yet a font which should make the difference.

gengisdave removed subscribers: aacid, cfeck.
nmel added a comment.Aug 18 2019, 6:47 AM

When I see the usage of the method, like

const float fontWidth = (fm.QFONTMETRICS_WIDTH("WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW") - fm.QFONTMETRICS_WIDTH("W")) / 99.0;

or

headerView->resizeSection(KrViewProperties::Ext, QFontMetrics(_viewFont).QFONTMETRICS_WIDTH("tar.bz2  "));
headerView->resizeSection(KrViewProperties::KrPermissions, QFontMetrics(_viewFont).QFONTMETRICS_WIDTH("rwx  "));
headerView->resizeSection(KrViewProperties::Size, QFontMetrics(_viewFont).QFONTMETRICS_WIDTH("9") * 10);

which are all dirty hacks, I doubt we need to care too much about the differences mentioned. This code needs to be refactored properly but it's not a goal of this change. QFONTMETRICS_WIDTH will actually become a good marker of the places that need review if someone will find time and courage to make it right one day...