hpereiradacosta added a comment.
Hello Marco, Thanks for the patch. 1/ On the implementation side: I have tried so far to keep all the magic numbers for breeze (and oxygen) in Breeze::Metrics, because it makes maintainability much easier. I think it should stays so, and not become split between Metrics and Helper. In the current implementation I see two ways to do this: - the easy way: move the enums back to Breeze::Metrics, using e.g. ScrollBar_Extend_Tiny, ScrollBar_SliderWidth_Tiny, ScrollBar_MinSliderHeight_Tiny (and the same widht _Default and with _Large) and let Helper (as it is now), switch between one set and the other (with no more magic nubers - the not so easy way: make metrics a real class and move all the new Helper code there. Either way is fine with me, for the moment. 2/ on the actual feature side: - Large: I really don't think we need this. It looks oversized with respect to the other breeze elements, and nobody (as far as I know) never asked for larger scrollbars. I think "small" and "medium" should be enough. And I would rename them to "default" and large. (if indeed small is the default) - small: I think scrollBar_MinSliderHeight should be increased to 10 (or 12). Hitting the handle when the scrollbar has its minimum size at 6, becomes very difficult (try open a large file in e.g. kate or kwrite). This makes the handle non-circular, but I think this is acceptable. Still on small: I think there is something not elegant with the size of the groove and the size of the arrows on both sides of the scrollbar, when visible. arrows appear too large. Should we make them smaller ? Disable them by default ? making them smaller has the inconvenient that it will become inconsistent with the other arrows in the rest of the style. On the other hand, using 8 instead of 6 for the groove width, make the two more balanced. (but then it makes again the scrollbar larger than the progressbars). Finally: on the need for an option (and this is true with the other commit). Do we really need one ? Will we add an option every time we change something in the style's appearance from now on ? I can see how this will turn breeze into qtcurve in the future, and make it unmaintainable. Also I thought the original idea was to make breeze consistent by design, so that you do not need options. In fact, options would be a nuisance: they would either break consistency, or make it appear as if we - or rather the designers- are not sure about their designs. I think Nuno's back in the oxygen days, also agreed with that. In other words, I do not think having an option for whether scrollbar grove is shown on mouse-over or not, and another one on the width of the scrollbar, enters the category of "powerful when needed", so as to justify an option (well two options). I would _really_ like to have the oppinion of VDG on that, but IMO, if we are sure about ourselves on this new scrollbar design we should just make these two defaults the only available options and stick to it. Sorry for the long posting REPOSITORY rBREEZE Breeze REVISION DETAIL https://phabricator.kde.org/D3210 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma, #vdg, hpereiradacosta Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
