Konqueror throbber size -- too small and doesn't resize

[James Richard Tyrer]

I find in: kdebase/apps/konqueror/src/konqmainwindow.cpp, line 3762

int size = style()->pixelMetric(QStyle::PM_SmallIconSize, NULL,
        m_paAnimatedLogo);

appears to set the size to "SmallIconSize" rather than choosing a size
based the pixel size of the menu bar which seems to be dependent on the
pixel size of menuFont.

Originally, I only made 16x16, 22x22, & 32x32 throbbers.  IIRC, someone
reversed the direction of rotation.  I can make larger ones if needed.

It might also be possible to make an SVG which could be rendered to the
exact size that is needed.  That would be simple since the PNGs are made
with InkScape.

--
JRT

On Wednesday 04 June 2008, James Richard Tyrer wrote:
> I find in: kdebase/apps/konqueror/src/konqmainwindow.cpp, line 3762
>
> int size = style()->pixelMetric(QStyle::PM_SmallIconSize, NULL,
> m_paAnimatedLogo);
>
> appears to set the size to "SmallIconSize" rather than choosing a size
> based the pixel size of the menu bar which seems to be dependent on the
> pixel size of menuFont.

Yeah, but the real problem seems to be the margin inside the QToolButton.
I tried int size = menuBar()->height() and obviously this was too big, but it
shows that the button contains a lot of inner padding -- much more than when
a QToolButton is put into a toolbar. This has to come from the widget style I think?
I see no special logic for this inside QToolButton. Cc'ing Casper who might have an
idea about this problem: how to get a toolbutton without inner margins, and what
would be the right icon size for a toolbutton that is put into a menubar?

I tried   m_paAnimatedLogo->setContentsMargins(0, 0, 0, 0); but it has no effect,
I think the style is changing the contents margins later on...

--
David Faure, faure@..., sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

[James Richard Tyrer]

David Faure wrote:

> On Wednesday 04 June 2008, James Richard Tyrer wrote:
>> I find in: kdebase/apps/konqueror/src/konqmainwindow.cpp, line 3762
>>
>> int size = style()->pixelMetric(QStyle::PM_SmallIconSize, NULL,
>> m_paAnimatedLogo);
>>
>> appears to set the size to "SmallIconSize" rather than choosing a size
>> based the pixel size of the menu bar which seems to be dependent on the
>> pixel size of menuFont.
>
> Yeah, but the real problem seems to be the margin inside the QToolButton.
> I tried int size = menuBar()->height() and obviously this was too big, but it
> shows that the button contains a lot of inner padding -- much more than when
> a QToolButton is put into a toolbar. This has to come from the widget style I think?
> I see no special logic for this inside QToolButton. Cc'ing Casper who might have an
> idea about this problem: how to get a toolbutton without inner margins, and what
> would be the right icon size for a toolbutton that is put into a menubar?
>
> I tried   m_paAnimatedLogo->setContentsMargins(0, 0, 0, 0); but it has no effect,
> I think the style is changing the contents margins later on...
>

See commit 817650

I tried:

        int size = menuBar()->height();

and the throbber is a little two large, but I think I fixed your problem
-- no boarder; if fills the whole button.

Is it possible to get the height of the Menu font rather than the height
of the menu or is it necessary to use QStyle::PM_MenuBarVMargin to
reduce it?

--
JRT

[James Richard Tyrer]

This is not going very well.

With 12 point type, the Menu bar is 30 pixels tall.  However, if I use a
throbber icon that is larger than 20 pixels, the Menu bar gets taller.

And using the pixel height of the font:

   int size = QFontInfo(KGlobalSettings::menuFont()).pixelSize();

seems to have problems as well since the value is only 17 (which is
correct).

So, I have the attached diff that is only going to help those with high
resolution screens.

This leaves me with questions.

17 pixels is correct for 12 points @ 100 DPI but where does the extra 3
pixels come from and is there a way to obtain this value?

Is there any way to use a throbber that is larger than Qt seems to think
will fit in the Menu bar?

See screen shot which shows a 20x20 throbber (I simply set size = 20 to
try it).  There is clearly room for a larger throbber -- 22x22 would
look OK.

--
JRT


Index: konqmainwindow.cpp
===================================================================
--- konqmainwindow.cpp (revision 818056)
+++ konqmainwindow.cpp (working copy)
@@ -3695,7 +3695,17 @@ void KonqMainWindow::initActions()
   m_paAnimatedLogo->setFocusPolicy(Qt::NoFocus);
   m_paAnimatedLogo->setToolButtonStyle(Qt::ToolButtonIconOnly);
 
-  int size = style()->pixelMetric(QStyle::PM_SmallIconSize, NULL, m_paAnimatedLogo);
+  int size = QFontInfo(KGlobalSettings::menuFont()).pixelSize();
+
+  if ( size < 22 )
+    size = 16;
+  else if ( size < 32 )
+    size = 22;
+  else if ( size < 48 )
+    size = 32;
+  else if ( size < 64 )
+    size = 48;
+
   m_paAnimatedLogo->setIconSize(QSize(size, size));
   m_paAnimatedLogo->setIcons("process-working-kde");
 

[James Richard Tyrer]

James Richard Tyrer wrote:

> This is not going very well.
>
> With 12 point type, the Menu bar is 30 pixels tall.  However, if I
> use a throbber icon that is larger than 20 pixels, the Menu bar gets
> taller.
>
> And using the pixel height of the font:
>
> int size = QFontInfo(KGlobalSettings::menuFont()).pixelSize();
>
> seems to have problems as well since the value is only 17 (which is
> correct).
>
> So, I have the attached diff that is only going to help those with
> high resolution screens.
>
> This leaves me with questions.
>
> 17 pixels is correct for 12 points @ 100 DPI but where does the extra
> 3 pixels come from and is there a way to obtain this value?
>
> Is there any way to use a throbber that is larger than Qt seems to
> think will fit in the Menu bar?
>
> See screen shot which shows a 20x20 throbber (I simply set size = 20
> to try it).  There is clearly room for a larger throbber -- 22x22
> would look OK.
>

Well, I looked through the Qt dox and the attached patch should work.

Unfortunately, there appears to be a bug in Qt.  Still, it works better
than what we have now, and will improve if the Qt bug is fixed.

Should I commit this?

--
JRT



Index: konqmainwindow.cpp
===================================================================
--- konqmainwindow.cpp (revision 818274)
+++ konqmainwindow.cpp (working copy)
@@ -3695,7 +3695,17 @@ void KonqMainWindow::initActions()
   m_paAnimatedLogo->setFocusPolicy(Qt::NoFocus);
   m_paAnimatedLogo->setToolButtonStyle(Qt::ToolButtonIconOnly);
 
-  int size = style()->pixelMetric(QStyle::PM_SmallIconSize, NULL, m_paAnimatedLogo);
+  int size = (menuBar()->contentsRect()).height();
+
+  if ( size < 22 )
+    size = 16;
+  else if ( size < 32 )
+    size = 22;
+  else if ( size < 48 )
+    size = 32;
+  else if ( size < 64 )
+    size = 48;
+
   m_paAnimatedLogo->setIconSize(QSize(size, size));
   m_paAnimatedLogo->setIcons("process-working-kde");
 

[Rafael Fernández López-3]

Hi,

Comments about the patch.

1) I don't see the point of getting out a very valid number got from the
style.
2) Hardcoded values => evil.

Despite I cannot reproduce this issue I am pretty sure there is a better way
of fixing this issue.

Please, instead of hardcoded values use KIconLoader:: enums for icon sizes.

> Well, I looked through the Qt dox and the attached patch should work.
> Unfortunately, there appears to be a bug in Qt.  Still, it works better
> than what we have now, and will improve if the Qt bug is fixed.

What bug in Qt, what will improve if the bug is fixed ?

If you found a bug in Qt and say "will improve if the Qt bug is fixed"... I
guess you reported it to qt-bugs, or is is going to be fixed by itself ? :)

BTW, cross posting is not fun :)


Regards,
Rafael Fernández López.

On Saturday 07 June 2008, James Richard Tyrer wrote:
> 17 pixels is correct for 12 points @ 100 DPI but where does the extra 3
> pixels come from and is there a way to obtain this value?
 
It comes from the widget style. QMenuBar::sizeHint adds:

QStyle::SH_MainWindow_SpaceBelowMenuBar + 2 * (QStyle::PM_MenuBarPanelWidth + QStyle::PM_MenuBarVMargin)

to the action's text, but then it looks at the corner widgets (like the throbber), and
calculates that the same margin as the one above will be above/below the corner widget.

So there's no way for the corner widget to be as high as the menubar, it can only be as
high as the text inside the menubar.

(and then style()->sizeFromContents(QStyle::CT_MenuBar) is called, which can make the
menubar higher than the above calculation, e.g. qplastiquestyle adds 2 pixels)

> There is clearly room for a larger throbber

There is, but not with Qt adding the above margin to the corner widget's height.
We're lucky setCornerWidget exists in the first place (it's still marked as \internal,
i.e. it wasn't meant to be used out of Qt...), I think we'll have to make do with the
inability to use a few pixels (I filed a request for setCornerWidget to be real public
API with more alignment options some time ago already; TT task 200779).

--
David Faure, faure@..., sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

On Saturday 07 June 2008, James Richard Tyrer wrote:

> This is not going very well.
>
> With 12 point type, the Menu bar is 30 pixels tall.  However, if I use a
> throbber icon that is larger than 20 pixels, the Menu bar gets taller.
>
> And using the pixel height of the font:
>
>    int size = QFontInfo(KGlobalSettings::menuFont()).pixelSize();
>
> seems to have problems as well since the value is only 17 (which is
> correct).

Like Rafael, I don't like the use of hardcoded values; I would prefer a precise
reverse calculation from QMenuBar::sizeHint.

This would give something like this....

int KonqMainWindow::maxThrobberHeight()
{
    const int vmargin = style()->pixelMetric(QStyle::PM_MenuBarVMargin, 0, menuBar());
    const int fw = style()->pixelMetric(QStyle::PM_MenuBarPanelWidth, 0, menuBar());
    const int spaceBelowMenuBar = style()->styleHint(QStyle::SH_MainWindow_SpaceBelowMenuBar, 0, menuBar());
    const int margin = 2*vmargin + 2*fw + spaceBelowMenuBar;
    // Let's see how much height the widget style adds
    QStyleOptionMenuItem opt;
    opt.rect = menuBar()->rect();
    opt.menuRect = opt.rect;
    opt.state = QStyle::State_None;
    opt.menuItemType = QStyleOptionMenuItem::Normal;
    opt.checkType = QStyleOptionMenuItem::NotCheckable;
    const QRect finalRect = style()->sizeFromContents(QStyle::CT_MenuBar, &opt, opt.rect), menuBar()));
    const int stylePadding = finalRect.height() - opt.rect.height();
    return menuBar()->height() - margin - stylePadding;
}

Oh... BUT -- that's the maximum height of the widget, not its icon size! We also need to subtract the
difference between the iconsize of a qtoolbutton and its final size...

More complete patch attached.
Although from a quick test I have the feeling it's still a little bit too big... Can you give it
a bit of testing?

--
David Faure, faure@..., sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

[attachment removed]

On Monday 09 June 2008, David Faure wrote:
> I filed a request for setCornerWidget to be real public
> API with more alignment options some time ago already; TT task 200779)

Typo in the task name, it's 200799.

--
David Faure, faure@..., sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

On Monday 09 June 2008, David Faure wrote:
>     return menuBar()->height() - margin - stylePadding;

Ah - this is the problem. The layouting hasn't been done yet, so we can't ask
for menuBar()->height(), it's the default 30 pixels like any QWidget before it's shown...
I'll redo this differently.

--
David Faure, faure@..., sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

[James Richard Tyrer]

David Faure wrote:
> On Monday 09 June 2008, David Faure wrote:
>>     return menuBar()->height() - margin - stylePadding;
>
> Ah - this is the problem. The layouting hasn't been done yet, so we can't ask
> for menuBar()->height(), it's the default 30 pixels like any QWidget before it's shown...
> I'll redo this differently.
>
After reading the dox, I tried this a different way.  See attached.
IIUC, Qt will compute the size of the contents rectangle in the MenuBar
so we don't have to do it.

I have gotten rid of the hard coded values.

However, this doesn't work exactly right. :-(

With 12 pt type @ 100 DPI (17 pixel font size) the menu bar is 30 pixels
high. and that appears to be the value which:

        (menuBar()->contentsRect()).height();

returns.  Since this is 30 pixels, I don't know why I am getting 30 pixels.

So, I tried different font sizes for the Menu font and it does not
change the throbber size.  So, the problem is probably as you said that
the layout hasn't been done yet and the 30 pixels is probably the
default height.

IIUC, the problem is that the operations are not in the proper order.

At this point, some help would be appreciated.

--
JRT


Index: konqmainwindow.cpp
===================================================================
--- konqmainwindow.cpp (revision 818585)
+++ konqmainwindow.cpp (working copy)
@@ -3695,7 +3695,17 @@ void KonqMainWindow::initActions()
   m_paAnimatedLogo->setFocusPolicy(Qt::NoFocus);
   m_paAnimatedLogo->setToolButtonStyle(Qt::ToolButtonIconOnly);
 
-  int size = style()->pixelMetric(QStyle::PM_SmallIconSize, NULL, m_paAnimatedLogo);
+  int size = (menuBar()->contentsRect()).height();
+
+  if ( size < KIconLoader::SizeSmallMedium )
+    size = KIconLoader::SizeSmall;
+  else if ( size < KIconLoader::SizeMedium  )
+    size = KIconLoader::SizeSmallMedium;
+  else if ( size < KIconLoader::SizeLarge )
+    size = KIconLoader::SizeMedium ;
+  else if ( size < KIconLoader::SizeHuge )
+    size = KIconLoader::SizeLarge;
+
   m_paAnimatedLogo->setIconSize(QSize(size, size));
   m_paAnimatedLogo->setIcons("process-working-kde");
 

On Wednesday 11 June 2008, James Richard Tyrer wrote:
>  I don't know why I am getting 30 pixels.
>
> So, I tried different font sizes for the Menu font and it does not
> change the throbber size.  So, the problem is probably as you said that
> the layout hasn't been done yet and the 30 pixels is probably the
> default height.

Yes. Please read the code in svn, I fixed all this already. But fredrikh made
me realize that setting an icon size to 18x18 or 19x19 was a bad idea,
the pixmap gets scaled, so now I understand your if()s that round to a known
icon size. I'll merge this in, but I'll also force the button size so that it gets
vertically centered, this is what was bothering me most.

--
David Faure, faure@..., sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

[James Richard Tyrer]

David Faure wrote:

> On Wednesday 11 June 2008, James Richard Tyrer wrote:
>>  I don't know why I am getting 30 pixels.
>>
>> So, I tried different font sizes for the Menu font and it does not
>> change the throbber size.  So, the problem is probably as you said that
>> the layout hasn't been done yet and the 30 pixels is probably the
>> default height.
>
> Yes. Please read the code in svn, I fixed all this already. But fredrikh made
> me realize that setting an icon size to 18x18 or 19x19 was a bad idea,
> the pixmap gets scaled, so now I understand your if()s that round to a known
> icon size. I'll merge this in, but I'll also force the button size so that it gets
> vertically centered, this is what was bothering me most.
>

I was having build problems so didn't get to try this till now.  It
doesn't seem to work for me.  I get a 21x21 icon with a 12 pt. Menu font
and the same with a 24 pt..

Yes, the icons sizes should be set to one of the standard values as my
patch does.  We have KDE throbber icons in 16x16, 22x22, 32x32 & 48x48.
  So, somewhere after 48 we should go to variable size.  The code in my
patch:

+  if ( size < KIconLoader::SizeSmallMedium )
+    size = KIconLoader::SizeSmall;
+  else if ( size < KIconLoader::SizeMedium  )
+    size = KIconLoader::SizeSmallMedium;
+  else if ( size < KIconLoader::SizeLarge )
+    size = KIconLoader::SizeMedium ;
+  else if ( size < KIconLoader::SizeHuge )
+    size = KIconLoader::SizeLarge;

does this for sizes greater than or equal to "SizeHuge" -- that is,
"SizeHuge" or larger drops through the If-Else structure unchanged.  I
don't suppose that we really need to worry about a throbber larger than
48x48 but I was asked to also make an SVG which I have put on my to-do list.

Your code is rather complex.  Should we consider it a bug in Qt that
using: "(menuBar()->contentsRect()).height()" doesn't provide us with
the correct number?

Thanks for working on this.  It seemed like it would be simple when I
started.

--
JRT

On Thursday 12 June 2008, James Richard Tyrer wrote:

> David Faure wrote:
> > On Wednesday 11 June 2008, James Richard Tyrer wrote:
> >>  I don't know why I am getting 30 pixels.
> >>
> >> So, I tried different font sizes for the Menu font and it does not
> >> change the throbber size.  So, the problem is probably as you said that
> >> the layout hasn't been done yet and the 30 pixels is probably the
> >> default height.
> >
> > Yes. Please read the code in svn, I fixed all this already. But fredrikh made
> > me realize that setting an icon size to 18x18 or 19x19 was a bad idea,
> > the pixmap gets scaled, so now I understand your if()s that round to a known
> > icon size. I'll merge this in, but I'll also force the button size so that it gets
> > vertically centered, this is what was bothering me most.
> >
> I was having build problems so didn't get to try this till now.  It
> doesn't seem to work for me.  I get a 21x21 icon with a 12 pt. Menu font
> and the same with a 24 pt..

Did you update again after I committed 819749 (only a few minutes before your mail) ?
Should be better now.

> Yes, the icons sizes should be set to one of the standard values as my
> patch does.  We have KDE throbber icons in 16x16, 22x22, 32x32 & 48x48.
>   So, somewhere after 48 we should go to variable size.  The code in my
> patch:

Yep I put that in, now.

> +  if ( size < KIconLoader::SizeSmallMedium )
> +    size = KIconLoader::SizeSmall;
> +  else if ( size < KIconLoader::SizeMedium  )
> +    size = KIconLoader::SizeSmallMedium;
> +  else if ( size < KIconLoader::SizeLarge )
> +    size = KIconLoader::SizeMedium ;
> +  else if ( size < KIconLoader::SizeHuge )
> +    size = KIconLoader::SizeLarge;
>
> does this for sizes greater than or equal to "SizeHuge" -- that is,
> "SizeHuge" or larger drops through the If-Else structure unchanged.  I
> don't suppose that we really need to worry about a throbber larger than
> 48x48 but I was asked to also make an SVG which I have put on my to-do list.

Ah so my initial value is wrong, I should set iconSize to buttonSize by default, OK.

> Your code is rather complex.

It asks the style, which is the only proper solution :)

> Should we consider it a bug in Qt that  
> using: "(menuBar()->contentsRect()).height()" doesn't provide us with
> the correct number?

Nope. There's no layouting before the window is shown.

> Thanks for working on this.  It seemed like it would be simple when I started.

Yeah :-)

But anyway I'm starting to consider making it a QLabel instead. This way we can get rid
of that internal padding that all styles want to use for a toolbutton. And if we really want
to still have some click action [broken in 4.x], we can still do it, the feature (well, easter egg :)
is just as hidden if it's a qlabel than it was with a qtoolbutton anyway :)

--
David Faure, faure@..., sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

[James Richard Tyrer]

David Faure wrote:
> On Thursday 12 June 2008, James Richard Tyrer wrote:

>> does this for sizes greater than or equal to "SizeHuge"

Duh!  I think that that should be:

does this for sizes *less* than or equal to "SizeHuge"

--
JRT

On Thursday 12 June 2008, David Faure wrote:
> Ah so my initial value is wrong, I should set iconSize to buttonSize by default, OK.

Ignore that. The code I committed to svn doesn't have that issue, so your SVG should work
once it exists :)

--
David Faure, faure@..., sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).