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

[Frank Reininghaus]

> On Feb. 14, 2014, 1 p.m., Emmanuel Pescosta wrote:
> > 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.

Cool, thank you very much for testing this!

There are some changes in master that make this patch not apply cleanly any more, so I think I'll push a slightly modified version to master only in order to prevent merge conflicts when merging KDE/4.12 into master.


- Frank


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


On Nov. 15, 2013, 8: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, 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: 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/20140217/57ba4b4a/attachment.htm>