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

[Christoph Feck]

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


Currently compiling, and reading the patch in the meantime. From what I see, this patch may not fix the issue, because you still use textHeight += line.height().
It might be need to changed to a final textHeight = lineCount * lineSpacing(), but before you start hacking, let me test it :)

- Christoph Feck


On Nov. 15, 2013, 8:15 a.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113871/
> -----------------------------------------------------------
> 
> (Updated Nov. 15, 2013, 8: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: http://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/20131116/ea4f445b/attachment.htm>