D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

[Ahmad Samir]

ahmadsamir added a comment.


  In D28774#676000 , @volkov wrote:
  
  > Why not add support for `QPlatformTheme::FontDialog` in 
`KdePlatformTheme::createPlatformDialogHelper()`?
  
  
  Sorry about the late reply; I had looked into the platformtheme integration 
stuff, but at the time I didn't think it was a viable solution (nor was it easy 
for me to port); see https://phabricator.kde.org/D27808 for more details.

REPOSITORY
  R236 KWidgetsAddons

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

To: ahmadsamir, #frameworks, dfaure, cfeck
Cc: volkov, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

[Alexander Volkov]

volkov added a comment.


  Why not add support for `QPlatformTheme::FontDialog` in 
`KdePlatformTheme::createPlatformDialogHelper()`?

REPOSITORY
  R236 KWidgetsAddons

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

To: ahmadsamir, #frameworks, dfaure, cfeck
Cc: volkov, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

[Ahmad Samir]

ahmadsamir added a comment.


  In D28774#647157 , @dfaure wrote:
  
  > I mean, the user can press cancel, that's when QColorDialog returns an 
invalid color.
  >  If there was a concept like an invalid font we could do the same here, but 
there's no such concept in QFont.
  >  So yep, no better way.
  
  
  Looking at the code, if the user presses cancel, "theFont" is returned 
unchanged; seems pretty good to me:
  
int KFontChooserDialog::getFont(QFont , const 
KFontChooser::DisplayFlags , QWidget *parent)
{
KFontChooserDialog dlg(flags, parent);
dlg.setObjectName(QStringLiteral("Font Selector"));
dlg.setFont(theFont, flags & KFontChooser::FixedFontsOnly);

const int result = dlg.exec();
if (result == Accepted) {
theFont = dlg.d->chooser->font();
stripRegularStyleName(theFont);
}
return result;
}
  
  QFontDialog::getFont() returns a QFont, and one may pass a bool * to check 
the dialog result code:
  QFont QFontDialog::getFont(bool *ok, const QFont , QWidget *parent, 
const QString , FontDialogOptions options)
  
  which is standard Qt behaviour from other dialogs/widgets that handle user 
yes/no use cases, IIRC.
  
  The KFontChooer* approach is more "economical", the "initial" font and the 
font you get from the dialog if you press OK, are the same object, and the 
function returns the dialog result code. As some wise old builder once said 
"there's no right or wrong way to lay bricks, as long as it works".

REPOSITORY
  R236 KWidgetsAddons

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

To: ahmadsamir, #frameworks, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

[David Faure]

dfaure added a comment.


  I mean, the user can press cancel, that's when QColorDialog returns an 
invalid color.
  If there was a concept like an invalid font we could do the same here, but 
there's no such concept in QFont.
  So yep, no better way.

REPOSITORY
  R236 KWidgetsAddons

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

To: ahmadsamir, #frameworks, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kfontrequester.cpp:188
> Plus we can't end up with a non-valid QFont, the dialog is filled from 
> QFontDataBase; so unless the user went and uninstalled the font while the 
> dialog was open, it should be OK. Also, font substitution will take care of a 
> font-gone-MIA (I am sure of that on Linux, and most likely other OS's have 
> similar solutions).
> 
> I was curious so I looked at QColorDialog::getColor() and it looks like they 
> changed it (or you meant getRgba?).

And yes, it's not obvious that m_selFont is modified, I had to go read the API 
docs KFontChooser multiple times... but I got used to it.

REPOSITORY
  R236 KWidgetsAddons

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

To: ahmadsamir, #frameworks, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in kfontrequester.cpp:188
> It's really not obvious from the above that this modifies m_selFont.
> 
> But I see QFont has no isValid() so we can't adopt an API like 
> QColorDialog::getColor (plus KFontChooserDialog has been released now).
> 
> Oh well.

Plus we can't end up with a non-valid QFont, the dialog is filled from 
QFontDataBase; so unless the user went and uninstalled the font while the 
dialog was open, it should be OK. Also, font substitution will take care of a 
font-gone-MIA (I am sure of that on Linux, and most likely other OS's have 
similar solutions).

I was curious so I looked at QColorDialog::getColor() and it looks like they 
changed it (or you meant getRgba?).

REPOSITORY
  R236 KWidgetsAddons

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

To: ahmadsamir, #frameworks, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

[Ahmad Samir]

This revision was automatically updated to reflect the committed changes.
Closed by commit R236:56090c8f380e: [KFontRequester] Port from QFontDialog to 
KFontChooserDialog (authored by ahmadsamir).

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28774?vs=79932=80007

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

AFFECTED FILES
  src/kfontrequester.cpp
  src/kfontrequester.h

To: ahmadsamir, #frameworks, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

[David Faure]

dfaure accepted this revision.
dfaure added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> kfontrequester.cpp:188
> +
> +const int result = KFontChooserDialog::getFont(m_selFont, flags, 
> q->parentWidget());
>  

It's really not obvious from the above that this modifies m_selFont.

But I see QFont has no isValid() so we can't adopt an API like 
QColorDialog::getColor (plus KFontChooserDialog has been released now).

Oh well.

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-kfontreq (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28774: [KFontRequester] Port from QFontDialog to KFontChooserDialog

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure, cfeck.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

TEST PLAN
  The kfontrequestertest app still works

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  l-kfontreq (branched from master)

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

AFFECTED FILES
  src/kfontrequester.cpp
  src/kfontrequester.h

To: ahmadsamir, #frameworks, dfaure, cfeck
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns