DevHeads.net

Review Request 117957: kcm_fonts: correctly restore default configuration values

Review request for kde-workspace.

Bugs: 324728
<a href="http://bugs.kde.org/show_bug.cgi?id=324728" title="http://bugs.kde.org/show_bug.cgi?id=324728">http://bugs.kde.org/show_bug.cgi?id=324728</a>

Repository: kde-workspace

Description
When restoring default configuration:
- Disable "Exclude range checkbox"
- Enable/Disable "Configure..." button accordingly to antialias setting

Diffs
kcontrol/fonts/fonts.cpp cf21cb1

Diff: <a href="https://git.reviewboard.kde.org/r/117957/diff/" title="https://git.reviewboard.kde.org/r/117957/diff/">https://git.reviewboard.kde.org/r/117957/diff/</a>

Testing
Tested it compiles and works as expected

Thanks,

Andrea Iacovitti

Comments

Re: Review Request 117957: kcm_fonts: correctly restore default

By Andrea Iacovitti at 05/17/2014 - 19:38

(Updated May 18, 2014, 12:38 a.m.)

Review request for kde-workspace.

Changes
Changed the combobox connection to currentIndexChanged signal as Thomas correctly suggested, thanks.

Bugs: 324728
<a href="http://bugs.kde.org/show_bug.cgi?id=324728" title="http://bugs.kde.org/show_bug.cgi?id=324728">http://bugs.kde.org/show_bug.cgi?id=324728</a>

Repository: kde-workspace

Description
When restoring default configuration:
- Disable "Exclude range checkbox"
- Enable/Disable "Configure..." button accordingly to antialias setting

Diffs (updated)
kcontrol/fonts/fonts.cpp cf21cb1

Diff: <a href="https://git.reviewboard.kde.org/r/117957/diff/" title="https://git.reviewboard.kde.org/r/117957/diff/">https://git.reviewboard.kde.org/r/117957/diff/</a>

Testing
Tested it compiles and works as expected

Thanks,

Andrea Iacovitti

Re: Review Request 117957: kcm_fonts: correctly restore default

By Andrea Iacovitti at 07/06/2014 - 05:28

(Updated Lug. 6, 2014, 10:28 a.m.)

Review request for kde-workspace.

Changes
Fixed a typo in comment

Bugs: 324728
<a href="http://bugs.kde.org/show_bug.cgi?id=324728" title="http://bugs.kde.org/show_bug.cgi?id=324728">http://bugs.kde.org/show_bug.cgi?id=324728</a>

Repository: kde-workspace

Description (updated)
When restoring default configuration:
- Disable "Exclude range checkbox"
- Enable/Disable "Configure..." button accordingly to antialias setting

Diffs (updated)
kcontrol/fonts/fonts.cpp cf21cb1

Diff: <a href="https://git.reviewboard.kde.org/r/117957/diff/" title="https://git.reviewboard.kde.org/r/117957/diff/">https://git.reviewboard.kde.org/r/117957/diff/</a>

Testing
Tested it compiles and works as expected

Thanks,

Andrea Iacovitti

Re: Review Request 117957: kcm_fonts: correctly restore default

By Andrea Iacovitti at 07/09/2014 - 08:06

(Updated Lug. 9, 2014, 1:06 p.m.)

Review request for kde-workspace.

Changes
Updated the patch based on David's suggestion

Bugs: 324728
<a href="http://bugs.kde.org/show_bug.cgi?id=324728" title="http://bugs.kde.org/show_bug.cgi?id=324728">http://bugs.kde.org/show_bug.cgi?id=324728</a>

Repository: kde-workspace

Description
When restoring default configuration:
- Disable "Exclude range checkbox"
- Enable/Disable "Configure..." button accordingly to antialias setting

Diffs (updated)
kcontrol/fonts/fonts.cpp cf21cb1

Diff: <a href="https://git.reviewboard.kde.org/r/117957/diff/" title="https://git.reviewboard.kde.org/r/117957/diff/">https://git.reviewboard.kde.org/r/117957/diff/</a>

Testing
Tested it compiles and works as expected

Thanks,

Andrea Iacovitti

Re: Review Request 117957: kcm_fonts: correctly restore default

By Andrea Iacovitti at 07/09/2014 - 14:39

(Updated July 9, 2014, 7:39 p.m.)

Status
This change has been marked as submitted.

Review request for kde-workspace.

Bugs: 324728
<a href="http://bugs.kde.org/show_bug.cgi?id=324728" title="http://bugs.kde.org/show_bug.cgi?id=324728">http://bugs.kde.org/show_bug.cgi?id=324728</a>

Repository: kde-workspace

Description
When restoring default configuration:
- Disable "Exclude range checkbox"
- Enable/Disable "Configure..." button accordingly to antialias setting

Diffs
kcontrol/fonts/fonts.cpp cf21cb1

Diff: <a href="https://git.reviewboard.kde.org/r/117957/diff/" title="https://git.reviewboard.kde.org/r/117957/diff/">https://git.reviewboard.kde.org/r/117957/diff/</a>

Testing
Tested it compiles and works as expected

Thanks,

Andrea Iacovitti

Re: Review Request 117957: kcm_fonts: correctly restore default

By David Faure at 07/09/2014 - 03:51

Ship it!

kcontrol/fonts/fonts.cpp
<https://git.reviewboard.kde.org/r/117957/#comment43133>

Instead of this "hack", I would personally call the slot at the end of the constructor, and let load() call it again if the loaded value is different from 0 (via the connect to currentIndexChanged).

But I guess that's a matter of taste (-1 looks hackish to me -- but indeed it's documented as a way to ensure no current index).
So, no veto.

- David Faure

On July 6, 2014, 10:28 a.m., Andrea Iacovitti wrote:

Re: Review Request 117957: kcm_fonts: correctly restore default

By Andrea Iacovitti at 07/09/2014 - 03:26

If there are no objections or other suggestions it would be good to have the bug fixed in 4.11.11 release :)

- Andrea Iacovitti

On Lug. 6, 2014, 10:28 a.m., Andrea Iacovitti wrote:

Re: Review Request 117957: kcm_fonts: correctly restore default

By Andrea Iacovitti at 07/15/2014 - 09:49

Unfortunately it seems they are different issues. In particular bug 244857 (i marked 325868 as aduplicate of this one) is about the inability to reset to system default font configuration. There is a TODO comment in fonts.cpp code that mentions this issue "TODO: With AASystem the changes already made by this module should be reverted somehow." (line 787)

- Andrea

On July 9, 2014, 7:39 p.m., Andrea Iacovitti wrote:

Re: Review Request 117957: kcm_fonts: correctly restore default

By Christoph Feck at 07/10/2014 - 17:58

Thanks Andrea. Could you please check if this also affects bug 244857, bug 325868, or bug 326971 ? Their descriptions all seem to be related.

- Christoph

On July 9, 2014, 7:39 p.m., Andrea Iacovitti wrote:

Re: Review Request 117957: kcm_fonts: correctly restore default

By Sebastian =?utf... at 05/19/2014 - 04:30

This will likely need a backport to Plasma 5. The code there has moved to the plasma-desktop repo, under kcms/fonts.

- Sebastian Kügler

On May 18, 2014, 12:38 a.m., Andrea Iacovitti wrote:

Re: Review Request 117957: kcm_fonts: correctly restore default

By Andrea Iacovitti at 07/06/2014 - 05:28

ok

- Andrea

On Mag. 18, 2014, 12:38 a.m., Andrea Iacovitti wrote:

Re: Review Request 117957: kcm_fonts: correctly restore default

By =?utf-8?Q?Thoma... at 05/17/2014 - 09:43

kcontrol/fonts/fonts.cpp
<https://git.reviewboard.kde.org/r/117957/#comment40398>

this seems wrong - the combobox is connected to some wrapper that sets the button state (dunno the code at all) but by the "activated" signal, where it should be the "currentIndexChanged" signal, since it should NOT be called when the user activates even the present entry but certainly should whenever the entry changes for whatever reason (config value re-read, defaults applied, whatever)

- Thomas Lübking

On May 2, 2014, 7:59 p.m., Andrea Iacovitti wrote:

Re: Review Request 117957: kcm_fonts: correctly restore default

By Andrea Iacovitti at 05/17/2014 - 03:58

ping

- Andrea Iacovitti

On May 2, 2014, 7:59 p.m., Andrea Iacovitti wrote: