D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfontchooserdialog.cpp:38
> +
> +    void stripRegularStyleName(QFont &font);
> +};

Should be static. Even file-static (move the implementation further up in the file).

> kfontchooserdialog.cpp:81
> +    KFontChooserDialog dlg(parent, flags | KFontChooser::ShowDifferences, QStringList());
> +    dlg.setModal(true);
> +    dlg.setObjectName(QStringLiteral("Font Selector"));

not needed, exec() makes it modal anyway

> kfontchooserdialog.cpp:85
> +
> +    int result = dlg.exec();
> +    if (result == Accepted) {

const

> kfontchooserdialog.cpp:98
> +    KFontChooserDialog dlg(parent, flags, QStringList());
> +    dlg.setModal(true);
> +    dlg.setObjectName(QStringLiteral("Font Selector"));

same

> kfontchooserdialog.h:70
> + * @author Preston Brown <pbrown at kde.org>, Bernd Wuebben <wuebben at kde.org>
> + *
> + */

@since 5.69

> kfontchooserdialog.h:89
> +     */
> +    explicit KFontChooserDialog(QWidget *parent = nullptr,
> +                                const KFontChooser::DisplayFlags &flags = KFontChooser::NoDisplayFlags,

The usual way in Qt API is that the parent is the last argument, rather than the first.

But for this to be convenient, the fontlist argument would have to move to a setFontList() method, and I see that KFontChooser itself doesn't support that. So, I won't block for this reason, but if you're aiming for perfection, you know what to do... ;)

Then flags can either be a setter, or a constructor overload:

  ctor(QWidget*)
  ctor(flags, QWidget*)

like QFontDialog or QLineEdit or many others do, in order to keep the parent pointer as the last argument.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D28122

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport, dfaure
Cc: dfaure, kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200322/c7fe929a/attachment-0001.html>