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 );
> +};

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 , Bernd Wuebben 
> + *
> + */

@since 5.69

> kfontchooserdialog.h:89
> + */
> +explicit KFontChooserDialog(QWidget *parent = nullptr,
> +const KFontChooser::DisplayFlags  = 
> 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

D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[Ahmad Samir]

ahmadsamir updated this revision to Diff 78153.
ahmadsamir added a comment.


  Tweak docs

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28122?vs=78124=78153

BRANCH
  l-kfontdlg (branched from master)

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

AFFECTED FILES
  docs/pics/kfontchooserdialog.png
  src/CMakeLists.txt
  src/kfontchooserdialog.cpp
  src/kfontchooserdialog.h
  tests/CMakeLists.txt
  tests/kfontchooserdialogtest.cpp

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[Ahmad Samir]

ahmadsamir added a comment.


  
  
  In D28122#631254 , @kossebau wrote:
  
  > Please also add a note in the documentation when to use 
new-name-of-KFontDialog and when QFontDialog, i.e. in which use-cases 
new-name-of-KFontDialog is superior and not using the non-platform one is 
justified.
  
  
  I tried documenting the features, the result is a bit shoddy... :)

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-kfontdlg (branched from master)

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

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[Ahmad Samir]

ahmadsamir updated this revision to Diff 78124.
ahmadsamir added a comment.


  - Rename to KFontChooserDialog to prevent conflict with the one from 
KDELibs4Support
  - Document "features"

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28122?vs=77904=78124

BRANCH
  l-kfontdlg (branched from master)

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

AFFECTED FILES
  docs/pics/kfontchooserdialog.png
  src/CMakeLists.txt
  src/kfontchooserdialog.cpp
  src/kfontchooserdialog.h
  tests/CMakeLists.txt
  tests/kfontchooserdialogtest.cpp

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Please also add a note in the documentation when to use KFontDialog and when 
QFontDialog, i.e. in which use-cases KFontDialog is superior and not using the 
non-platform one is justified.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-kfontdlg (branched from master)

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

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[Ahmad Samir]

ahmadsamir added a comment.


  In D28122#631237 , @kossebau wrote:
  
  > Moving a class (and thus its exported symbols) from a library to another 
one in its public dependency interface should be doable I guess (no actual 
experience in having done that though).
  >
  > But you sadly cannot change/remove exported symbols and the data layout, 
like done here (e.g. by dropping the KDialog intermediate inherited class data).
  >
  > So you will need a new name, KFontDialog cannot be reused.
  
  
  Nearest candidate is KFontChooserDialog, which describes this class, a dialog 
for KFontChooser widget :)

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-kfontdlg (branched from master)

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

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Moving a class (and thus its exported symbols) from a library to another one 
in its public dependency interface should be doable I guess (no actual 
experience in having done that though).
  
  But you sadly cannot change/remove exported symbols and the data layout, like 
done here (e.g. by dropping the KDialog intermediate inherited class data).
  
  So you will need a new name, KFontDialog cannot be reused.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-kfontdlg (branched from master)

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

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[Méven Car]

meven accepted this revision.
meven added a comment.
This revision is now accepted and ready to land.


  Seems good to me.
  Please wait for me review.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-kfontdlg (branched from master)

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

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[Ahmad Samir]

ahmadsamir updated this revision to Diff 77904.
ahmadsamir added a task: T12760: Port/move KFontDialog from KDELibs4Support to 
KWidgetAddons.
ahmadsamir added a comment.


  Link to KF6 task

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28122?vs=77903=77904

BRANCH
  l-kfontdlg (branched from master)

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

AFFECTED FILES
  docs/pics/kfontdialog.png
  src/CMakeLists.txt
  src/kfontchooser.h
  src/kfontdialog.cpp
  src/kfontdialog.h
  tests/CMakeLists.txt
  tests/kfontdialogtest.cpp

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28122: Move/port KFontDialog from KDELibs4Support to KWidgetAddons

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, davidedmundson, cfeck, broulik, ervin, 
meven, bport.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  - Drop relative size bits, seems not that used
  - Use a KFontDialogPrivate class instead of KFontDialog::Private (see: 
https://mail.kde.org/pipermail/kde-frameworks-devel/2015-August/025956.html)
  - Port to QDialog
  
  See https://phabricator.kde.org/D27808 for more details about the reasons
  to make this port.

TEST PLAN
  it builds and kfontdialog works (see test app)

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-kfontdlg (branched from master)

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

AFFECTED FILES
  docs/pics/kfontdialog.png
  src/CMakeLists.txt
  src/kfontchooser.h
  src/kfontdialog.cpp
  src/kfontdialog.h
  tests/CMakeLists.txt
  tests/kfontdialogtest.cpp

To: ahmadsamir, #frameworks, davidedmundson, cfeck, broulik, ervin, meven, bport
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns