Review Request 113871: Make the handling of the "maximum text lines" setting in Icons View more robust

[Emmanuel Pescosta]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/113871/#review49774
-----------------------------------------------------------

Ship it!


So after playing around with some custom fonts, I could finally reproduce these bugs :)

Settings (Icons view mode):
Icon Size isn't relevant
Width: Small
Max. Lines: 2
Font: Custom Font (Font: Clean - Style: Regular - Size: 9.0)

I can confirm that this patch fixes these bugs, so please ship it!

I have also found two other layouting/view bugs while I was trying to reproduce these bugs, I'll have a look at them, open new bug reports and hopefully provide patches.

- Emmanuel Pescosta


On Nov. 15, 2013, 9:15 a.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/113871/
> -----------------------------------------------------------
> 
> (Updated Nov. 15, 2013, 9:15 a.m.)
> 
> 
> Review request for Dolphin and Christoph Feck.
> 
> 
> Bugs: 308022 and 323841
>     http://bugs.kde.org/show_bug.cgi?id=308022
>     http://bugs.kde.org/show_bug.cgi?id=323841
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> If the user sets a maximum number of text lines in the settings, this number is translated into a maximum height in pixels using QFontMetrics::lineSpacing().
>  
> In In KStandardItemListWidgetInformant::itemSizeHint(int, const KItemListView*), this maximum height limits the size that is reserved for the item.
> 
> However, in KStandardItemListWidget::updateIconsLayoutTextCache(), the maximum height is translated back into a maximum number of lines, which limits the number of lines that are created using the QTextLayout.
> 
> This approach can lead to problems if the real height of the layouted text is 1 pixel more or less than QFontMetrics::lineSpacing() times "number of lines". According to Christoph's analysis, this is the reason for the blurry icons in bug 323841.
> 
> I propose to change this by not storing a "maximum height" inside the "maximum size" explicitly, but store a maximum number of lines and a maximum with (for Compact View) separately, and then use the number of lines also to calculate the required size in KStandardItemListWidgetInformant::itemSizeHint(int, const KItemListView*). This should make sure that the correct height is reserved for each item. It also makes the code a bit simpler IMHO.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kitemliststyleoption.h 1a304fc 
>   dolphin/src/kitemviews/kitemliststyleoption.cpp ac25879 
>   dolphin/src/kitemviews/kitemlistview.cpp 7f49721 
>   dolphin/src/kitemviews/kstandarditemlistwidget.cpp 302150f 
>   dolphin/src/views/dolphinitemlistview.cpp 4799d76 
> 
> Diff: https://git.reviewboard.kde.org/r/113871/diff/
> 
> 
> Testing
> -------
> 
> I could not see any regressions. I was not able to reproduce the bug, so I cannot really verify if this patch really fixes it.
> 
> @Christoph: your comment in bug 323841 sounds like you could reproduce the problem? Could you have a look at this patch? Thanks in advance!
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140214/8b08bf68/attachment.htm>