I tried out this patch and there are a lot of design questions that it leaves open. We should make sure that the chat notification looks good in all cases before we commit the changes for the new look. I'll attach the screenshots to illustrated some cases I'm talking about.

1) How should the chat notification show up in the banner mode? The bigger icon is only partially visible due to the current height of the banner. The text is cut off too because it is centered vertically with respect to the icon.

2) How should the timestamps and presence changes be styles? Right now the timestamps show up on the right and the presence changes show up on the left. The timestamps showing up on the right for all messages conflicts with the messages from me that show up on the right. The current styling of timestamps and presence changes is too similar to the new styling of chat messages.

3) How should messages from me that are right-aligned wrap? As can be seen in one of the screenshots, the wrapping looks off now.

4) I'm not sure right-aligning some messages works out that well in general. The screenshot that is exactly like the mockup looks good, but short messages look odd. It's also odd to enter some short message in the box on the left and have it show up all the way to the right.

5) The notification sinks in a lot when you respond so that the text entry window becomes only partially visible. This is unrelated bug that I filed as bug 640484.

This bug possibly blocks on bug 636838. Jon wanted to to try to style the banner, expanded notification, and message tray after the changes he requested there are done.

Created attachment 179246 [details]
The icon and the text are only partially visible in the banner mode

Created attachment 179247 [details]
The same chat example as in the mockup looks good

Created attachment 179248 [details]
Different length messages don't looks as good; messages that are too short look scattered and right-aligned messages from me don't wrap nicely

Created attachment 179249 [details]
The timestamp getting added on the right isn't right if some of the messages get added on the right

Created attachment 179250 [details]
The presence changes and timestamps need different styling

*** Bug 647162 has been marked as a duplicate of this bug. ***

Might as well continue here :) The attachment #185519 [details] doesn't introduce the layout issues mentione above, but I would still like to get in touch with you on IRC to discuss the best possible solution.

Created attachment 185542 [details] [review]
improve styling of the chat window/bubble

This is the same patch without the introduced indentation as per fmuellner's request

Review of attachment 185542 [details] [review]:

::: data/theme/gnome-shell.css
@@ +1141,1 @@
+.chat-received:rtl { }

:rtl is an automatic pseudo class in RTL locales, no point in having it empty (I do see the point for leaving .chat-received though)

Created attachment 185543 [details]
layout changes

After talking to Marine on IRC and faced with my inability to grok the JS bits, I am attaching an image describing my intent and asking for a little assistence and provision of new additional classes to the dialog to aid styling.

1) In contrast to the original design I would prefer the avatar to remain the same size collapse and expanded, but in this case, remove the indention of everything but the avatar in the dialog. I will attach a mockup of my intent.

2) I would love to have a container around same direction messages to allow to space sent/received messages apart rather than spacing individual messages evenly. Also the fadeout of the messages is too tall, making it difficult to make out whether the first line is a response or not.

3) If the text entry spans the whole width, I would like the whole dialog to have more padding (applies to messages and avatar/name too). I take it the container already has a class... which is it?

Thank you for all the help.

Created attachment 185544 [details]
my immediate goal :)

(likely to change when I get a chance to iterate ;)

Created attachment 189279 [details] [review]
messageTray: Remove indent on conversation bubble

This makes the style look more what was intended in the mockups.



This removes the "left intend", jimmac. The code is a bit ugly though.

For the fade, you can use the new '-st-fade-offset' I added for you in
bug 651813.

I don't believe we have a better thing for you to tweak the padding than
"#notification", and I'll be working on the direction containers soon,
I promise.

Comment on attachment 189279 [details] [review]
messageTray: Remove indent on conversation bubble

if we're not lining up the content area with the banner, then there's no need to have a table. we can just have a vertical boxlayout whose first row contains a horizontal boxlayout

I initially tried this, but the hard part is making sure that the "rows" appear in the correct order. Depending on what type of message it was, the action area could appear above or below the body, and sometimes they were sandwiched in the middle.

I know of no way to "pin an actor" to a certain position, except for St.Table.

For things like presence and alias changes, timestamps, or actions (think /me), how do you want the direction containers to group things?

Comment on attachment 189279 [details] [review]
messageTray: Remove indent on conversation bubble

>+        this._titleArea.set_style('spacing: %dpx;'.format(this._spacing));

ok except for that; you should give the titlearea its own CSS, with the appropriate spacing

Created attachment 190883 [details] [review]
telepathyClient: Add group containers

All messages are now assigned one of three groups: 'received', 'sent',
or 'meta'. Group containers contain all messages of the same group.

Created attachment 191913 [details] [review]
messageTray: Remove indent on notifications

This makes the style look more what was intended in the mockups.



Fixed.

Created attachment 191919 [details] [review]
telepathyClient: Add group containers

All messages are now assigned one of three groups: 'received', 'sent',
or 'meta'. Group containers contain all messages of the same group.



Marking "Remove indent" obsolete because it broke stacking and other things. Feel free
to add a design that doesn't break everything else.

Should I separate this patch out, or is it good to go? I have three changes:

  * Add group containers.
  * Update text to remove the old string freeze hack.
  * Add styles to look like mockups.

Comment on attachment 185542 [details] [review]
improve styling of the chat window/bubble

Not going to take this because it breaks stacked notifications.

Created attachment 193020 [details] [review]
telepathyClient: Add our own translations for timestamps

As an effort to prevent a string freeze to land timestamps on 3.0, we reused
translations for the calendar. Now that the string freeze is long gone, make
some proper strings.



If the direction containers don't land, I'd like to get this in before string freeze.

Created attachment 193021 [details] [review]
telepathyClient: Add direction containers

Direction containers group all contiguous messages in the same direction into
their own parent container, allowing for smarter styling of similar messages.

Comment on attachment 193020 [details] [review]
telepathyClient: Add our own translations for timestamps

>+            /* Translators: this is a time format in the style of "Wednesday, May 25, 2012",
>+             shown when you get a chat message in the same year. */

different, not same

otherwise ok

Comment on attachment 193020 [details] [review]
telepathyClient: Add our own translations for timestamps

Attachment 193020 [details] pushed as aa1405e - telepathyClient: Add our own translations for timestamps


Pushed with s/the same/a different/

Created attachment 194024 [details] [review]
patch - theme updates for chat input widget (plus smaller meta messages)

Created attachment 194102 [details] [review]
telepathyClient: Add direction containers

Direction containers group all contiguous messages in the same direction into
their own parent container, allowing for smarter styling of similar messages.



Rebased onto master (fix conflict with aday's notification softening patches)

Comment on attachment 194102 [details] [review]
telepathyClient: Add direction containers

>+.chat-received {
>+    padding-left: 24px;
>     border-radius: 4px;
>+    color: #7E7E7E;

I am pretty sure the indentation and color scheme in your patch is the opposite of what Jakub intended. You want to highlight the received messages and dim (and indent) the sent ones.

You should also not use px for padding here, since it means there will be relatively less padding when using larger text zooms. Multiply by 0.75 and use pt.

>+            if (group.get_children().length == 0)
>+                this._contentArea.remove_actor(group);

group.destroy() is preferred, since it results in more garbage being collected right away.

>+    /**
>+     * append:

"_append"

>+        let timestamp = props.timestamp || currentTime;
>+        let styles = props.styles || [];

use Params.parse()

Created attachment 194501 [details] [review]
telepathyClient: Use sent timestamp instead of received timestamp

It's generally more useful to see when a person sent a message instead of when
we received it. Also, a recent change in Telepathy made the received timestamp
be 0 for messages we send.



--
<magcius> cassidy, do you know why  tpMessage.get_received_timestamp() returns 0?
<magcius> (other tp people can answer that as well)
...
<magcius> oh, this is a MessageSent
<magcius> will the received timestamp always be 0?
<wjt> probably for sent messages, yes
<wjt> generally speaking i'd recommend using the sent timestamp for both -- it's more useful to see when the other person sent the message than when we received it
<magcius> OK.
<wjt> this was recently changed in empathy too
<magcius> That's fair.
<wjt> it does: ts = get_send_timestamp(); if (ts == 0) ts = get_received_timestamp

Created attachment 194502 [details] [review]
telepathyClient: Use sent timestamp instead of received timestamp

It's generally more useful to see when a person sent a message instead of when
we received it. Also, a recent change in Telepathy made the received timestamp
be 0 for messages we send.



---

<magcius> cassidy, do you know why  tpMessage.get_received_timestamp() returns 0?
<magcius> (other tp people can answer that as well)
...
<magcius> oh, this is a MessageSent
<magcius> will the received timestamp always be 0?
<wjt> probably for sent messages, yes
<wjt> generally speaking i'd recommend using the sent timestamp for bothâit's more useful to see when the other person sent the message than when we received it
<magcius> OK.
<wjt> this was recently changed in empathy too
<magcius> That's fair.
<wjt> it does: ts = get_send_timestamp(); if (ts == 0) ts = get_received_timestamp

Created attachment 194507 [details] [review]
telepathyClient: Add direction containers

Direction containers group all contiguous messages in the same direction into
their own parent container, allowing for smarter styling of similar messages.



> You should also not use px for padding here, since it means there will be
> relatively less padding when using larger text zooms. Multiply by 0.75 and use
> pt.

The 4px padding was already there, but sure.

Comment on attachment 194507 [details] [review]
telepathyClient: Add direction containers

>+    border-radius: 3pt;

oh, i really only meant use pt for the 24px padding, not any of the rest. (Padding and border-radius in the rest of the shell is done in px, so using pt for the normal padding or border-radius here would be inconsistent. But the rule with 24px padding isn't really padding, it's an indent, and we want that to be proportional.)

>diff --git a/js/ui/notificationDaemon.js b/js/ui/notificationDaemon.js

this part was not in the previous patch... is this from some other bug?

Comment on attachment 194502 [details] [review]
telepathyClient: Use sent timestamp instead of received timestamp

Attachment 194502 [details] pushed as 712ea9b - telepathyClient: Use sent timestamp instead of received timestamp

Created attachment 194635 [details] [review]
telepathyClient: Add direction containers

Direction containers group all contiguous messages in the same direction into
their own parent container, allowing for smarter styling of similar messages.



Is this better?

Comment on attachment 194635 [details] [review]
telepathyClient: Add direction containers

Attachment 194635 [details] pushed as 06d906b - telepathyClient: Add direction containers

This should mostly be fixed. Do the CSS changes look right to you, jimmac?

I see the last three patches have been committed. Is anything left here ?
Btw, jimmac is on vacation this week, afaik.

No.