[KFontChooser] Add new DisplayFlag; modify how flags are used
ClosedPublic

Authored by ahmadsamir on Apr 21 2020, 4:56 PM.

Details

Summary

Add a DisplayFlag to allow users of KFontChooser to disable showing the
checkbox that toggles listing only monospaced fonts. This is useful for
apps that embed KFontChooser, e.g. Konsole that has its own checkbox
and an accompanying "warning" button (in fact the checkbox in KFontChooser
was inspired by the one in Konsole, and we don't want the font widget
there to suffer by having two checkboxes doing the opposite function of
each other).

In ShowDifferences mode, call setEnabled(false) on the list views and co.
after checking "flags & ShowDifferences", using XOR "flags ^ ShowDifferences"
would fail if another flag was set.

Test Plan

See the kfontchooserdialogtest app

Diff Detail

Repository
R236 KWidgetsAddons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ahmadsamir created this revision.Apr 21 2020, 4:56 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 21 2020, 4:56 PM
ahmadsamir requested review of this revision.Apr 21 2020, 4:56 PM
dfaure requested changes to this revision.Apr 22 2020, 1:22 PM
dfaure added inline comments.
src/kfontchooser.h
90

1/2/4 looked like powers of 2, i.e. a flag set.

Using 5 here gives is the same value as FixedFontsOnly | ShowDifferences.

On the other hand it makes sense FixedFontsOnly would automatically hide the checkbox, no? If the app says "fixed fonts only", the user has no choice in the matter.

I'm also confused by the name "No[...]Toggle" and the docu saying this will show a checkbox. Something's off by "not" ;)
Sounds wrong?

This revision now requires changes to proceed.Apr 22 2020, 1:22 PM
ahmadsamir added inline comments.Apr 22 2020, 5:28 PM
src/kfontchooser.h
90

1/2/4 looked like powers of 2, i.e. a flag set.
Using 5 here gives is the same value as FixedFontsOnly | ShowDifferences.

I missed the fact that combining two or more flags would give the same value... will fix.

On the other hand it makes sense FixedFontsOnly would automatically hide the checkbox, no? If the app says "fixed fonts only", the user has no choice in the matter.

Yeah, that makes sense (a special case is Konsole, which embeds KFontChooser and adds a checkbox to "show all fonts", IIRC this is to offer users a way out with some mis-configured fonts... etc).

I'm also confused by the name "No[...]Toggle" and the docu saying this will show a checkbox. Something's off by "not" ;)
Sounds wrong?

I wrote the enum docs, _then_ changed the name of the member to NoFixedOnlyToggle (this way apps can set it if they want but it doesn't have to be set by default, i.e. we always show the box unless told otherwise). So yeah off by "Do not".

  • Use a proper flag set, 0/1/2/4/8, from dfaure
  • When checking a flag is set use bitwise &, not bitwise ^, the latter would fail if another flag was set
  • As per dfaure's suggestion, when FixedFontsOnly is set, don't use the checkbox that toggles/filters fixed fonts
  • Add more use cases to the kfontchooserdialogtest app
ahmadsamir marked 2 inline comments as done.
ahmadsamir retitled this revision from [KFontChooser] Add a DisplayFlag to allow not showing fixedOnly checkbox to [KFontChooser] Add new DisplayFlag; modify how flags are used.
ahmadsamir edited the summary of this revision. (Show Details)

Verbatim

dfaure accepted this revision.Apr 23 2020, 1:36 PM
This revision is now accepted and ready to land.Apr 23 2020, 1:36 PM
dfaure added inline comments.Apr 23 2020, 1:38 PM
src/kfontchooser.h
87

maybe document that FixedFontsOnly implies NoFixedCheckBox?

(as in, the widget will behave as if NoFixedCheckBox was set)

kossebau added inline comments.
src/kfontchooser.h
90

I would add a final ",", so you do not need to touch the line if any further flags are added in the future. Improves git blame display per line :)

ahmadsamir marked an inline comment as done.Apr 23 2020, 2:34 PM
ahmadsamir added inline comments.
src/kfontchooser.h
87

Yes, will do (less surprises for users of the class).

90

Wouldn't that look a bit weird as if something has been removed or missing?

kossebau added inline comments.Apr 23 2020, 2:47 PM
src/kfontchooser.h
90

I might perhaps while getting used to if you are coming from old code habits, but the syntax of C++ has been extra extended to allow this, among others to make patches/diffs less noisy or #if#else around list ends, but many also fancy this for line-by-line git blame improvements.

And the majority of projects I contribute to now does this by default, and no confusion known to me, the eyes get quickly adapted to the pattern, even more as each line now has the same content

I agree, it's now common practice to have a final comma.

ahmadsamir updated this revision to Diff 81007.Apr 23 2020, 3:05 PM
ahmadsamir marked an inline comment as done.

More docs, and a trailing comma in the enum to ease git diff/blame if more flags are added

ahmadsamir marked an inline comment as done.Apr 23 2020, 3:05 PM
This revision was automatically updated to reflect the committed changes.

I forgot to change the commit message about hiding the show only monospaced fonts checkbox when FixedFontsOnly is set... at least the API docs do mention it :/.