Review Request: Plasma::IconWidget: Add support for constrained size hints

[Ingomar Wesp]

> On 2010-10-11 08:34:14, Marco Martin wrote:
> > > The reported preferredSize is not bounded by the maximumSize. 
> > 
> > you don't have to call maximumSize(), it's the hardcoded overriding size hint that you can set from the outside ith setMaximumSize.
> > 
> > you just have to obtain it in hte same way you use in sizeHint:
> > sizeFromIconSize(qMax(d->maximumIconSize.height(), d->maximumIconSize.width()));
> > 
> > and yes, size hints are cached, that's (besides the existence of setMaximum/MinimumSize) why sizeHint() is protected and effectiveSizeHint exists

>> The reported preferredSize is not bounded by the maximumSize.
> 
> you don't have to call maximumSize(), it's the hardcoded overriding size
> hint that you can set from the outside ith setMaximumSize.

Err... but maximumSize() delegates to sizeHint() if no maximum size has been explicitly set, no?

> you just have to obtain it in hte same way you use in sizeHint:
> sizeFromIconSize(qMax(d->maximumIconSize.height(),
> d->maximumIconSize.width()));

This has crossed my mind as well. It works if there is a valid maximumIconSize set. Otherwise, falling back to QGraphicsLayoutItem::sizeHint(Qt::MaximumSize) would probably be the way to go. Unfortunately, if the logic for the maximumSize in sizeHint ever changes, we'll need to duplicate this here as well.

I will change the patch accordingly.

> and yes, size hints are cached, that's (besides the existence of
> setMaximum/MinimumSize) why sizeHint() is protected and effectiveSizeHint
> exists

I know, I just found it surprising that effectiveSizeHint tries to fetch all size hints and that it apparently even uses the previously given constraint for that...


- Ingomar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5580/#review8073
-----------------------------------------------------------


On 2010-10-11 01:40:48, Ingomar Wesp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5580/
> -----------------------------------------------------------
> 
> (Updated 2010-10-11 01:40:48)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> I poked around the IconWidget class and came up with the following patch that
> implements handling of constraints in sizeHint(...). It basically pulls the 
> code that determines the icon size for a given widget size out of 
> layoutIcons(...) into an the a new iconSizeForWidgetSize(...) method so that 
> it can be reused by sizeHint(...).
> 
> I've also changed the signatures of elidedText(...) and layoutText(...) by 
> removing the need to pass a QStyleOptionGraphicsItem*, since it wasn't used 
> there anyway. If there's some reason why this could be a bad idea, I'll put it 
> back in.
> 
> The patch changes the behavior of sizeHint(...) in the following way:
> 
> - If there is no constraint, the behavior remains unchanged.
> 
> - Likweise, the minimum and maximum sizes are unaffected by constraints. I 
>   don't see how limiting the available width/height would affect either one?
>   
> - The preferred size computation now uses the constraint to compute the 
>   expected size of the icon and adjusts the returned value accordingly.
> 
> I must admit that I'm not entirely happy with the solution as it has (at
> least ;) the following issues:
> 
> - The reported preferredSize is not bounded by the maximumSize. Unfortunately 
>   I can't call maximumSize() and its derivatives from within sizeHint, since
>   this might cause QGraphicsLayoutItem to update all sizeHints, which might in
>   turn lead to unbound recursion, since it apparently also caches the 
>   previously passed constraints :/
>   
>   This is why I'm setting any unconstrained dimension to QWIDGETSIZE_MAX for 
>   the purpose of determining the icon size that fits within. 
> 
> - I'm not very fond of having to initialize and pass around
>   QStyleOptionGraphicsItem objects, especially since they are apparently only 
>   used for determining whether we've got an RTL layout. 
> 
> Any testing / feedback would be greatly appreciated.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/widgets/iconwidget.cpp 1182894 
>   /trunk/KDE/kdelibs/plasma/widgets/iconwidget_p.h 1182894 
> 
> Diff: http://svn.reviewboard.kde.org/r/5580/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ingomar
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20101011/7f828b10/attachment.htm