Re: Review Request: Allow setting a custom font list in KFontComboBox

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106425/#review19253
---


This review has been submitted with commit 
5c677ceb397de9b40a050489d1890ac6c9be17d9 by Christoph Feck to branch KDE/4.9.

- Commit Hook


On Sept. 11, 2012, 7:33 p.m., Christoph Feck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106425/
> ---
> 
> (Updated Sept. 11, 2012, 7:33 p.m.)
> 
> 
> Review request for kdelibs and Chusslove Illich.
> 
> 
> Description
> ---
> 
> KFontAction has a constructor to filter the font list by certain criteria. 
> When it creates a KFontComboBox to allow the user to interact with it, this 
> box always shows all fonts.
> 
> Instead of adding a "setFontFilterCriteria()" method, I opted for adding a 
> "setFontList()" method, for the following reasons:
> - it does not have to query the Qt font database twice
> - it is more flexible (you could, for example, filter by size)
> 
> @since: TBD has to be decided:
> - 4.9.x, because it allows to fix bug 83212 and bug 306625
> - 4.10, because it adds new API
> - KF5, because it adds new API
> - keep it private, only to be called by KFontAction
> 
> Note that this commit alone does NOT fix the bugs, but adds the required API.
> 
> 
> This addresses bugs 83212 and 306625.
> http://bugs.kde.org/show_bug.cgi?id=83212
> http://bugs.kde.org/show_bug.cgi?id=306625
> 
> 
> Diffs
> -
> 
>   kdeui/fonts/kfontcombobox.h de3cb16 
>   kdeui/fonts/kfontcombobox.cpp 0ded6fb 
> 
> Diff: http://git.reviewboard.kde.org/r/106425/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request: Allow setting a custom font list in KFontComboBox

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106425/#review19010
---

Ship it!


New API to fix bugs is fine in 4.9.x, as long as you don't use it in other 
modules immediately. If you say this is for KFontAction, then no problem.


kdeui/fonts/kfontcombobox.h


contructed -> constructed


- David Faure


On Sept. 11, 2012, 7:33 p.m., Christoph Feck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106425/
> ---
> 
> (Updated Sept. 11, 2012, 7:33 p.m.)
> 
> 
> Review request for kdelibs and Chusslove Illich.
> 
> 
> Description
> ---
> 
> KFontAction has a constructor to filter the font list by certain criteria. 
> When it creates a KFontComboBox to allow the user to interact with it, this 
> box always shows all fonts.
> 
> Instead of adding a "setFontFilterCriteria()" method, I opted for adding a 
> "setFontList()" method, for the following reasons:
> - it does not have to query the Qt font database twice
> - it is more flexible (you could, for example, filter by size)
> 
> @since: TBD has to be decided:
> - 4.9.x, because it allows to fix bug 83212 and bug 306625
> - 4.10, because it adds new API
> - KF5, because it adds new API
> - keep it private, only to be called by KFontAction
> 
> Note that this commit alone does NOT fix the bugs, but adds the required API.
> 
> 
> This addresses bugs 83212 and 306625.
> http://bugs.kde.org/show_bug.cgi?id=83212
> http://bugs.kde.org/show_bug.cgi?id=306625
> 
> 
> Diffs
> -
> 
>   kdeui/fonts/kfontcombobox.h de3cb16 
>   kdeui/fonts/kfontcombobox.cpp 0ded6fb 
> 
> Diff: http://git.reviewboard.kde.org/r/106425/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request: Allow setting a custom font list in KFontComboBox

[Christoph Feck]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106425/
---

(Updated Sept. 11, 2012, 7:33 p.m.)


Review request for kdelibs and Chusslove Illich.


Changes
---

typos


Description (updated)
---

KFontAction has a constructor to filter the font list by certain criteria. When 
it creates a KFontComboBox to allow the user to interact with it, this box 
always shows all fonts.

Instead of adding a "setFontFilterCriteria()" method, I opted for adding a 
"setFontList()" method, for the following reasons:
- it does not have to query the Qt font database twice
- it is more flexible (you could, for example, filter by size)

@since: TBD has to be decided:
- 4.9.x, because it allows to fix bug 83212 and bug 306625
- 4.10, because it adds new API
- KF5, because it adds new API
- keep it private, only to be called by KFontAction

Note that this commit alone does NOT fix the bugs, but adds the required API.


This addresses bugs 83212 and 306625.
http://bugs.kde.org/show_bug.cgi?id=83212
http://bugs.kde.org/show_bug.cgi?id=306625


Diffs
-

  kdeui/fonts/kfontcombobox.h de3cb16 
  kdeui/fonts/kfontcombobox.cpp 0ded6fb 

Diff: http://git.reviewboard.kde.org/r/106425/diff/


Testing
---


Thanks,

Christoph Feck

Review Request: Allow setting a custom font list in KFontComboBox

[Christoph Feck]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106425/
---

Review request for kdelibs and Chusslove Illich.


Description
---

KFontAction has a constructor to filter the font list by certain criteria. When 
it creates a KFontComboBox to allow the user to interact with it, this box 
always shows all fonts.

Instead of adding a "setFontFilterCriteria()" method, I opted for adding a 
"setFontList()" method, for the following reasons:
- it does not have to Qt database twice
- it is more flexible (you could, for example, filter by size)

@since: TBD has to be decided:
- 4.9.x, because it allows to fix bug 83212 and bug 306625
- 4.10, because it adds new API
- KF5, because it adds new API
- keep it private, only to be called by KFontAction

Note that this commit alone does NOT fix the bugs, but adds the required API.


This addresses bugs 83212 and 306625.
http://bugs.kde.org/show_bug.cgi?id=83212
http://bugs.kde.org/show_bug.cgi?id=306625


Diffs
-

  kdeui/fonts/kfontcombobox.h de3cb16 
  kdeui/fonts/kfontcombobox.cpp 0ded6fb 

Diff: http://git.reviewboard.kde.org/r/106425/diff/


Testing
---


Thanks,

Christoph Feck