When using an action in irc (also known as /me), you are writing in a
way that it feels like an external narrator is dictating what the user
is doing, instead of the user saying that themselves. Rendering this
text inside of a chat bubble breaks this model, so change the looks to
render it as an annoucement instead.
---
This is my first time writing in flutter, so please be critical with the
feedback! I'm particially uncertain about the following:
> if (isAction) {
> // isAction can only ever be true if we have a ctcp
> assert(ctcp != null);
> var actionText = stripAnsiFormatting(ctcp?.param ?? '');
If isAction is true, then ctcp must exist, as that is a pre-condition
for isAction being true. Dart doens't seem to be concined of this,
however, and instead requires a null-check on ctcp. The assert doesn't
seem to have made ctcp happy either. Is there a better way to handle
this?
> WidgetSpan(
> child: Container(
> width: 8.0,
> height: 8.0,
> margin: EdgeInsets.all(3.0),
> decoration: BoxDecoration(
> shape: BoxShape.circle,
> color: boxColor,
> ),
> ),
> ),
Is there a better way of drawing a circle? The size and margin are also
something I came up with experimentation, I am not sure if this would
still look as good if the user has a different font-size.
Feedback welcome!
lib/page/buffer.dart | 76 +++++++++++++++++++++++++++++---------------
1 file changed, 51 insertions(+), 25 deletions(-)
diff --git a/lib/page/buffer.dart b/lib/page/buffer.dart
index f1daf70..c5fac7f 100644
--- a/lib/page/buffer.dart+++ b/lib/page/buffer.dart
@@ -516,14 +516,17 @@ class _MessageItem extends StatelessWidget {
assert(ircMsg.cmd == 'PRIVMSG' || ircMsg.cmd == 'NOTICE');
var prevIrcMsg = prevMsg?.msg;
+ var prevCtcp = prevIrcMsg != null ? CtcpMessage.parse(prevIrcMsg) : null; var prevEntry = prevMsg?.entry;
var prevMsgSameSender = prevIrcMsg != null && ircMsg.source!.name == prevIrcMsg.source!.name;
+ var prevMsgIsAction = prevCtcp != null && prevCtcp.cmd == 'ACTION'; var nextMsgSameSender = nextMsg != null && ircMsg.source!.name == nextMsg!.msg.source!.name;
+ var isAction = ctcp != null && ctcp.cmd == 'ACTION'; var showUnreadMarker = prevEntry != null && unreadMarkerTime != null && unreadMarkerTime!.compareTo(entry.time) < 0 && unreadMarkerTime!.compareTo(prevEntry.time) >= 0;
var showDateMarker = prevEntry == null || !_isSameDate(localDateTime, prevEntry.dateTime.toLocal());
- var showSender = showUnreadMarker || !prevMsgSameSender;+ var showSender = showUnreadMarker || !prevMsgSameSender || (prevMsgIsAction != isAction); var showTime = !nextMsgSameSender || nextMsg!.entry.dateTime.difference(entry.dateTime) > Duration(minutes: 2);
var unreadMarkerColor = Theme.of(context).accentColor;
@@ -535,8 +538,14 @@ class _MessageItem extends StatelessWidget {
//var boxColor = Theme.of(context).accentColor;
var boxColor = colorScheme.primary;
var boxAlignment = Alignment.centerLeft;
- var textStyle = DefaultTextStyle.of(context).style.apply(color: colorScheme.onPrimary);- if (client.isMyNick(sender)) {+ var textStyle = DefaultTextStyle.of(context).style;+ if (!isAction) {+ textStyle = textStyle.apply(color: colorScheme.onPrimary);+ }++ // Actions are displayed as if they were told by an external narrator.+ // To preserve this effect, always show actions on the left side.+ if (client.isMyNick(sender) && !isAction) { boxColor = Colors.grey[200]!;
boxAlignment = Alignment.centerRight;
textStyle = DefaultTextStyle.of(context).style.apply(color: boxColor.computeLuminance() > 0.5 ? Colors.black : Colors.white);
@@ -560,17 +569,23 @@ class _MessageItem extends StatelessWidget {
var linkStyle = textStyle.apply(decoration: TextDecoration.underline);
List<InlineSpan> content;
- if (ctcp != null && ctcp.cmd == 'ACTION') {- textStyle = textStyle.apply(fontStyle: FontStyle.italic);-- String actionText;- if (ctcp.cmd == 'ACTION') {- actionText = stripAnsiFormatting(ctcp.param ?? '');- } else {- actionText = 'has sent a CTCP "${ctcp.cmd}" command';- }+ if (isAction) {+ // isAction can only ever be true if we have a ctcp+ assert(ctcp != null);+ var actionText = stripAnsiFormatting(ctcp?.param ?? ''); content = [
+ WidgetSpan(+ child: Container(+ width: 8.0,+ height: 8.0,+ margin: EdgeInsets.all(3.0),+ decoration: BoxDecoration(+ shape: BoxShape.circle,+ color: boxColor,+ ),+ ),+ ), senderTextSpan,
TextSpan(text: ' '),
linkify(actionText, textStyle: textStyle, linkStyle: linkStyle),
@@ -612,20 +627,31 @@ class _MessageItem extends StatelessWidget {
]);
}
- Widget bubble = Align(- alignment: boxAlignment,- child: Container(- decoration: BoxDecoration(- borderRadius: BorderRadius.circular(10),- color: boxColor,+ Widget decoratedMessage;+ if (isAction) {+ decoratedMessage = Align(+ alignment: boxAlignment,+ child: Container(+ child: inner, ),
- padding: EdgeInsets.all(10),- child: inner,- ),- );+ );+ } else {+ decoratedMessage = Align(+ alignment: boxAlignment,+ child: Container(+ decoration: BoxDecoration(+ borderRadius: BorderRadius.circular(10),+ color: boxColor,+ ),+ padding: EdgeInsets.all(10),+ child: inner,+ ),+ );+ }+ if (!client.isMyNick(sender)) {
- bubble = SwipeAction(- child: bubble,+ decoratedMessage = SwipeAction(+ child: decoratedMessage, background: Align(
alignment: Alignment.centerLeft,
child: Opacity(
@@ -654,7 +680,7 @@ class _MessageItem extends StatelessWidget {
),
Container(
margin: EdgeInsets.only(left: margin, right: margin, top: marginTop, bottom: marginBottom),
- child: bubble,+ child: decoratedMessage, ),
]);
}
--
2.35.1
On Sunday, April 3rd, 2022 at 15:54, Noah Loomans <noah@noahloomans.com> wrote:
> When using an action in irc (also known as /me), you are writing in a> way that it feels like an external narrator is dictating what the user> is doing, instead of the user saying that themselves. Rendering this> text inside of a chat bubble breaks this model, so change the looks to> render it as an annoucement instead.
Very nice! A few nitpick comments below, but overall this looks good to me.
> This is my first time writing in flutter, so please be critical with the> feedback! I'm particially uncertain about the following:>> > if (isAction) {> > // isAction can only ever be true if we have a ctcp> > assert(ctcp != null);> > var actionText = stripAnsiFormatting(ctcp?.param ?? '');>> If isAction is true, then ctcp must exist, as that is a pre-condition> for isAction being true. Dart doens't seem to be concined of this,> however, and instead requires a null-check on ctcp. The assert doesn't> seem to have made ctcp happy either. Is there a better way to handle> this?
Unfortunately, not that I know of. I'd just use ! instead of ? since crashing
is fine (and a good thing) if the programmer's expectations are not correct.
> > WidgetSpan(> > child: Container(> > width: 8.0,> > height: 8.0,> > margin: EdgeInsets.all(3.0),> > decoration: BoxDecoration(> > shape: BoxShape.circle,> > color: boxColor,> > ),> > ),> > ),>> Is there a better way of drawing a circle? The size and margin are also> something I came up with experimentation, I am not sure if this would> still look as good if the user has a different font-size.
I don't know of a better way!
> Feedback welcome!>> lib/page/buffer.dart | 76 +++++++++++++++++++++++++++++---------------> 1 file changed, 51 insertions(+), 25 deletions(-)>> diff --git a/lib/page/buffer.dart b/lib/page/buffer.dart> index f1daf70..c5fac7f 100644> --- a/lib/page/buffer.dart> +++ b/lib/page/buffer.dart> @@ -516,14 +516,17 @@ class _MessageItem extends StatelessWidget {> assert(ircMsg.cmd == 'PRIVMSG' || ircMsg.cmd == 'NOTICE');>> var prevIrcMsg = prevMsg?.msg;> + var prevCtcp = prevIrcMsg != null ? CtcpMessage.parse(prevIrcMsg) : null;> var prevEntry = prevMsg?.entry;> var prevMsgSameSender = prevIrcMsg != null && ircMsg.source!.name == prevIrcMsg.source!.name;> + var prevMsgIsAction = prevCtcp != null && prevCtcp.cmd == 'ACTION';>> var nextMsgSameSender = nextMsg != null && ircMsg.source!.name == nextMsg!.msg.source!.name;>> + var isAction = ctcp != null && ctcp.cmd == 'ACTION';> var showUnreadMarker = prevEntry != null && unreadMarkerTime != null && unreadMarkerTime!.compareTo(entry.time) < 0 && unreadMarkerTime!.compareTo(prevEntry.time) >= 0;> var showDateMarker = prevEntry == null || !_isSameDate(localDateTime, prevEntry.dateTime.toLocal());> - var showSender = showUnreadMarker || !prevMsgSameSender;> + var showSender = showUnreadMarker || !prevMsgSameSender || (prevMsgIsAction != isAction);
Hm I think we'll want to always display the sender for all ACTIONs? Otherwise
it'll look like this:
● emersion does the thing
● does the thing again
or was that intended?
> var showTime = !nextMsgSameSender || nextMsg!.entry.dateTime.difference(entry.dateTime) > Duration(minutes: 2);>> var unreadMarkerColor = Theme.of(context).accentColor;> @@ -535,8 +538,14 @@ class _MessageItem extends StatelessWidget {> //var boxColor = Theme.of(context).accentColor;> var boxColor = colorScheme.primary;> var boxAlignment = Alignment.centerLeft;> - var textStyle = DefaultTextStyle.of(context).style.apply(color: colorScheme.onPrimary);> - if (client.isMyNick(sender)) {> + var textStyle = DefaultTextStyle.of(context).style;> + if (!isAction) {> + textStyle = textStyle.apply(color: colorScheme.onPrimary);> + }> +> + // Actions are displayed as if they were told by an external narrator.> + // To preserve this effect, always show actions on the left side.
Ah, yes, I guess a right-aligned circle+text would look a bit weird. It's a bit
confusing to left-align always, but the color differences help.
> + if (client.isMyNick(sender) && !isAction) {> boxColor = Colors.grey[200]!;> boxAlignment = Alignment.centerRight;> textStyle = DefaultTextStyle.of(context).style.apply(color: boxColor.computeLuminance() > 0.5 ? Colors.black : Colors.white);
On Wednesday 6 April 2022 at 16:40 CEST, Simon Ser wrote:
> On Sunday, April 3rd, 2022 at 15:54, Noah Loomans <noah@noahloomans.com> wrote:>> > When using an action in irc (also known as /me), you are writing in a> > way that it feels like an external narrator is dictating what the user> > is doing, instead of the user saying that themselves. Rendering this> > text inside of a chat bubble breaks this model, so change the looks to> > render it as an annoucement instead.>> Very nice! A few nitpick comments below, but overall this looks good to me.
Good to hear!
> > var prevIrcMsg = prevMsg?.msg;> > + var prevCtcp = prevIrcMsg != null ? CtcpMessage.parse(prevIrcMsg) : null;> > var prevEntry = prevMsg?.entry;> > var prevMsgSameSender = prevIrcMsg != null && ircMsg.source!.name == prevIrcMsg.source!.name;> > + var prevMsgIsAction = prevCtcp != null && prevCtcp.cmd == 'ACTION';> >> > var nextMsgSameSender = nextMsg != null && ircMsg.source!.name == nextMsg!.msg.source!.name;> >> > + var isAction = ctcp != null && ctcp.cmd == 'ACTION';> > var showUnreadMarker = prevEntry != null && unreadMarkerTime != null && unreadMarkerTime!.compareTo(entry.time) < 0 && unreadMarkerTime!.compareTo(prevEntry.time) >= 0;> > var showDateMarker = prevEntry == null || !_isSameDate(localDateTime, prevEntry.dateTime.toLocal());> > - var showSender = showUnreadMarker || !prevMsgSameSender;> > + var showSender = showUnreadMarker || !prevMsgSameSender || (prevMsgIsAction != isAction);>> Hm I think we'll want to always display the sender for all ACTIONs? Otherwise> it'll look like this:>> ● emersion does the thing> ● does the thing again>> or was that intended?
That is actually not what happens, the code that fills in the content
variable for the isAction branch actually never looks at the isSender
variable, nor did it before this patch. I can see it being confusing
though, especially since showSender used for padding as well. Maybe
isFirstOfGroup would be a better name?
> > + // Actions are displayed as if they were told by an external narrator.> > + // To preserve this effect, always show actions on the left side.>> Ah, yes, I guess a right-aligned circle+text would look a bit weird. It's a bit> confusing to left-align always, but the color differences help.
What colour differences are you takling about?
On Wednesday, April 6th, 2022 at 19:33, Noah Loomans <noah@noahloomans.com> wrote:
> > > var prevIrcMsg = prevMsg?.msg;> > > + var prevCtcp = prevIrcMsg != null ? CtcpMessage.parse(prevIrcMsg) : null;> > > var prevEntry = prevMsg?.entry;> > > var prevMsgSameSender = prevIrcMsg != null && ircMsg.source!.name == prevIrcMsg.source!.name;> > > + var prevMsgIsAction = prevCtcp != null && prevCtcp.cmd == 'ACTION';> > >> > > var nextMsgSameSender = nextMsg != null && ircMsg.source!.name == nextMsg!.msg.source!.name;> > >> > > + var isAction = ctcp != null && ctcp.cmd == 'ACTION';> > > var showUnreadMarker = prevEntry != null && unreadMarkerTime != null && unreadMarkerTime!.compareTo(entry.time) < 0 && unreadMarkerTime!.compareTo(prevEntry.time) >= 0;> > > var showDateMarker = prevEntry == null || !_isSameDate(localDateTime, prevEntry.dateTime.toLocal());> > > - var showSender = showUnreadMarker || !prevMsgSameSender;> > > + var showSender = showUnreadMarker || !prevMsgSameSender || (prevMsgIsAction != isAction);> >> > Hm I think we'll want to always display the sender for all ACTIONs? Otherwise> > it'll look like this:> >> > ● emersion does the thing> > ● does the thing again> >> > or was that intended?>> That is actually not what happens, the code that fills in the content> variable for the isAction branch actually never looks at the isSender> variable, nor did it before this patch. I can see it being confusing> though, especially since showSender used for padding as well. Maybe> isFirstOfGroup would be a better name?
Ah yeah indeed, +1 for the new name!
> > > + // Actions are displayed as if they were told by an external narrator.> > > + // To preserve this effect, always show actions on the left side.> >> > Ah, yes, I guess a right-aligned circle+text would look a bit weird. It's a bit> > confusing to left-align always, but the color differences help.>> What colour differences are you takling about?
ACTIONs sent by the current user will always use the gray color. ACTIONs sent
by other users will use a different one.
On Wednesday 6 April 2022 at 19:42 CEST, Simon Ser wrote:
> On Wednesday, April 6th, 2022 at 19:33, Noah Loomans <noah@noahloomans.com> wrote:>> > > > var prevIrcMsg = prevMsg?.msg;> > > > + var prevCtcp = prevIrcMsg != null ? CtcpMessage.parse(prevIrcMsg) : null;> > > > var prevEntry = prevMsg?.entry;> > > > var prevMsgSameSender = prevIrcMsg != null && ircMsg.source!.name == prevIrcMsg.source!.name;> > > > + var prevMsgIsAction = prevCtcp != null && prevCtcp.cmd == 'ACTION';> > > >> > > > var nextMsgSameSender = nextMsg != null && ircMsg.source!.name == nextMsg!.msg.source!.name;> > > >> > > > + var isAction = ctcp != null && ctcp.cmd == 'ACTION';> > > > var showUnreadMarker = prevEntry != null && unreadMarkerTime != null && unreadMarkerTime!.compareTo(entry.time) < 0 && unreadMarkerTime!.compareTo(prevEntry.time) >= 0;> > > > var showDateMarker = prevEntry == null || !_isSameDate(localDateTime, prevEntry.dateTime.toLocal());> > > > - var showSender = showUnreadMarker || !prevMsgSameSender;> > > > + var showSender = showUnreadMarker || !prevMsgSameSender || (prevMsgIsAction != isAction);> > >> > > Hm I think we'll want to always display the sender for all ACTIONs? Otherwise> > > it'll look like this:> > >> > > ● emersion does the thing> > > ● does the thing again> > >> > > or was that intended?> >> > That is actually not what happens, the code that fills in the content> > variable for the isAction branch actually never looks at the isSender> > variable, nor did it before this patch. I can see it being confusing> > though, especially since showSender used for padding as well. Maybe> > isFirstOfGroup would be a better name?>> Ah yeah indeed, +1 for the new name!
Cool I'll update the name for v2.
> > > > + // Actions are displayed as if they were told by an external narrator.> > > > + // To preserve this effect, always show actions on the left side.> > >> > > Ah, yes, I guess a right-aligned circle+text would look a bit weird. It's a bit> > > confusing to left-align always, but the color differences help.> >> > What colour differences are you takling about?>> ACTIONs sent by the current user will always use the gray color. ACTIONs sent> by other users will use a different one.
Actually that's not what this patch does, the colour will be as if it
was send by a different user. I would agree that having it grey makes
more sense though, I'll update that for v2.