~eliasnaur/gio-patches

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
7 2

[PATCH] widget/material: constrain button insets

Details
Message ID
<20200213145338.83460-1-larry@theclapp.org>
DKIM signature
missing
Download raw message
Patch: +25 -2
Constrain button insets to gtx.Constraints.

Signed-off-by: Larry Clapp <larry@theclapp.org>
---
 widget/material/button.go | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/widget/material/button.go b/widget/material/button.go
index ba9c8e6..ad78bb8 100644
--- a/widget/material/button.go
+++ b/widget/material/button.go
@@ -60,7 +60,9 @@ func (b Button) Layout(gtx *layout.Context, button *widget.Button) {
	col := b.Color
	bgcol := b.Background
	hmin := gtx.Constraints.Width.Min
	hmax := gtx.Constraints.Width.Max
	vmin := gtx.Constraints.Height.Min
	vmax := gtx.Constraints.Height.Max
	layout.Stack{Alignment: layout.Center}.Layout(gtx,
		layout.Expanded(func() {
			rr := float32(gtx.Px(unit.Dp(4)))
@@ -80,9 +82,23 @@ func (b Button) Layout(gtx *layout.Context, button *widget.Button) {
			gtx.Constraints.Width.Min = hmin
			gtx.Constraints.Height.Min = vmin
			layout.Center.Layout(gtx, func() {
				layout.Inset{Top: unit.Dp(10), Bottom: unit.Dp(10), Left: unit.Dp(12), Right: unit.Dp(12)}.Layout(gtx, func() {
				// Draw the label into a macro to get its dimensions, so we can
				// constrain the inset.
				var m op.MacroOp
				m.Record(gtx.Ops)
				widget.Label{}.Layout(gtx, b.shaper, b.Font, b.TextSize, b.Text)
				m.Stop()
				labelDims := gtx.Dimensions

				// Constrain the inset to the outer max h/v, minus the label size
				hInset := unit.Px(float32(min(gtx.Px(unit.Dp(24)), hmax-labelDims.Size.X)) / 2)
				vInset := unit.Px(float32(min(gtx.Px(unit.Dp(20)), vmax-labelDims.Size.Y)) / 2)

				// Finally, draw the label inside the inset.
				layout.Inset{Top: vInset, Bottom: vInset, Left: hInset, Right: hInset}.Layout(gtx, func() {
					paint.ColorOp{Color: col}.Add(gtx.Ops)
					widget.Label{}.Layout(gtx, b.shaper, b.Font, b.TextSize, b.Text)
					m.Add()
					gtx.Dimensions = labelDims
				})
			})
			pointer.Rect(image.Rectangle{Max: gtx.Dimensions.Size}).Add(gtx.Ops)
@@ -91,6 +107,13 @@ func (b Button) Layout(gtx *layout.Context, button *widget.Button) {
	)
}

func min(a, b int) int {
	if a < b {
		return a
	}
	return b
}

func (b IconButton) Layout(gtx *layout.Context, button *widget.Button) {
	layout.Stack{}.Layout(gtx,
		layout.Expanded(func() {
-- 
2.25.0
Details
Message ID
<C0L78WK1KJXR.3LM2WPT62WFQ2@toolbox>
In-Reply-To
<20200213145338.83460-1-larry@theclapp.org> (view parent)
DKIM signature
missing
Download raw message
On Thu Feb 13, 2020 at 09:53, Larry Clapp wrote:
> Constrain button insets to gtx.Constraints.
>

Thanks. Why not adjust insets in layout.Insets directly?
Details
Message ID
<CAE_4BPAKXoguLRvpQ5MDCC_aYFkDhphQh7U2TAvBtMLgqC1yTA@mail.gmail.com>
In-Reply-To
<C0L78WK1KJXR.3LM2WPT62WFQ2@toolbox> (view parent)
DKIM signature
missing
Download raw message
You mean, like, pass inset values (or maybe just an Inset object) into
the Layout function?  That seemed pretty intrusive.  Also, I know
we're trying to conform to the Material Design spec (which I think is
a good idea, in general), and I assumed that the insets you'd
hardcoded conformed to that spec, and (in general) shouldn't be
changed.  But I also wanted a shorter button, so I made it care about
the constraints I gave it.

If you meant something else, well, please elaborate.  :)

— L

On Thu, Feb 13, 2020 at 12:07 PM Elias Naur <mail@eliasnaur.com> wrote:
>
> On Thu Feb 13, 2020 at 09:53, Larry Clapp wrote:
> > Constrain button insets to gtx.Constraints.
> >
>
> Thanks. Why not adjust insets in layout.Insets directly?
Details
Message ID
<C0LWS0QYN5JT.1JPRPG7WYY3UY@toolbox>
In-Reply-To
<CAE_4BPAKXoguLRvpQ5MDCC_aYFkDhphQh7U2TAvBtMLgqC1yTA@mail.gmail.com> (view parent)
DKIM signature
missing
Download raw message
On Thu Feb 13, 2020 at 12:39, Larry Clapp wrote:
> You mean, like, pass inset values (or maybe just an Inset object) into
> the Layout function?  That seemed pretty intrusive.  Also, I know
> we're trying to conform to the Material Design spec (which I think is
> a good idea, in general), and I assumed that the insets you'd
> hardcoded conformed to that spec, and (in general) shouldn't be
> changed.  But I also wanted a shorter button, so I made it care about
> the constraints I gave it.
>
> If you meant something else, well, please elaborate.  :)
>

Sorry for not being clear. I'm wondering whether it would be useful to add this
inset squeezing logic to the layout.Inset.Layout method? More precisely,
layout.Inset can be changed to cope with its child widget overflowing the inset
constraints, by adjusting the final TransformOp at

	https://git.sr.ht/~eliasnaur/gio/tree/master/layout/layout.go#L125

the same way you're doing it in your patch.

The advantage would be a more robust layout.Inset and simpler code in
Button.Layout.

-- elias
Details
Message ID
<CAE_4BPDhszRxfXp_q8unC5AnEk6235Mmc9QX2k2nHdsL_u-WdA@mail.gmail.com>
In-Reply-To
<C0LWS0QYN5JT.1JPRPG7WYY3UY@toolbox> (view parent)
DKIM signature
missing
Download raw message
On Fri, Feb 14, 2020 at 8:07 AM Elias Naur <mail@eliasnaur.com> wrote:
> Sorry for not being clear. I'm wondering whether it would be useful to add this
> inset squeezing logic to the layout.Inset.Layout method? More precisely,
> layout.Inset can be changed to cope with its child widget overflowing the inset
> constraints, by adjusting the final TransformOp

So I tried this, and maybe I did it wrong, but I couldn't get it to work.

It turns out that a lot of widgets will take whatever space you give
them (reasonably enough), so unless you reduce the constraint by the
insets, they'll take up that space.

And if you *do* reduce the constraints by the insets, then it's
impossible for the widget to take up more space than that, so there's
no point in reducing the insets, because of course there's space for
them.

You're welcome to give it a shot, but I couldn't get it to work.
(Though again, maybe I did it wrong.)  My buttons in my app were fine,
but the "kitchen" example insisted on drawing flush-left with 0 inset.

— L
Details
Message ID
<C0MZ2J6MCWOT.2XYSNMFZ6UOSU@toolbox>
In-Reply-To
<CAE_4BPDhszRxfXp_q8unC5AnEk6235Mmc9QX2k2nHdsL_u-WdA@mail.gmail.com> (view parent)
DKIM signature
missing
Download raw message
On Sat Feb 15, 2020 at 12:40, Larry Clapp wrote:
> On Fri, Feb 14, 2020 at 8:07 AM Elias Naur <mail@eliasnaur.com> wrote:
> > Sorry for not being clear. I'm wondering whether it would be useful to add this
> > inset squeezing logic to the layout.Inset.Layout method? More precisely,
> > layout.Inset can be changed to cope with its child widget overflowing the inset
> > constraints, by adjusting the final TransformOp
>
> So I tried this, and maybe I did it wrong, but I couldn't get it to work.
>
> It turns out that a lot of widgets will take whatever space you give
> them (reasonably enough), so unless you reduce the constraint by the
> insets, they'll take up that space.
>
> And if you *do* reduce the constraints by the insets, then it's
> impossible for the widget to take up more space than that, so there's
> no point in reducing the insets, because of course there's space for
> them.
>
> You're welcome to give it a shot, but I couldn't get it to work.
> (Though again, maybe I did it wrong.)  My buttons in my app were fine,
> but the "kitchen" example insisted on drawing flush-left with 0 inset.
>

I see now why my idea wasn't any good, sorry for wasting your time.

However, I'm still not convinced your approach is a good idea. For example,
your method will not support button labels that otherwise fall back to multiple
lines if insets were preserved (e.g. a "Delete now" that displays as
"Delete\nnow" on two lines when squeezed).

How about adding an Insets field to Button, initialized by the
material.Theme.Button method, and honored in material.Button.Layout? You can
then tweak the insets (possibly to 0) before calling Layout.

-- elias
Details
Message ID
<CAE_4BPCyZE+JzhFGH2wM7=uQ8JJ+jHuoRss-4LssEdS7EnQTzg@mail.gmail.com>
In-Reply-To
<C0MZ2J6MCWOT.2XYSNMFZ6UOSU@toolbox> (view parent)
DKIM signature
missing
Download raw message
On Sat, Feb 15, 2020 at 2:08 PM Elias Naur <mail@eliasnaur.com> wrote:
> I see now why my idea wasn't any good, sorry for wasting your time.

No worries.

> However, I'm still not convinced your approach is a good idea. For example,
> your method will not support button labels that otherwise fall back to multiple
> lines if insets were preserved (e.g. a "Delete now" that displays as
> "Delete\nnow" on two lines when squeezed).
>
> How about adding an Insets field to Button, initialized by the
> material.Theme.Button method, and honored in material.Button.Layout? You can
> then tweak the insets (possibly to 0) before calling Layout.

That does seem to work.  See new patch "allow Button Inset to be customizable".

That said, this still doesn't automatically adjust a button's insets
given its constraints, but at least it makes it *possible* to do it
manually.

Thanks for your help.

— L
Details
Message ID
<C0NFSBX8TXT4.2EDYIV0QIZG00@testmac>
In-Reply-To
<CAE_4BPCyZE+JzhFGH2wM7=uQ8JJ+jHuoRss-4LssEdS7EnQTzg@mail.gmail.com> (view parent)
DKIM signature
missing
Download raw message
On Sat Feb 15, 2020 at 3:49 PM, Larry Clapp wrote:
> On Sat, Feb 15, 2020 at 2:08 PM Elias Naur <mail@eliasnaur.com> wrote:
> > I see now why my idea wasn't any good, sorry for wasting your time.
>
> 
> No worries.
>
> 
> > However, I'm still not convinced your approach is a good idea. For example,
> > your method will not support button labels that otherwise fall back to multiple
> > lines if insets were preserved (e.g. a "Delete now" that displays as
> > "Delete\nnow" on two lines when squeezed).
> >
> > How about adding an Insets field to Button, initialized by the
> > material.Theme.Button method, and honored in material.Button.Layout? You can
> > then tweak the insets (possibly to 0) before calling Layout.
>
> 
> That does seem to work. See new patch "allow Button Inset to be
> customizable".
>

It appears the patch didn't get through. Can you please resend the patch
to my address? I'm very sorry for the inconvenience.

	https://todo.sr.ht/~sircmpwn/lists.sr.ht/138