Copy KFontDialog from KDELibs4Support to KWidgetAddons, now KFontChooserDialog
ClosedPublic

Authored by ahmadsamir on Mar 18 2020, 12:02 PM.

Details

Summary
  • Drop relative size bits, seems not that useful or widely 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
  • Drop the fontList parameter in the ctor, it's not useful, 99% of the times users of this class called the static getFont() methods
  • Make the parent QWidget * the last param. in the ctor, a la Qt API
  • Update the screenshot of the font dialog

See https://phabricator.kde.org/D27808 for more details.

Test Plan

it builds and kfontchooserdialog works (see test app)

Diff Detail

Repository
R236 KWidgetsAddons
Branch
l-kfontdlg (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24000
Build 24018: arc lint + arc unit
ahmadsamir created this revision.Mar 18 2020, 12:02 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 18 2020, 12:02 PM
ahmadsamir requested review of this revision.Mar 18 2020, 12:02 PM
meven accepted this revision.Mar 20 2020, 11:04 AM

Seems good to me.
Please wait for me review.

This revision is now accepted and ready to land.Mar 20 2020, 11:04 AM

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.

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 :)

kossebau added a comment.EditedMar 20 2020, 3:45 PM

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.

ahmadsamir updated this revision to Diff 78124.Mar 20 2020, 8:18 PM
  • Rename to KFontChooserDialog to prevent conflict with the one from KDELibs4Support
  • Document "features"

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... :)

dfaure requested changes to this revision.Mar 22 2020, 12:54 PM
dfaure added a subscriber: dfaure.
dfaure added inline comments.
src/kfontchooserdialog.cpp
39

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

82

not needed, exec() makes it modal anyway

86

const

99

same

src/kfontchooserdialog.h
71

@since 5.69

90

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.

This revision now requires changes to proceed.Mar 22 2020, 12:54 PM
ahmadsamir updated this revision to Diff 78241.Mar 22 2020, 5:20 PM
ahmadsamir retitled this revision from Move/port KFontDialog from KDELibs4Support to KWidgetAddons to Copy KFontDialog from KDELibs4Support to KWidgetAddons, now KFontChooserDialog.
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir edited the test plan for this revision. (Show Details)
ahmadsamir removed a reviewer: dfaure.
ahmadsamir removed subscribers: dfaure, kossebau.

Address comments:

  • Drop the fontList parameter in the ctor, it's not useful, 99% of the times users of this class call the static getFont() methods
  • Make the parent QWidget * the last param. in the ctor, a la Qt API
  • Change stripRegularStyleName() to be file-static
  • Add @since
  • Don't call setModal(true), exec() takes care of that
  • More const
This revision is now accepted and ready to land.Mar 22 2020, 5:20 PM
ahmadsamir marked 6 inline comments as done.Mar 22 2020, 5:30 PM
ahmadsamir added inline comments.
src/kfontchooserdialog.h
90

Done. And I removed the fontList parameter altogether, it wasn't used, and even QFontDialog doesn't have it. I think the need to set a specific font list to display in a font selection dialog is pretty niche, I couldn't find a single usage of it in lxr (even in stable-qt4 branch).

Since the ctor parameters both have default values, I won't overload the ctor.

ahmadsamir updated this revision to Diff 78249.Mar 22 2020, 7:09 PM
ahmadsamir marked an inline comment as done.
ahmadsamir edited the summary of this revision. (Show Details)

Update the screenshot in the docs, Breeze style/colors

dfaure accepted this revision.Mar 22 2020, 7:54 PM
dfaure added inline comments.
src/kfontchooserdialog.h
82

Do @param need to be in order of the arguments? (I'm not sure)

90

Good call. I can't think of any use case for this.

Maybe choosing an emoji font? But then how to know which of the installed fonts qualify... Yeah let's wait until there's actually a use case, if ever.

ahmadsamir updated this revision to Diff 78251.Mar 22 2020, 8:38 PM

@param should have the same order as the in the ctor declaration (patterns make life slightly easier)

ahmadsamir marked 3 inline comments as done.Mar 22 2020, 8:45 PM
ahmadsamir added inline comments.
src/kfontchooserdialog.h
82

They should be, it's more logical.

90

emoji fonts... that would need a whole rewrite in a lot of places, starting at QFontDatabase, so we'd have isSmoothlyScalable and isEmojiScalable, and maybe isEmojiMonospace, just kidding.

(what's wrong with ":D" ":)" ";)" ":(" "\o/"? all fonts support them, and they look cool :D).

dfaure accepted this revision.Mar 22 2020, 8:56 PM
This revision was automatically updated to reflect the committed changes.
ahmadsamir marked 2 inline comments as done.
kossebau added inline comments.Mar 23 2020, 5:05 AM
src/kfontchooserdialog.cpp
98

Is having dialog objects on the stack a good idea?

What if the application gets closed while the dialog is open, would that not crash?

At least at some point people thought it was not a good idea, see https://blogs.kde.org/node/3919

meven added inline comments.Mar 24 2020, 10:53 AM
src/kfontchooserdialog.cpp
94

Shouldn't this be not commented

111

Shouldn't this be not commented

ahmadsamir added inline comments.Mar 24 2020, 11:07 AM
src/kfontchooserdialog.cpp
94

IIUC, if a function is declared static inside the class body (e.g. in the header file), the static keyword can't be repeated when it's defined outside the class body.

98

hmmm..... dfaure knows more about exec() and event loops more than me. @dfaure WDYT?

dfaure added inline comments.Mar 24 2020, 8:22 PM
src/kfontchooserdialog.cpp
98

The theoretical answer is yes, this would crash. But note that the user cannot just close the application by clicking somewhere while a modal dialog is up. This requires much more subtle interaction like a DBus call.

There are a million modal dialogs out there created on the stack, but sure, if you want to be pedantic, use QPointer, new (and manual delete, for lack of a proper smart pointer for this)...

(or we could set WA_DeleteOnClose instead of the manual delete? I just added a comment to the blog about that)

ahmadsamir added inline comments.Mar 26 2020, 5:36 PM
src/kfontchooserdialog.cpp
98

OK, given how many modal dialogs are out there, I'll leave it as is.