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 <[email protected]>, Bernd Wuebben <[email protected]>
> + *
> + */
@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
