From kde-panel-devel Mon Oct 31 16:14:15 2016 From: "hpereiradacosta (Hugo Pereira Da Costa)" Date: Mon, 31 Oct 2016 16:14:15 +0000 To: kde-panel-devel Subject: [Differential] [Updated] D3210: make scrollbar size configurable Message-Id: <20161031161414.34695.28530.3BC74DB9 () phabricator ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-panel-devel&m=147793046918760 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--13cc19c6ffb14398a74d5ff58c6731ac" --13cc19c6ffb14398a74d5ff58c6731ac Content-Type: text/plain; charset="ascii" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit 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 --13cc19c6ffb14398a74d5ff58c6731ac Content-Type: text/html; charset="ascii" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable hpereiradacosta=20added=20a=20comment. View=20Revision

Hello=20Marco, Thanks=20for=20the=20patch.=20 1/=20On=20the=20implementation=20side:=20I=20have=20tried=20so=20far=20to= =20keep=20all=20the=20magic=20numbers=20for=20breeze=20(and=20oxygen)=20in= =20Breeze::Metrics,=20because=20it=20makes=20maintainability=20much=20easie= r.=20I=20think=20it=20should=20stays=20so,=20and=20not=20become=20split=20b= etween=20Metrics=20and=20Helper.=20In=20the=20current=20implementation=20I= =20see=20two=20ways=20to=20do=20this:

the=20easy=20way:=20move=20the=20enums= =20back=20to=20Breeze::Metrics,=20using=20e.g.

ScrollBar_Extend_Tiny,=20ScrollBar_SliderWidth_Tiny,=20ScrollBar_MinSlid= erHeight_Tiny=20(and=20the=20same=20widht=20_Default=20and=20with=20_Large)= =20and=20let=20Helper=20(as=20it=20is=20now),=20switch=20between=20one=20se= t=20and=20the=20other=20(with=20no=20more=20magic=20nubers

the=20not=20so=20easy=20way:=20make=20me= trics=20a=20real=20class=20and=20move=20all=20the=20new=20Helper=20code=20t= here.

Either=20way=20is=20fine=20with=20me,=20for=20the=20moment.

2/=20on=20the=20actual=20feature=20side:

Large:=20I=20really=20don't=20think= =20we=20need=20this.=20It=20looks=20oversized=20with=20respect=20to=20the= =20other=20breeze=20elements,=20and=20nobody=20(as=20far=20as=20I=20know)= =20never=20asked=20for=20larger=20scrollbars.=20I=20think=20"small&quo= t;=20and=20"medium"=20should=20be=20enough.=20And=20I=20would=20r= ename=20them=20to=20"default"=20and=20large.=20(if=20indeed=20sma= ll=20is=20the=20default) small:=20=20I=20think=20scrollBar_MinSli= derHeight=20should=20be=20increased=20to=2010=20(or=2012).=20Hitting=20the= =20handle=20when=20the=20scrollbar=20has=20its=20minimum=20size=20at=206,= =20becomes=20very=20difficult=20(try=20open=20a=20large=20file=20in=20e.g.= =20kate=20or=20kwrite).=20This=20makes=20the=20handle=20non-circular,=20but= =20I=20think=20this=20is=20acceptable.

Still=20on=20small:=20I=20think=20there=20is=20something=20not=20elegant= =20with=20the=20size=20of=20the=20groove=20and=20the=20size=20of=20the=20ar= rows=20on=20both=20sides=20of=20the=20scrollbar,=20when=20visible.=20arrows= =20appear=20too=20large.=20 Should=20we=20make=20them=20smaller=20?=20 Disable=20them=20by=20default=20?=20 making=20them=20smaller=20has=20the=20inconvenient=20that=20it=20will=20bec= ome=20inconsistent=20with=20the=20other=20arrows=20in=20the=20rest=20of=20t= he=20style.

On=20the=20other=20hand,=20using=208=20instead=20of=206=20for=20the=20gr= oove=20width,=20make=20the=20two=20more=20balanced.=20(but=20then=20it=20ma= kes=20again=20the=20scrollbar=20larger=20than=20the=20progressbars).

Finally:=20on=20the=20need=20for=20an=20option=20(and=20this=20is=20true= =20with=20the=20other=20commit).=20 Do=20we=20really=20need=20one=20?=20Will=20we=20add=20an=20option=20every= =20time=20we=20change=20something=20in=20the=20style's=20appearance=20= from=20now=20on=20?=20 I=20can=20see=20how=20this=20will=20turn=20breeze=20into=20qtcurve=20in=20t= he=20future,=20and=20make=20it=20unmaintainable.

Also=20I=20thought=20the=20original=20idea=20was=20to=20make=20breeze=20= consistent=20by=20design,=20=20so=20that=20you=20do=20not=20need=20options.= =20In=20fact,=20options=20would=20be=20a=20nuisance:=20they=20would=20eithe= r=20break=20consistency,=20or=20make=20it=20appear=20as=20if=20we=20-=20or= =20rather=20the=20designers-=20are=20not=20sure=20about=20their=20designs.<= /p>

I=20think=20Nuno's=20back=20in=20the=20oxygen=20days,=20also=20agre= ed=20with=20that.

In=20other=20words,=20I=20do=20not=20think=20having=20an=20option=20for= =20whether=20scrollbar=20grove=20is=20shown=20on=20mouse-over=20or=20not,= =20and=20another=20one=20on=20the=20width=20of=20the=20scrollbar,=20enters= =20the=20category=20of=20"powerful=20when=20needed",=20so=20as=20= to=20justify=20an=20option=20(well=20two=20options).

I=20would=20_really_=20like=20to=20have=20the=20oppinion=20of=20VDG=20on= =20that,=20but=20IMO,=20if=20we=20are=20sure=20about=20ourselves=20on=20thi= s=20new=20scrollbar=20design=20we=20should=20just=20make=20these=20two=20de= faults=20the=20only=20available=20options=20and=20stick=20to=20it.

Sorry=20for=20the=20long=20posting

R= EPOSITORY
rBREEZE=20Breeze
= REVISION=20DETAIL
https://phabricator.kde.org/D3210
EMAIL=20PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To:=20mart,=20Plasma,=20VDG,=20hperei= radacostaCc:=20plasma-devel,=20lesliezhai,=20ali-m= ohamed,=20jensreuterberg,=20abetts,=20sebas
--13cc19c6ffb14398a74d5ff58c6731ac--