Use QTextFormat::TextUnderlineStyle instead of QTextFormat::FontUnderline
ClosedPublic

Authored by ahmadsamir on Feb 19 2019, 3:08 PM.

Details

Summary

QTextFormat::FontUnderline has been deprecated upstream for a long time.

It seems that for some time setting the underline attribute for syntax
highlighting has been broken, i.e. a user can't set it via the config
widget and setKateExtendedAttributeList always sets it to false. The
latter meant that when the user applied any changes that would write
katesyntaxhighlightingrc, the underline attribute for syntax HL would
unconditionally be set to false which leads to some weird situations,
(e.g. bug 399278 where un/setting "highlight trailing spaces" causes
katesyntaxhighlightingrc to get written to disk which leads to
Markdown:linebreak losing its underline attribute for good).

BUG: 399278
FIXED-IN: 5.57

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ahmadsamir created this revision.Feb 19 2019, 3:08 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptFeb 19 2019, 3:08 PM
ahmadsamir requested review of this revision.Feb 19 2019, 3:08 PM

This patch looks good to me, even though I cannot reproduce the issue following the steps in https://bugs.kde.org/show_bug.cgi?id=399278.

But yes, the Qt documentation says to use TextUnderlineStyle instead of FontUnderline. The implementation of fontUnderline() indeed also first tries to use TextUnderlineStyle, see: https://code.woboq.org/qt5/qtbase/src/gui/text/qtextformat.cpp.html#_ZNK15QTextCharFormat13fontUnderlineEv

So from my side this change is OK, but probably before committing this should be tested again.

Btw, searching in lxr.kde.org for FontUnderline reveals some more hits: https://lxr.kde.org/ident?_i=FontUnderline&_remember=1

This patch looks good to me, even though I cannot reproduce the issue following the steps in https://bugs.kde.org/show_bug.cgi?id=399278.

Another way to test, try setting the underline attribute for any item in the syntax highlighting config widget, hitting apply unsets it again.

It might be dependent on how qt is compiled, I am using Fedora, the OP in the BR is using Ubuntu. But I didn't look into that too much, since upstream docs clearly say FontUnderline is deprecated (and it looks like it's been deprecated since qtbase was split from monolithic qt git repo).

But yes, the Qt documentation says to use TextUnderlineStyle instead of FontUnderline. The implementation of fontUnderline() indeed also first tries to use TextUnderlineStyle, see: https://code.woboq.org/qt5/qtbase/src/gui/text/qtextformat.cpp.html#_ZNK15QTextCharFormat13fontUnderlineEv

Yes, I was puzzled by that too; if FontUnderline is still supported in upstream code, why doesn't it work now...

So from my side this change is OK, but probably before committing this should be tested again.

[...]

Btw, searching in lxr.kde.org for FontUnderline reveals some more hits: https://lxr.kde.org/ident?_i=FontUnderline&_remember=1

OK, I'll try and get them all. :)

cullmann accepted this revision.Feb 24 2019, 5:04 PM

Given the docs explicitly state one shall use the new enum value, I think this should go in.

This revision is now accepted and ready to land.Feb 24 2019, 5:04 PM
This revision was automatically updated to reflect the committed changes.

! In D19161#415702, @dhaumann wrote:
Btw, searching in lxr.kde.org for FontUnderline reveals some more hits: https://lxr.kde.org/ident?_i=FontUnderline&_remember=1

Turns out these are false positives; I checked kdev-python and calligra, and there is no trace of QTextFormat::FontUnderline.