Details

Reviewers

dfaure
cfeck
apol
bport

Group Reviewers

Frameworks

Commits

R236:e9c92b7e3620: [KFontChooser] Add a checkbox to toggle showing only monospaced fonts

Summary

The use case is applications where the user is usually interested in
monospaced/fixe-width fonts, e.g. using Kate to edit source code.

Remove one redundant ++row.

Depends on D28271

Test Plan

See 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.Mar 25 2020, 10:40 AM

Restricted Application added a project: Frameworks. · View Herald TranscriptMar 25 2020, 10:40 AM

ahmadsamir requested review of this revision.Mar 25 2020, 10:40 AM

Harbormaster completed remote builds in B24203: Diff 78440.Mar 25 2020, 10:40 AM

Depends on D28271

Harbormaster completed remote builds in B24204: Diff 78441.Mar 25 2020, 10:41 AM

ngraham edited the summary of this revision. (Show Details)Mar 25 2020, 4:04 PM

ngraham added a dependency: D28271: [KFontChooser] More code cleanup.

bport requested changes to this revision.Mar 31 2020, 8:03 AM

bport added inline comments.

src/kfontchooser.cpp
346–347	This comment is still valid ?
384	I think there you can connect directly to setEnabled, you don't need a lambda

This revision now requires changes to proceed.Mar 31 2020, 8:03 AM

Don't use a lambda in connetc() where it's not needed

Harbormaster completed remote builds in B24527: Diff 78979.Mar 31 2020, 11:23 AM

ahmadsamir marked an inline comment as done.Mar 31 2020, 11:24 AM

ahmadsamir added inline comments.

src/kfontchooser.cpp
346–347	Yes, that's after setting the font family/style/size list views and the size DoubleSpinBox.

Ping.

bport accepted this revision.Apr 10 2020, 1:04 PM

This revision is now accepted and ready to land.Apr 10 2020, 1:04 PM

Handle the case where KFontChooser::FixedFontsOnly is set, the fixedOnlyCheckBox should be shown as "checked" from the get go.

Harbormaster completed remote builds in B25054: Diff 79823.Apr 11 2020, 11:49 AM

dfaure accepted this revision.Apr 11 2020, 2:10 PM

dfaure added inline comments.

src/kfontchooser.cpp
394	would `true` be more readable? (same for the previous line, actually)

A better location for onlyFixedCheckbox->setChecked() is next to its siblings

Harbormaster completed remote builds in B25073: Diff 79861.Apr 11 2020, 2:50 PM

ahmadsamir marked an inline comment as done.Apr 11 2020, 2:51 PM

ahmadsamir added inline comments.

src/kfontchooser.cpp
394	Indeed; however onlyFixedCheckbox->setChecked(usingFixed) doesn't need to be in that if block at all.

Closed by commit R236:e9c92b7e3620: [KFontChooser] Add a checkbox to toggle showing only monospaced fonts (authored by ahmadsamir). · Explain WhyApr 11 2020, 2:53 PM

This revision was automatically updated to reflect the committed changes.

ahmadsamir marked an inline comment as done.

			Path	Packages
M			src/kfontchooser.cpp (17 lines)
M			tests/kfontchooserdialogtest.cpp (3 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	78440	60828c7		Mar 25 2020, 10:40 AM	★	★
Diff 2	78441	60828c7	Depends on D28271	Mar 25 2020, 10:41 AM	★	★
Diff 3	78979	60828c7	Don't use a lambda in connetc() where it's not needed	Mar 31 2020, 11:23 AM	★	★
Diff 4	79823	11589f5	Handle the case where KFontChooser::FixedFontsOnly is set, the…	Apr 11 2020, 11:49 AM	★	★
Diff 5	79861	11589f5	A better location for onlyFixedCheckbox->setChecked() is next to its siblings	Apr 11 2020, 2:50 PM	★	★
Diff 6	79862	11589f5	R236:e9c92b7e3620d1b7ae51005724fe2acf80e3235a	Apr 11 2020, 2:52 PM	★	★

	Status	Author	Revision
	Closed	ahmadsamir	D28274 [KFontChooser] Add a checkbox to toggle showing only monospaced fonts
	Closed	ahmadsamir	D28271 [KFontChooser] More code cleanup

[KFontChooser] Add a checkbox to toggle showing only monospaced fonts
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 79862

src/kfontchooser.cpp

tests/kfontchooserdialogtest.cpp

[KFontChooser] Add a checkbox to toggle showing only monospaced fontsClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 79862

src/kfontchooser.cpp

tests/kfontchooserdialogtest.cpp

[KFontChooser] Add a checkbox to toggle showing only monospaced fonts
ClosedPublic
Actions

Revision Contents
Changeset List