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

Sun Jun 19 08:14:13 UTC 2016

> On June 19, 2016, 2:39 a.m., Aleix Pol Gonzalez wrote:
> > Can you explain why it's better? To me the new code looks rather scary and it's rather weird that it needs so much OSX-specific complexity.

To me the images speak the proverbial 1000 words :) but basically I think it actually not only looks better because there is no more icon clipping or render outside the button box. My initial attempt solved that by sizing the button to accomodate the icon, but that ignored the fact that the button has to be the same height as the progressbar.
The current implementation achieves both, and I've used my usual (non-standard) icon theme to exclude  the possibility that the original implementation works well with the standard themes by chance.

Could you explain exactly what looks scary in the new code, there's hardly anything that does actually scary stuff, no? The OS X specific code is in fact style-specific; we're dealing here with aspects over which the active style has the last word to a large extent. That's apparent from the fact that neither `macintosh` nor `Breeze` allow height control of the progressbar and as far as I've been able to understand it also explains why you need to resize the icon explicitly rather than simply sizing the button and hope its content resize along. But the progressbar aspect is the main culprit for requiring style-specific code.
Note that it might become simpler if we go for "flat" buttons everywhere; I haven't even tried that because I didn't want to assume that would be acceptable outside of the `macintosh` style.

I saw that this widget comes from KDE PIM, and has been co-authored by David Faure. I've added him to this RR. Wouldn't it be an idea to move this widget to KGUIAddons where it could be of use to more applications? I presume that things like style-specific tuning might raise less eyebrows in a library than in an application?

- René J.V.

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

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
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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