~gioverse/chat

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

[PATCH chat v1 0/3] plato style tweaks

Details
Message ID
<20210727074631.697-1-jackmordaunt.dev@gmail.com>
DKIM signature
pass
Download raw message
In preparation for typing indicators.
Since I'm re-using the base MessageStyle to implement the indicators, 
I would like to optionally remove the timestamp and read receipt from 
the message bubble. Thus this patch implements compact messages. 

Since typing indicators wont necessarily be interactive, I made the
RowStyle api safe to use with nil state and nil interact. 

Jack Mordaunt (3):
  widget/plato: make row safe to use with nil state
  widget/plato: compact message layout when configured with empty
    timestamp
  widget/plato: style tweaks

 widget/plato/message.go | 25 ++++++++++++++++++++++---
 widget/plato/plato.go   |  2 +-
 widget/plato/row.go     |  9 ++++++++-
 3 files changed, 31 insertions(+), 5 deletions(-)

-- 
2.32.0.windows.1

[PATCH chat v1 1/3] widget/plato: make row safe to use with nil state

Details
Message ID
<20210727074631.697-2-jackmordaunt.dev@gmail.com>
In-Reply-To
<20210727074631.697-1-jackmordaunt.dev@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +6 -0
Avoid panicking when caller passes nil'd values.

Signed-off-by: Jack Mordaunt <jackmordaunt.dev@gmail.com>
---
 widget/plato/row.go | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/widget/plato/row.go b/widget/plato/row.go
index ee9f22e..7c4ce4a 100644
--- a/widget/plato/row.go
+++ b/widget/plato/row.go
@@ -60,6 +60,12 @@ func NewRow(
	menu *component.MenuState,
	msg RowConfig,
) RowStyle {
	if interact == nil {
		interact = &chatwidget.Row{}
	}
	if menu == nil {
		menu = &component.MenuState{}
	}
	interact.Avatar.Cache(msg.Avatar)
	ms := RowStyle{
		Row: chatlayout.Row{
-- 
2.32.0.windows.1

[PATCH chat v1 2/3] widget/plato: compact message layout when configured with empty timestamp

Details
Message ID
<20210727074631.697-3-jackmordaunt.dev@gmail.com>
In-Reply-To
<20210727074631.697-1-jackmordaunt.dev@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +14 -1
This allows sensible re-use for typing indicators by removing timestamp
and read-receipt.

Compactness is determined by whether there is a non-zero time for this 
"message". 

Signed-off-by: Jack Mordaunt <jackmordaunt.dev@gmail.com>
---
 widget/plato/message.go | 14 +++++++++++++-
 widget/plato/row.go     |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/widget/plato/message.go b/widget/plato/message.go
index a4d1866..ebfdd78 100644
--- a/widget/plato/message.go
+++ b/widget/plato/message.go
@@ -46,6 +46,8 @@ type MessageStyle struct {
	// Clickable indicates whether the message content should be able to receive
	// click events.
	Clickable bool
	// Compact mode avoids laying out timestamp and read-receipt.
	Compact bool
}

// MessageConfig describes aspects of a chat message.
@@ -59,6 +61,8 @@ type MessageConfig struct {
	// Color of the message bubble.
	// Defaults to LocalMessageColor.
	Color color.NRGBA
	// Compact mode avoids laying out timestamp and read-receipt.
	Compact bool
}

// Message constructs a MessageStyle with sensible defaults.
@@ -90,7 +94,8 @@ func Message(th *material.Theme, interact *chatwidget.Message, msg MessageConfig
			l.Color = component.WithAlpha(l.Color, 200)
			return l
		}(),
		Receipt:   TickIcon,
		Receipt: TickIcon,
		Compact: msg.Compact,
	}
}

@@ -130,6 +135,13 @@ func (m MessageStyle) Layout(gtx C) D {
	if m.NinePatch != nil {
		surface = m.NinePatch.Layout
	}
	if m.Compact {
		return surface(gtx, func(gtx C) D {
			return m.ContentPadding.Layout(gtx, func(gtx C) D {
				return m.Content.Layout(gtx)
			})
		})
	}
	macro := op.Record(gtx.Ops)
	dims := m.ContentPadding.Layout(gtx, func(gtx C) D {
		return m.Content.Layout(gtx)
diff --git a/widget/plato/row.go b/widget/plato/row.go
index 7c4ce4a..eedd8b9 100644
--- a/widget/plato/row.go
+++ b/widget/plato/row.go
@@ -104,6 +104,7 @@ func NewRow(
				}
				return NonLocalMessageColor
			}(),
			Compact: msg.SentAt == (time.Time{}),
		}),
	}
	ms.UserInfoStyle.Local = msg.Local
-- 
2.32.0.windows.1

[PATCH chat v1 3/3] widget/plato: style tweaks

Details
Message ID
<20210727074631.697-4-jackmordaunt.dev@gmail.com>
In-Reply-To
<20210727074631.697-1-jackmordaunt.dev@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +11 -4
Ensure avatar aligns when message is compact, and read-receipt is padded
off the outside the same as the content, but not on in the inside.

This is a balancing act of data vlaues. The data dependencies are
somewhat implicit here because the decisions must coincide but are
made independently. 

A better solution is to make the data dependencies more concrete via 
structure, but I think this is okay for now.

More precisely, there are two such dependencies:

1. if the avatar is too large or too small it doesn't align vertically
with the bubble
2. the read-receipt must be padded by the same amount as the content, 
but there should be no padding between them
	- wrapping them both in the same padding interacts with the macro in  
	a way I don't understand

Signed-off-by: Jack Mordaunt <jackmordaunt.dev@gmail.com>
---
 widget/plato/message.go | 11 +++++++++--
 widget/plato/plato.go   |  2 +-
 widget/plato/row.go     |  2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/widget/plato/message.go b/widget/plato/message.go
index ebfdd78..34edf66 100644
--- a/widget/plato/message.go
+++ b/widget/plato/message.go
@@ -143,7 +143,11 @@ func (m MessageStyle) Layout(gtx C) D {
		})
	}
	macro := op.Record(gtx.Ops)
	dims := m.ContentPadding.Layout(gtx, func(gtx C) D {
	dims := layout.Inset{
		Top:   m.ContentPadding.Top,
		Left:  m.ContentPadding.Left,
		Right: m.ContentPadding.Right,
	}.Layout(gtx, func(gtx C) D {
		return m.Content.Layout(gtx)
	})
	call := macro.Stop()
@@ -167,7 +171,10 @@ func (m MessageStyle) Layout(gtx C) D {
							width = dims.Size.X
						}
						gtx.Constraints.Max.X = width
						return m.ContentPadding.Layout(gtx, func(gtx C) D {
						return layout.Inset{
							Bottom: m.ContentPadding.Right,
							Right:  m.ContentPadding.Bottom,
						}.Layout(gtx, func(gtx C) D {
							return layout.Flex{
								Axis:      layout.Horizontal,
								Alignment: layout.Middle,
diff --git a/widget/plato/plato.go b/widget/plato/plato.go
index c7cc250..e8edb15 100644
--- a/widget/plato/plato.go
+++ b/widget/plato/plato.go
@@ -22,7 +22,7 @@ var (
	DefaultMaxImageHeight  = unit.Dp(400)
	DefaultMaxMessageWidth = unit.Dp(600)
	DefaultMinMessageWidth = unit.Dp(80)
	DefaultAvatarSize      = unit.Dp(32)
	DefaultAvatarSize      = unit.Dp(28)
	LocalMessageColor      = color.NRGBA{R: 63, G: 133, B: 232, A: 255}
	NonLocalMessageColor   = color.NRGBA{R: 50, G: 50, B: 50, A: 255}
)
diff --git a/widget/plato/row.go b/widget/plato/row.go
index eedd8b9..343d704 100644
--- a/widget/plato/row.go
+++ b/widget/plato/row.go
@@ -154,7 +154,7 @@ func (c RowStyle) layoutBubble(gtx C) D {
// layoutAvatar lays out the user avatar image.
func (c RowStyle) layoutAvatar(gtx C) D {
	return layout.Inset{
		Top:    unit.Dp(12),
		Top:    unit.Dp(8),
		Bottom: unit.Dp(6),
		Left:   unit.Dp(6),
		Right:  unit.Dp(6),
-- 
2.32.0.windows.1

Re: [PATCH chat v1 1/3] widget/plato: make row safe to use with nil state

Details
Message ID
<CD5S2T45N3C6.20COCFBN52PR3@vendetta>
In-Reply-To
<20210727074631.697-2-jackmordaunt.dev@gmail.com> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
Hey Jack,

The rest of the patchset looks great. I have one gripe on this one,
but I've merged anyway.

Cheers,
Chris

On Tue Jul 27, 2021 at 3:46 AM EDT, Jack Mordaunt wrote:
> Avoid panicking when caller passes nil'd values.
>
> Signed-off-by: Jack Mordaunt <jackmordaunt.dev@gmail.com>
> ---
> widget/plato/row.go | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/widget/plato/row.go b/widget/plato/row.go
> index ee9f22e..7c4ce4a 100644
> --- a/widget/plato/row.go
> +++ b/widget/plato/row.go
> @@ -60,6 +60,12 @@ func NewRow(
> menu *component.MenuState,
> msg RowConfig,
> ) RowStyle {
> + if interact == nil {
> + interact = &chatwidget.Row{}
> + }
> + if menu == nil {
> + menu = &component.MenuState{}
> + }

It kills me that we allocate to the heap twice every time we do this.
I really wish there was a better way. It's not intuitive to the user
that their decision not to allocate/provide state will end up causing
a state allocation.

That being said, I tried to rework the API around a non-allocating
approach, and it got complex fast. I've decided to merge this for the
time being, but I'm not super happy with how it works.

> interact.Avatar.Cache(msg.Avatar)
> ms := RowStyle{
> Row: chatlayout.Row{
> --
> 2.32.0.windows.1

Re: [PATCH chat v1 1/3] widget/plato: make row safe to use with nil state

Details
Message ID
<EFE1CC08-DE6B-4B0D-8183-D1D3C084FE00@gmail.com>
In-Reply-To
<CD5S2T45N3C6.20COCFBN52PR3@vendetta> (view parent)
DKIM signature
pass
Download raw message

> On 30 Jul 2021, at 12:51 am, Chris Waldon <christopher.waldon.dev@gmail.com> wrote:
> 
> Hey Jack,
> 
> The rest of the patchset looks great. I have one gripe on this one,
> but I've merged anyway.
> 
> Cheers,
> Chris
> 
>> On Tue Jul 27, 2021 at 3:46 AM EDT, Jack Mordaunt wrote:
>> Avoid panicking when caller passes nil'd values.
>> 
>> Signed-off-by: Jack Mordaunt <jackmordaunt.dev@gmail.com>
>> ---
>> widget/plato/row.go | 6 ++++++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/widget/plato/row.go b/widget/plato/row.go
>> index ee9f22e..7c4ce4a 100644
>> --- a/widget/plato/row.go
>> +++ b/widget/plato/row.go
>> @@ -60,6 +60,12 @@ func NewRow(
>> menu *component.MenuState,
>> msg RowConfig,
>> ) RowStyle {
>> + if interact == nil {
>> + interact = &chatwidget.Row{}
>> + }
>> + if menu == nil {
>> + menu = &component.MenuState{}
>> + }
> 
> It kills me that we allocate to the heap twice every time we do this.
> I really wish there was a better way. It's not intuitive to the user
> that their decision not to allocate/provide state will end up causing
> a state allocation.
> 
> That being said, I tried to rework the API around a non-allocating
> approach, and it got complex fast. I've decided to merge this for the
> time being, but I'm not super happy with how it works.
> 

I wasn’t thinking about heap allocations at the time, but now that you’ve mentioned it I completely agree. 

I’ll think about some strategies.

Off the top of my head:

First strategy that comes to mind is to just use a predeclared zero value as a package global.

Perhaps changing the signature to use values instead of pointers could work.

Or even another constructor that doesn’t accept any state eg for a “stateless” row. 

Anyway, good catch! 

>> interact.Avatar.Cache(msg.Avatar)
>> ms := RowStyle{
>> Row: chatlayout.Row{
>> --
>> 2.32.0.windows.1
> 
Reply to thread Export thread (mbox)