DevHeads.net

Review Request: fix 234407 - set minimum width for system settings icon view

Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu.

Description
have a minimum width for system settings icon view depends on font.

This fontHeight * 6 heuristic value works all languages. CJK character is usually square so this roughly means 6 CJK character, and 12 latin letter, which will always look good.

And to fengchao, sorry if you feel I steal your job.. but I can't stop but self since I think I can finish it in 10 minutes..

This addresses bug 234407.
<a href="http://bugs.kde.org/show_bug.cgi?id=234407" title="http://bugs.kde.org/show_bug.cgi?id=234407">http://bugs.kde.org/show_bug.cgi?id=234407</a>

Diffs
systemsettings/icons/CMakeLists.txt 0830dd7
systemsettings/icons/FileItemDelegate.h PRE-CREATION
systemsettings/icons/FileItemDelegate.cpp PRE-CREATION
systemsettings/icons/IconMode.cpp 37cfc4b

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

Testing
see screenshot.

Screenshots
english after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1014/" title="http://git.reviewboard.kde.org/r/108328/s/1014/">http://git.reviewboard.kde.org/r/108328/s/1014/</a>
chinese after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1015/" title="http://git.reviewboard.kde.org/r/108328/s/1015/">http://git.reviewboard.kde.org/r/108328/s/1015/</a>
chinese before change
<a href="http://git.reviewboard.kde.org/r/108328/s/1016/" title="http://git.reviewboard.kde.org/r/108328/s/1016/">http://git.reviewboard.kde.org/r/108328/s/1016/</a>
spanish after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1017/" title="http://git.reviewboard.kde.org/r/108328/s/1017/">http://git.reviewboard.kde.org/r/108328/s/1017/</a>

Thanks,

Xuetian Weng

Comments

Re: Review Request: fix 234407 - set minimum width for system se

By Weng Xuetian at 01/10/2013 - 13:09

(Updated Jan. 10, 2013, 6:09 p.m.)

Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu.

Description (updated)
Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?)

This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good.

To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes..

This addresses bug 234407.
<a href="http://bugs.kde.org/show_bug.cgi?id=234407" title="http://bugs.kde.org/show_bug.cgi?id=234407">http://bugs.kde.org/show_bug.cgi?id=234407</a>

Diffs
systemsettings/icons/CMakeLists.txt 0830dd7
systemsettings/icons/FileItemDelegate.h PRE-CREATION
systemsettings/icons/FileItemDelegate.cpp PRE-CREATION
systemsettings/icons/IconMode.cpp 37cfc4b

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

Testing
see screenshot.

Screenshots
english after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1014/" title="http://git.reviewboard.kde.org/r/108328/s/1014/">http://git.reviewboard.kde.org/r/108328/s/1014/</a>
chinese after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1015/" title="http://git.reviewboard.kde.org/r/108328/s/1015/">http://git.reviewboard.kde.org/r/108328/s/1015/</a>
chinese before change
<a href="http://git.reviewboard.kde.org/r/108328/s/1016/" title="http://git.reviewboard.kde.org/r/108328/s/1016/">http://git.reviewboard.kde.org/r/108328/s/1016/</a>
spanish after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1017/" title="http://git.reviewboard.kde.org/r/108328/s/1017/">http://git.reviewboard.kde.org/r/108328/s/1017/</a>

Thanks,

Xuetian Weng

Re: Review Request: fix 234407 - set minimum width for system se

By Weng Xuetian at 01/10/2013 - 15:47

(Updated Jan. 10, 2013, 8:47 p.m.)

Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu.

Description
Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?)

This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good.

To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes..

This addresses bug 234407.
<a href="http://bugs.kde.org/show_bug.cgi?id=234407" title="http://bugs.kde.org/show_bug.cgi?id=234407">http://bugs.kde.org/show_bug.cgi?id=234407</a>

Diffs (updated)
systemsettings/icons/CMakeLists.txt 0830dd7
systemsettings/icons/FileItemDelegate.h PRE-CREATION
systemsettings/icons/FileItemDelegate.cpp PRE-CREATION
systemsettings/icons/IconMode.cpp 37cfc4b

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

Testing
see screenshot.

Screenshots
english after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1014/" title="http://git.reviewboard.kde.org/r/108328/s/1014/">http://git.reviewboard.kde.org/r/108328/s/1014/</a>
chinese after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1015/" title="http://git.reviewboard.kde.org/r/108328/s/1015/">http://git.reviewboard.kde.org/r/108328/s/1015/</a>
chinese before change
<a href="http://git.reviewboard.kde.org/r/108328/s/1016/" title="http://git.reviewboard.kde.org/r/108328/s/1016/">http://git.reviewboard.kde.org/r/108328/s/1016/</a>
spanish after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1017/" title="http://git.reviewboard.kde.org/r/108328/s/1017/">http://git.reviewboard.kde.org/r/108328/s/1017/</a>

Thanks,

Xuetian Weng

Re: Review Request: fix 234407 - set minimum width for system se

By Weng Xuetian at 01/10/2013 - 15:48

(Updated Jan. 10, 2013, 8:48 p.m.)

Review request for kde-workspace, Ben Cooksley, Thomas Lübking, Chao Feng, and Yichao Yu.

Changes
add spanish screenshot before apply the patch.

Description
Set minimum width for system settings icon view item depending on font. Since KFileItemDelegate doesn't provides setMinimumSize, we make a sub-class that can have a minimumSize. (Maybe should be add to kdelibs in the future?)

This fontHeight * 6 heuristic value works for all languages. CJK character is usually square (width = height) so this roughly means 6 CJK character, and 12 latin letter (height = width * 2), which will always look good.

To fengchao, I'm sorry if you feel I steal your job.. but I can't stop myself since I think I can fix it in 10 minutes..

This addresses bug 234407.
<a href="http://bugs.kde.org/show_bug.cgi?id=234407" title="http://bugs.kde.org/show_bug.cgi?id=234407">http://bugs.kde.org/show_bug.cgi?id=234407</a>

Diffs
systemsettings/icons/CMakeLists.txt 0830dd7
systemsettings/icons/FileItemDelegate.h PRE-CREATION
systemsettings/icons/FileItemDelegate.cpp PRE-CREATION
systemsettings/icons/IconMode.cpp 37cfc4b

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

Testing
see screenshot.

Screenshots (updated)
english after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1014/" title="http://git.reviewboard.kde.org/r/108328/s/1014/">http://git.reviewboard.kde.org/r/108328/s/1014/</a>
chinese after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1015/" title="http://git.reviewboard.kde.org/r/108328/s/1015/">http://git.reviewboard.kde.org/r/108328/s/1015/</a>
chinese before change
<a href="http://git.reviewboard.kde.org/r/108328/s/1016/" title="http://git.reviewboard.kde.org/r/108328/s/1016/">http://git.reviewboard.kde.org/r/108328/s/1016/</a>
spanish after change
<a href="http://git.reviewboard.kde.org/r/108328/s/1017/" title="http://git.reviewboard.kde.org/r/108328/s/1017/">http://git.reviewboard.kde.org/r/108328/s/1017/</a>
spanish before change
<a href="http://git.reviewboard.kde.org/r/108328/s/1023/" title="http://git.reviewboard.kde.org/r/108328/s/1023/">http://git.reviewboard.kde.org/r/108328/s/1023/</a>

Thanks,

Xuetian Weng

Re: Review Request 108328: fix 234407 - set minimum width for sy

By Ben Cooksley at 07/16/2014 - 07:25

Do we have any movement on fixing the arbitrary value issue raised, or should this review be discarded?

- Ben Cooksley

On Jan. 10, 2013, 8:48 p.m., Xuetian Weng wrote:

Re: Review Request: fix 234407 - set minimum width for system se

By Chao Feng at 01/10/2013 - 18:46

Screenshot: chinese after change
<http://git.reviewboard.kde.org//r/108328/#scomment131>
Others looks good, I still think here it looks strange. Increase to 7 characters?

Screenshot: chinese after change
<http://git.reviewboard.kde.org//r/108328/#scomment132>
Same here

- Chao Feng

On Jan. 10, 2013, 8:48 p.m., Xuetian Weng wrote:

Re: Review Request: fix 234407 - set minimum width for system se

By =?utf-8?Q?Thoma... at 01/10/2013 - 20:03

See my last comment at <a href="https://git.reviewboard.kde.org/r/108285/" title="https://git.reviewboard.kde.org/r/108285/">https://git.reviewboard.kde.org/r/108285/</a>
Picking a random value will actually not do if the goal is to prevent "stupid" wrapping of single glyphs.
You'll have to reimplement the delegate painting and paint the rows omitting the cliprect and thus ignoring the maximum size for single remains.

The risc with this approach is overlapping text (what can be migitated by a 2em margin between elements) and it needs to be accomplished by a reasonable minimum width (to prevent a 2 or even 3 char wide column) but picking a random static minimum size will fail on item a in language b for absolutely sure.

- Thomas

On Jan. 10, 2013, 8:48 p.m., Xuetian Weng wrote:

Re: Review Request: fix 234407 - set minimum width for system se

By Yichao Yu at 01/10/2013 - 18:59

Increase to 7 characters will only make 8 the number of characters in the text that looks strange. (and in fact it is 7 characters with my font settings.)
This should be a different issue that may be solved by changing how word wrapping is done for short Chinese text in Qt instead of disabling word wrapping or increasing the limit to a magic value IMHO.

- Yichao

On Jan. 10, 2013, 8:48 p.m., Xuetian Weng wrote:

Re: Review Request: fix 234407 - set minimum width for system se

By =?utf-8?Q?Thoma... at 01/10/2013 - 13:13

Screenshot: spanish after change
<http://git.reviewboard.kde.org//r/108328/#scomment129>
Is "gest..." elided before the patch as well? (got no es locales installed)

- Thomas Lübking

On Jan. 10, 2013, 6:09 p.m., Xuetian Weng wrote:

Re: Review Request: fix 234407 - set minimum width for system se

By Weng Xuetian at 01/10/2013 - 15:47

Yes.. I choose es as example here since I know es string is usually longer than English and I want to check how does this patch affect it.

- Xuetian

On Jan. 10, 2013, 8:47 p.m., Xuetian Weng wrote:

Re: Review Request: fix 234407 - set minimum width for system se

By =?utf-8?Q?Thoma... at 01/10/2013 - 13:12

systemsettings/icons/FileItemDelegate.cpp
<http://git.reviewboard.kde.org/r/108328/#comment19247>

relict?!

systemsettings/icons/FileItemDelegate.cpp
<http://git.reviewboard.kde.org/r/108328/#comment19245>

, m_minimumSize(0,0)

systemsettings/icons/FileItemDelegate.cpp
<http://git.reviewboard.kde.org/r/108328/#comment19246>

return KFileItemDelegate::sizeHint(option, index).expandedTo(m_minimumSize);

systemsettings/icons/IconMode.cpp
<http://git.reviewboard.kde.org/r/108328/#comment19248>

should this not rely on the icon size rather than on the fontmetrics (only)?
The point is to get more size between the icons, i thought ;-)

- Thomas Lübking

On Jan. 10, 2013, 6:09 p.m., Xuetian Weng wrote:

Re: Review Request: fix 234407 - set minimum width for system se

By Weng Xuetian at 01/10/2013 - 15:47

no, it's to have more space for the string, and to make it "less wrapped", Current code doesn't work well since it seems trying to "shrink the string" as much as possible, and CJK character doesn't break in to "non-wrappable" string like English so it doesn't work well. It doesn't matter how big is your icon.

- Xuetian

On Jan. 10, 2013, 8:47 p.m., Xuetian Weng wrote: