Differential D23367

make the sizing of the toolbuttons more coherent
ClosedPublic
Actions

Authored by camiloh on Aug 22 2019, 10:36 PM.

Details

Reviewers
mart
ngraham
Group Reviewers
Plasma
Commits
R242:3887f5ec9152: make the sizing of the toolbuttons more coherent
R242:069b4e95c155: make the sizing of the toolbuttons more coherent
Summary

this makes the toolbutton icons follow a standard size.
Maybe one could make it follow the icon group size props? like icon.width and icon.height if those props are set.

Before patch:

After patch:

Diff Detail

Repository
R242 Plasma Framework (Library)
Branch
toolbutton-icon-sizing (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15545
Build 15563: arc lint + arc unit
camiloh created this revision.Aug 22 2019, 10:36 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 22 2019, 10:36 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
camiloh requested review of this revision.Aug 22 2019, 10:36 PM
apol added a subscriber: apol.Aug 23 2019, 9:26 AM
apol added inline comments.
src/declarativeimports/plasmacomponents3/ToolButton.qml
56 ↗(On Diff #64368)

That's always the case, isn't it? if a parent isn't visible, an item will never be visible.

camiloh marked an inline comment as done.Aug 23 2019, 4:07 PM
camiloh added inline comments.
src/declarativeimports/plasmacomponents3/ToolButton.qml
56 ↗(On Diff #64368)

yes. you're right.

camiloh updated this revision to Diff 64432.Aug 23 2019, 4:13 PM
camiloh marked an inline comment as done.

if parent is nto visible then children are not visible. fix

camiloh edited the summary of this revision. (Show Details)Aug 23 2019, 4:13 PM
ngraham added a comment.Aug 23 2019, 5:34 PM

Not that it's an inappropriate fix, but are you sure you want to be using PlasmaComponents ToolButtons in Index? Why not use regular old QQC2 ToolButtons?

camiloh added a comment.EditedAug 23 2019, 7:32 PM
In D23367#517658, @ngraham wrote:

Not that it's an inappropriate fix, but are you sure you want to be using PlasmaComponents ToolButtons in Index? Why not use regular old QQC2 ToolButtons?

My intention with these patches is to "fix" visual issues affecting apps on PlaMo platform, issues that come from the Plasma style.
Apps using regular QQC2 components are no as nicely styled with the Plasma style as they are with others styles intended for standalone applications. some of the issues are affecting QCC2.ToolButton, QCC2.Button, QCC2.ToolBar... etc.
Index is using regular old QQC2 ToolButtons.

From the screenshots, the current style, makes QCC2 ToolButton be huge, taking the size from the layout. The idea is to make the ToolButton size standard

ngraham accepted this revision.Aug 23 2019, 9:20 PM

I see, that's just a test.

LGTM!

This revision is now accepted and ready to land.Aug 23 2019, 9:20 PM
Closed by commit R242:3887f5ec9152: make the sizing of the toolbuttons more coherent (authored by camiloh). · Explain WhyAug 23 2019, 9:35 PM
This revision was automatically updated to reflect the committed changes.
broulik added a subscriber: broulik.Aug 28 2019, 8:09 AM

This breaks the sizing of larger buttons like in media controller:

mart added inline comments.Aug 28 2019, 8:40 AM
src/declarativeimports/plasmacomponents3/ToolButton.qml
56 ↗(On Diff #64368)

This fixes the icons to a single size no matter what, and wether it fits or not, and is going to break badly any time a bigger or smaller buttons is needed, as shown by breaking media controller.

I have to revert this now, the icon should scale only to "standard" sizes, but still scale with the button size.
if you really need fixed icons no matter what, this size hint may follow icon.width/icon.height from the control iff set to a valid measure

mart reopened this revision.Aug 28 2019, 8:41 AM
This revision is now accepted and ready to land.Aug 28 2019, 8:41 AM
mart requested changes to this revision.Aug 28 2019, 8:42 AM
This revision now requires changes to proceed.Aug 28 2019, 8:42 AM
mart added a comment.Aug 28 2019, 8:57 AM
In D23367#517658, @ngraham wrote:

Not that it's an inappropriate fix, but are you sure you want to be using PlasmaComponents ToolButtons in Index? Why not use regular old QQC2 ToolButtons?

The phone is using the plasma style for its qqc2

mart added inline comments.Aug 28 2019, 9:00 AM
src/declarativeimports/plasmacomponents3/ToolButton.qml
49 ↗(On Diff #64368)

this further item enclosing is not necessary, is an object instantation more for no functional gain

55 ↗(On Diff #64368)

This was ok as it was, including fillwidth/fillheight (perhaps)
but with the line:
Layout.preferredWidth: control.icon.width > 0 ? control.icon.width : -1
same for preferredHeight

camiloh updated this revision to Diff 64832.Aug 28 2019, 12:30 PM

make usage of the icon group properties: height and width

camiloh updated this revision to Diff 64844.Aug 28 2019, 1:23 PM

make the icon fill height and width if the icon group height or width props are not set or are 0

mart accepted this revision.Aug 28 2019, 1:38 PM
This revision is now accepted and ready to land.Aug 28 2019, 1:38 PM
Closed by commit R242:069b4e95c155: make the sizing of the toolbuttons more coherent (authored by camiloh). · Explain WhyAug 28 2019, 1:59 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 64432

View Options

src/declarativeimports/plasmacomponents3/ToolButton.qml

Loading...