D29065: [KFontChooser] Add a DisplayFlag to allow not showing fixedOnly checkbox
ahmadsamir updated this revision to Diff 80988. ahmadsamir added a comment. - 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 REPOSITORY R236 KWidgetsAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29065?vs=80792=80988 BRANCH l-kfontchooser-onlyfixed-display-flag (branched from master) REVISION DETAIL https://phabricator.kde.org/D29065 AFFECTED FILES src/kfontchooser.cpp src/kfontchooser.h tests/kfontchooserdialogtest.cpp To: ahmadsamir, #frameworks, dfaure, cfeck, bport Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29065: [KFontChooser] Add a DisplayFlag to allow not showing fixedOnly checkbox
ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in 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? > 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". REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29065 To: ahmadsamir, #frameworks, dfaure, cfeck, bport Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29065: [KFontChooser] Add a DisplayFlag to allow not showing fixedOnly checkbox
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kfontchooser.h:90 > +ShowDifferences = 4,///< Display the font differences interfaces > +NoFixedOnlyToggle = 5 ///< Show a checkbox to toggle showing only > fixed fonts > }; 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? REPOSITORY R236 KWidgetsAddons REVISION DETAIL https://phabricator.kde.org/D29065 To: ahmadsamir, #frameworks, dfaure, cfeck, bport Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29065: [KFontChooser] Add a DisplayFlag to allow not showing fixedOnly checkbox
ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, dfaure, cfeck, bport. Herald added a project: Frameworks. ahmadsamir requested review of this revision. REVISION SUMMARY This is useful for apps that only want users to select monospaced fonts, and 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). TEST PLAN See the kfontchooserdialogtest app REPOSITORY R236 KWidgetsAddons BRANCH l-kfontchooser-onlyfixed-display-flag (branched from master) REVISION DETAIL https://phabricator.kde.org/D29065 AFFECTED FILES src/kfontchooser.cpp src/kfontchooser.h tests/kfontchooserdialogtest.cpp To: ahmadsamir, #frameworks, dfaure, cfeck, bport Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
