Review Request 128201: Improved StatusbarProgressDidget that looks better on OS X and Linux

Sun Jun 19 13:00:47 UTC 2016

> On June 19, 2016, 12:20 p.m., David Faure wrote:
> > shell/progresswidget/statusbarprogresswidget.cpp, line 90
> > <https://git.reviewboard.kde.org/r/128201/diff/2/?file=469229#file469229line90>
> >
> >     Yes, see QWidget::setStyle().
> >     
> >     Not sure what the problem is with alignment, but at least it seems clear to me that if CT_ProgressBar returns a value smaller than the contents, we need to resize down the contents. Isn't that the issue here?

That's what I'm doing - and I'm also sizing up the contents when the progress bar height is taller. As to replacing `QApplication::style()` with `m_pProgressBar->style()`:

```
    qWarning() << "progbar style:" << m_pProgressBar->style() << m_pProgressBar->style()->objectName()
        << "app style:" << QApplication::style();
```

gives

```
progbar style: QStyleSheetStyle(0x7fdbf2656c00) "" app style: QMacStyle(0x7fdbf248e8d0, name = "macintosh")
progbar style: QStyleSheetStyle(0x7fc515d44190) "" app style: QtCurve::Style(0x7fc513876600, name = "qtcurve")
progbar style: QStyleSheetStyle(0x7fcb7c505e10) "" app style: Breeze::Style(0x7fcb79cbfd30, name = "breeze")
```

In other words, `QWidget::style()` returns something that is really a `QStyleSheetStyle` most (?) of the time. QStyleSheetStyle does have a `QStyle *baseStyle()` method, but you need to include a private header to gain access to that class. Do you know of any other way to get the current style name? I've tried `QProxyStyle pbStyle(m_pProgressBar->style())` but that doesn't help (and crashes when pbStyle goes out of scope). I'm tempted to conclude that using `m_pProgressBar->style()` may well be a theoretically more correct, but in practice not workable approach.

As to the alignment issue: I also couldn't fathom what that is about, in any case nothing I tried allowed me to get rid of it completely. I only managed to make it "acceptably small" with the current code (without flat buttons). See the 1st screenshot of the Mac/native widget without the patch; the alignment offset is visible there, and is maybe slightly worse than what I'm getting now but still acceptable as long as you don't study the layout too closely. It becomes much less noticeable when using a flat button, IMHO good enough not to complexify the code even more in an attempt to improve things.

BTW, QProgressBar does accept `WA_MacMiniSize` but 1) that affects all styles on OS X and 2) it ascerbates the alignment issue to an extreme point: the icon vcentre is aligned more or less with the progressbar bottom. Not usable AFAIAC even if the lower progress bar does look better.

> On June 19, 2016, 12:20 p.m., David Faure wrote:
> > shell/progresswidget/statusbarprogresswidget.cpp, line 96
> > <https://git.reviewboard.kde.org/r/128201/diff/2/?file=469229#file469229line96>
> >
> >     Oops I meant to remove that comment once I wrote the previous one (i.e. after understanding the difference between styles where CT_ProgressBar adapts to the contents, and styles where it doesn't)

What's the issue to fix here? NB: after re-reading the comment I remember that the choice of height estimate also depends on the kind of button that's used. With a non-flat button I find the overal result better using `minimumSizeHint()`.

- René J.V.

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

On June 19, 2016, 12:29 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 19, 2016, 12:29 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
> Linux, Breeze, new patch with maximumHeight from progressBar->sizeHint().height()
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/06/19/2a5b48bf-38f1-4ba9-a291-5de65537f8e1__progress-patched-breeze2.png
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160619/b89b8177/attachment.html>