Set height and width for MediaPlayerControl buttons
AbandonedPublic

Authored by astippich on Jun 20 2019, 5:58 PM.

Details

Reviewers
ngraham
mgallien
Summary

Set the height and width of the buttons in the media player
control such that they do not change their size when different
styles are used.

Diff Detail

Repository
R255 Elisa
Branch
player_control_width
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13142
Build 13160: arc lint + arc unit
astippich requested review of this revision.Jun 20 2019, 5:58 PM
astippich created this revision.

Basically a tweaked 21430984e5a4 up for review, which I reverted since @ngraham raised concerns

ngraham added a comment.EditedJun 20 2019, 10:08 PM

Thanks @astippich!

Do we even need to manually specify the width and height at all? To me they look just fine without these lines:

What problem does it solve to manually specify the height?

with different styles, this can look like:

after patch:

It seems like that bug should be fixed in FlatButtonWithToolTip then.

Why that? If you do not specify a button size, the style will automatically scale it. Also happes with a plain QML Button

astippich updated this revision to Diff 60335.Jun 22 2019, 12:31 PM
  • use Layout.preferredHeight/Width
astippich updated this revision to Diff 60337.Jun 22 2019, 12:35 PM
  • fix previous commit

Why that? If you do not specify a button size, the style will automatically scale it. Also happes with a plain QML Button

I'm saying that it makes sense to do this change in one place (the parent control) rather than in n places (all the instances of that control).

astippich retitled this revision from set implicitHeight and Width for MediaPlayerControl buttons to Set height and width for MediaPlayerControl buttons.Jun 22 2019, 5:26 PM

Why that? If you do not specify a button size, the style will automatically scale it. Also happes with a plain QML Button

I'm saying that it makes sense to do this change in one place (the parent control) rather than in n places (all the instances of that control).

That would mean hardcoding the size. FlatButtonWithTooltop could not be reused in e.g. the PlayListEntry then

Maybe we could just make the aspect ratio fixed to square in the FlatButtonWithToolTip control? Then we wouldn't need any custom sizing in the player control bar because the width would be limited by the height, which would be correctly chosen based on the height of the player bar.

Maybe we could just make the aspect ratio fixed to square in the FlatButtonWithToolTip control? Then we wouldn't need any custom sizing in the player control bar because the width would be limited by the height, which would be correctly chosen based on the height of the player bar.

I think that is somewhat prone to binding loop errors as you make a size dependent on another size.
Also, height might work for the player bar. For others, maybe the width is the critical size.

It is imho pretty normal to set the height and width of items, especially in a layout component
What I could do here is to define an additional component inside MediaPlayerControl that has the fixed size

True, but then again these are basically glorified ToolButtons, and you don't need to set the size for every ToolButton; it has a correct default size and you only override it as needed. It just seems odd to me that we have to do this at all, and I feel like it points to a problem in the component itself. It should have a correct default size without us having to tell every instance of it how big it needs to be.

Maybe I'm just being a grumpy pedantic old man though. :) I'll let @mgallien decide.

True, but then again these are basically glorified ToolButtons, and you don't need to set the size for every ToolButton; it has a correct default size and you only override it as needed. It just seems odd to me that we have to do this at all, and I feel like it points to a problem in the component itself. It should have a correct default size without us having to tell every instance of it how big it needs to be.

Maybe I'm just being a grumpy pedantic old man though. :) I'll let @mgallien decide.

I could base the component on a ToolButton. I actually do not remember the exact reason to base this on a normal Button when I started implementing it, it was probably because of some missing parts in the qqc2 style.
Funnily, this ToolButton then spans the full height of the player bar, which is what you initially complained about :)

LOL! That's probably because of the height of the player bar being hardcoded rather than sizing itself according to its contents + top and bottom padding (an anti-pattern best moved away from in QML user interfaces). I think basing this on a ToolButton rather than a Button is more semantically correct since that's what it is, so +1 if you want to do that.

astippich abandoned this revision.Jun 22 2019, 6:43 PM

Turns out that was some leftover code :) so the ToolButton does the correct sizing. I'll just abandon this one here and open a new one. Thanks for insisting! :)