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.
Details
Diff Detail
- Repository
- R255 Elisa
- Branch
- player_control_width
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 13143 Build 13161: arc lint + arc unit
Basically a tweaked 21430984e5a4 up for review, which I reverted since @ngraham raised concerns
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?
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.
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.
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.
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! :)