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

[Emmanuel Pescosta]

> On Nov. 16, 2013, 4:54 p.m., Christoph Feck wrote:
> > 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 wrote:
>     Yes, at the time I commented on the bug, I was able to reproduce. I either forgot the exact conditions/settings needed to reproduce, or the bug simply vanished, in other words, I cannot verify the patch.
>     
>     What I remember, though, is that Qt indeed gave me two different values for line.height(), so the concern above should actually still be valid. There is no need to accumulate wrong values, when you assume it to be a multiple of lineSpacing anyway.
> 
> Frank Reininghaus wrote:
>     My assumption was that the values returned by line.height() are valid, i.e., that the height of the text lines really depends on the text in the line, and can differ from option.fontMetrics.lineSpacing(). In that case, it would be incorrect to simply use "option.fontMetrics.lineSpacing()" * "max number of lines" as the limit for the height of the "text rect".
>     
>     If that is really the cause of the problem, I think that my patch would fix it.

@Christoph: Any updates?


- Emmanuel


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


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/20140120/0539f82a/attachment.htm>