Review Request 128201: Improved StatusbarProgressDidget that looks better on OS X and Linux
René J.V. Bertin
rjvbertin at gmail.com
Sun Jun 19 09:51:45 UTC 2016
> On June 19, 2016, 10:39 a.m., David Faure wrote:
> > shell/progresswidget/statusbarprogresswidget.cpp, line 90
> > <https://git.reviewboard.kde.org/r/128201/diff/2/?file=469229#file469229line90>
> >
> > Technically this should check m_pProgressBar->style() rather than QApp::style().
> >
> > I see why this check is needed: the mac style doesn't care for the contents size in CT_ProgressBar, the return value is indeed "fixed".
> >
> > I guess the alternative to a hardcoded "==macintosh" would be a runtime test on the style,
> > i.e. calling CT_ProgressBar with maximumHeight as input, and checking if the return value is smaller than that. Then we know the style isn't taking into accounts the contents size, whatever the name of the style is. This might be a more future-proof and general approach.
Can a widget ever have a different style than the application - at least if the application doesn't use stylesheets? I have no objection to do the test that way.
Using CT_ProgressBar is likely to be too generic though. The fact that the return value is smaller (or larger, in case of `macintosh`!) doesn't tell whether or not the style will have other ideosyncrasies like its own ideas how to align widgets.
> On June 19, 2016, 10:39 a.m., David Faure wrote:
> > shell/progresswidget/statusbarprogresswidget.cpp, line 95
> > <https://git.reviewboard.kde.org/r/128201/diff/2/?file=469229#file469229line95>
> >
> > I don't understand that comment, this is about a progressbar, not a button (there's no call to minimumSizeHint in here)
What that comment tries to tell is something that would have been clearer if I'd left the qDebug() statement in: between `m_pProgressBar->height()`, `->sizeHint().height()` and `->minimumSizeHint().height()`, it's `sizeHint().height()` that works best for resizing the icon. I'll remove the comment.
> On June 19, 2016, 10:39 a.m., David Faure wrote:
> > shell/progresswidget/statusbarprogresswidget.cpp, line 96
> > <https://git.reviewboard.kde.org/r/128201/diff/2/?file=469229#file469229line96>
> >
> > Wouldn't this logic work everywhere, not just on mac?
Yes, if there were only the progressbar, and that bar didn't need to show the percentage text badge (see the QtCurve and Oxygen screenshots). From what I understand that is the reason why the original code determines `maximumHeight` from the rendered text label. I have assumed that the original authors verified whether `m_pProgressBar->*height()` returns a value that takes that text label into account. I can check whether that's the case: if it doesn't then the icon and button will become very small indeed for Breeze.
Adding a pair of debug calls:
```
m_pProgressBar = new QProgressBar( this );
m_pProgressBar->installEventFilter( this );
m_pProgressBar->setMinimumWidth( w );
qDebug() << "maximumHeight=" << maximumHeight << "progBar height=" << m_pProgressBar->height()
<< "sizeHint:" << m_pProgressBar->sizeHint() << "minSizeHint:" << m_pProgressBar->minimumSizeHint();
m_pProgressBar->setMinimumHeight( maximumHeight );
m_pProgressBar->setAttribute( Qt::WA_LayoutUsesWidgetRect, true );
qDebug() << "new? progBar height=" << m_pProgressBar->height()
<< "sizeHint:" << m_pProgressBar->sizeHint() << "minSizeHint:" << m_pProgressBar->minimumSizeHint();
```
I get (on Linux, we already know what to do on OS X):
```
QtCurve:
maximumHeight= 18 progBar height= 30 sizeHint: QSize(91, 24) minSizeHint: QSize(91, 18)
new? progBar height= 30 sizeHint: QSize(91, 24) minSizeHint: QSize(91, 18)
Breeze:
maximumHeight= 18 progBar height= 30 sizeHint: QSize(91, 24) minSizeHint: QSize(91, 18)
new? progBar height= 30 sizeHint: QSize(91, 24) minSizeHint: QSize(91, 18)
Oxygen:
maximumHeight= 18 progBar height= 30 sizeHint: QSize(91, 24) minSizeHint: QSize(91, 18)
new? progBar height= 30 sizeHint: QSize(91, 24) minSizeHint: QSize(91, 18)
```
So using `sizeHint().height()` will give an apparently style-independent height indication, but the wrong one. On Linux we'd have to use `minimumSizeHint().height()` to preserve the current visual aspect or else accept that the widget will be about 50% taller (for a 10pt font; I use Segoe UI Semibold as my standard font). I think that becomes a bit too much (see the screenshot).
We could initialise `maximumHeight = m_pProgressBar->minimumSizeHint().height()`, but we'd still have to keep the adjusting code for `macintosh`, so I'm not sure about such a change.
Also, the `m_pProgressBar->setMinimumHeight(maximumHeight)` call is apparently redundant for the 3 styles I've tested, but can we be sure that this will always be the case, for all styles and/or stylesheets?
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128201/#review96706
-----------------------------------------------------------
On June 18, 2016, 7:47 p.m., René J.V. Bertin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128201/
> -----------------------------------------------------------
>
> (Updated June 18, 2016, 7:47 p.m.)
>
>
> Review request for KDevelop and David Faure.
>
>
> Repository: kdevplatform
>
>
> Description
> -------
>
> I came across this old bit of code, judging from the use of the deprecated `Q_OS_MAC` token.
> Some quick testing suggests that using `WA_LayoutUsesWidgetRect` achieves what I think is the intended effect without platform-specific code. (That platform test ought really check for using the `macintosh` widget style btw, rather than "are we running on OS X").
>
>
> Diffs
> -----
>
> shell/progresswidget/statusbarprogresswidget.cpp 1a32ea8
>
> Diff: https://git.reviewboard.kde.org/r/128201/diff/
>
>
> Testing
> -------
>
> For now only on OS X, with the native theme as well as QtCurve, Breeze and Oxygen.
>
>
> File Attachments
> ----------------
>
> OS X/Mac native *without* the patch
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/16/1498e8dc-4541-473b-b6db-2c2ae138cbc3__Screen_Shot_2016-06-17_at_00.17.17.png
> stock widget under Linux (same layout using Breeze)
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/17/dd352994-1753-4b33-a0d2-cd2a3132df48__progress-stock.png
> OS X/native with the new patch
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/8e6a0761-b760-4f41-be67-7f9ba7318349__Screen_Shot_2016-06-18_at_19.21.52.png
> OS X using QtCurve with the new patch
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/3535be4c-3034-493d-88c0-61ab9a13844d__Screen_Shot_2016-06-18_at_19.24.46.png
> Linux, Breeze, new patch
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/359d5f57-bdaa-45a7-a0b0-6acb5609459f__Screen_Shot_2016-06-18_at_19.16.19.png
> Linux, QtCurve, new patch
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/f5eb0902-884a-47e5-bf84-a3581a21bece__Screen_Shot_2016-06-18_at_19.14.17.png
> Linux, Oxygen, new patch
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/536d4f09-8409-4eb4-80ca-27d3f744d6a0__Screen_Shot_2016-06-18_at_19.16.56.png
> Linux, Breeze + Breeze icons, new patch
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/19/ee2d687e-69ea-493f-9ae4-87d90be614e2__progress-patched-breeze.png
> Linux, QtCurve + Breeze icons, new patch
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/19/7e3cfc5c-cb80-491c-b4d2-4d6f9c839a48__progress-patched-qtcurve.png
> Linux,Oxygen+Breeze icons, new patch
> https://git.reviewboard.kde.org/media/uploaded/files/2016/06/19/0b10d97d-3ba6-4d05-a663-3a03e29bc1c9__progress-patched-oxygen.png
>
>
> Thanks,
>
> René J.V. Bertin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160619/2b82ce49/attachment-0001.html>
More information about the KDevelop-devel
mailing list