(In reply to comment #0)
> There are a few things we can do to improve the expanded chat view.
> 
> View:
>  * Group all uninterrupted lines from the same person together if sent within a
> few minutes

I'm not sure this is necessary / positive. I use the Classic empathy theme, which repeates the name for every message, and among all themes offered it is the one I like most, for this particular reason (you save one line of space for each run of consecutive messages - very useful if you intermix or send isolated long messages)

>  * When a new message comes in after a period of more than a few minutes we
> should show a timestamp of some kind to show the previous message is old

Patch coming

>  * show log of older messages from same person
>  * if scrollback goes too far have an action to view full conversation log


You can either:
1) rebuild the logger UI inside the Shell (use TplLogger for data)
2) take away the logger UI from Empathy, as a separate process / command
3) have Empathy show the logger UI responding to a DBus method

If you consider 1 unfeasible (even though it would be appropriate with some earlier designs of "people as first class UI objects" and "contact management directly in the Shell"), this is blocked on Empathy

>  * support links
>  * Support smileys :)

Unless you want to add some existing HTML rendering engine to the Shell (which is not impossible, but would break the interface), adding images within text is going to be impossible (you would need to override ClutterText / StLabel text rendering and working manually with Pango - and I'm not even sure that is enough). Links are no better, as you need to track the position of a particular span of text.

(You could hack something by making every word a ClutterText and then putting everything inside a ClutterFlowLayout. I'm not sure how good this solution would be).

Created attachment 166506 [details] [review]
Show timestamp in expanded chat

When the last message is older than SCROLLBACK_IMMEDIATE_TIME (5
minutes), show a timestamp in the middle, indicating current time
(and date, if messages is from a different day than today)

Created attachment 166596 [details] [review]
Show timestamp in expanded chat view

Now tested, and some bugs fixed.

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

Review of attachment 166596 [details] [review]:

Thank you for the patch!

The way it implements timestamps is not quite how we want it to work. We'd like it to add a timestamp after SCROLLBACK_IMMEDIATE_TIME regardless of whether a new message was received. The timestamp should be of when the last message was received, not of the current time. The rationale is that when you see a new message, you know when it was received, but if you look at the scrollback with either the messages you missed or with the conversation you had some time ago, you need to know when that happened. This is what Gmail in-browser chat does. It adds timestamps that say "Sent at 4:04 PM on Thursday" after a minute (so when it is already 4:05 PM). We don't need a timestamp in the beginning of the chat either.

As for the format of the message, Jon might have more feedback about it, but we can sort it out once we have the rest of functionality ready. I don't think we want "--" before and after the time and since we have a lot of space, I think we can just always say "Sent Thursday, August 12 4:04 PM" or "Sent Thursday, August 12 16:04" for the locales where AM and PM are not used. "Sent" is used to give an indication that timestamp that is being appended is not the current time, but rather the time when the last message was sent. Again, this is up to Jon and we can decide on that later.

As for the code, I think we need to use this._timestampHistory in addition to this._history because otherwise it's kind of arbitrary how many actual messages we leave in the scrollback when we splice out older messages. We'll then need to splice out the timestamps separately. We can use the time of the last message we remove to figure out which timestamps need to be removed.

Wouldn't that conflict with the behaviour of Empathy, where timestamps are highlighted with - (even though there is much more space), are automatically added at the the beginning of the conversation and updated with the current timestamp when the chat is idle?

Do you think Empathy should be changed as well?

Created attachment 167873 [details]
Empathy chat with timestamps

So it looks like Empathy prints a full timestamp in the beginning of the conversation and then prints the time of the first message in the sequence of messages from each person. I haven't seen it update an idle chat later with another timestamp.

The layout we have is somewhat different since we don't print the names of people along with the conversation. Also, because the primary use case for the chat notification is immediate use, adding the initial timestamp by default would clutter the notification with unneeded information.

Jon, could you please look into this and clarify how you would like the timestamps to look, as well as whether the layout should be coordinated with the layout of the Empathy chat window.

Sorry, I should have specified I mean the Classic theme, which doesn't group message by person.
The layout, is completely different, more like

      - 07.00 -
MyName: my message
MyName: with continuation
Other: his message
      - 07.10 -
MyName: sorry, didn't hear you!

(which mirrors the expanded chat notification layout, if you ignore contact names, indicated by different gradients)

Created attachment 170032 [details] [review]
Patch corrected with Marina's suggestions

Some work on ensuring a different selection color is in bug 615731.

Supporting links is filed as a separate bug 610219.

Hmm, trying this out I get:
    JS ERROR: !!!   Exception was: SyntaxError: missing } after property list
    JS ERROR: !!!     lineNumber = '655'
    JS ERROR: !!!     fileName = '/home/jmccann/gnome-shell/source/gnome-shell/js/ui/telepathyClient.js'
    JS ERROR: !!!     stack = '@/home/jmccann/gnome-shell/source/gnome-shell/js/ui/main.js:34

Created attachment 171517 [details] [review]
Show timestamps in expanded chat

Weird, I would swear I tested it...

Review of attachment 171517 [details] [review]:

The patch looks good overall. The filtering thing for this._history is particularly elegant :).

There was a number of simple errors in this patch, which need to be fixed for it to work.

I think the functionality is right, but Jon should test it once you upload the new patch. Questions for Jon:
1) What should the timeout be?
2) What should the style of the timestamp be? Should it be centered? I definitely think it should be de-emphasized further.
3) The current behavior is that the timestamp is added after a timeout from the last message from either user. The timeout is being reset on each new message, so if the sender sends 10 messages spaced with the interval just under timeout, there will be no timestamps until after the last message. I think that's fine. That's what happens in Google chat too. Is this the timestamp behavior you'd like?

Giovanni, the timestamps are now being added after the presence change (e.g. online, offline, away, busy) messages too. We'll need to change them to use 'realMessage: false' flag and append them in a similar way we append timestamps and with the same style. We can do that as a follow-up patch.

::: data/theme/gnome-shell.css
@@ +933,3 @@
+    border-radius: 4px;
+
+    font-style: italic;

I don't think there should be a blank line here.

::: js/ui/telepathyClient.js
@@ +18,2 @@
 // See Notification.appendMessage
+const SCROLLBACK_IMMEDIATE_TIME = 5 * 60; // 5 minutes

I think the timeout should be more like 1 minute. It's up to Jon to define it.

@@ +612,3 @@
+        // Schedule a new timestamp in SCROLLBACK_IMMEDIATE_TIME
+        this._timestampTimeoutId = Mainloop.timeout_add_seconds (SCROLLBACK_IMMEDIATE_TIME,
+                Lang.bind(this, this._appendTimestamp));

You need to add Mainloop import. There should be no space after timeout_add_seconds. The spacing of the second line is a bit odd.

@@ +630,2 @@
                 SCROLLBACK_IDLE_LENGTH : SCROLLBACK_RECENT_LENGTH;
+            let filteredHistory = this._history.filter (function (item) { return item.realMessage });

No space after filter and after function.

@@ +640,3 @@
 
+    _appendTimestamp: function() {
+        let lastMessageTime = this._history[0].timestamp;

It's 'time', rather than 'timestamp'.

@@ +645,3 @@
+        /* Translators: this is the same string as used in the clock
+        for 24-hour time and date, with hyphens */
+        let timelabel = this.addBody(now.toLocaleFormat(_("Sent %a %b %e, %R")));

It's 'lastMessageDate', rather than 'now'. The variable should probably be named timeLabel.

Created attachment 171637 [details] [review]
Show timestamps in expanded chat

Fixed the various comments and also included the fix for the presence message, which is now a meta-message like timestamps.

Review of attachment 171637 [details] [review]:

Looks good overall!

I discussed the current look with Jon and he said he'll play around more and will comment on what the meta info message style should be. He was going to try smaller font and right-alignment of the meta info messages.

He also said that we should use friendly time for the timestamp, as implemented here
http://git.gnome.org/browse/rhythmbox/tree/lib/rb-cut-and-paste-code.c rb_utf_friendly_time()
and here
http://git.gnome.org/browse/evolution/tree/mail/message-list.c filter_date()

What's implemented there is:
"Today 12:34 am"
"Yesterday 12:34 am"
"Wed 12:34 am"
"Feb 12 12:34 am"
"Feb 12 1997"

Jon, could you select the corresponding messages that we should display? Should it be:
"Sent today at 12:34 AM" or ""Sent at 12:34 AM" or "Sent at 12:34 AM today"
"Sent yesterday at 12:34 AM" or "Sent at 12:34 AM yesterday"
"Sent on Wednesday at 12:34 AM" or "Sent at 12:34 AM on Wednesday" (this is what Google talk uses)
"Sent on February 12 at 12:34 AM" or "Sent at 12:34 AM on February 12"
"Sent on February 12 1997" or "Sent on February 12, 1997"

The use of 12-hour vs. 24-hour should be locale dependant of course. Maybe we can base it on the CLOCK_FORMAT_KEY that is used for the clock in the panel.

I think we can commit this patch with just the style changes, and have it just say "Sent at 12:34 AM on Wednesday" and use the CLOCK_FORMAT_KEY to determine 12-hour vs. 24-hour.

We can then make another patch that will implement friendly time and an ability to update the timestamps.

::: js/ui/telepathyClient.js
@@ +19,2 @@
 // See Notification.appendMessage
+const SCROLLBACK_IMMEDIATE_TIME = 5 * 60; // 5 minutes

The timeout should be 1 minute.

@@ +592,3 @@
     },
 
     appendMessage: function(text, asTitle) {

appendMessage() no longer needs to take 'asTitle'.

@@ +645,3 @@
+
+        /* Translators: this is the same string as used in the clock
+        for 24-hour time and date, with hyphens */

We are not using hyphens, are we?

@@ +655,3 @@
+
+    appendPresence: function(text) {
+        this.update(text, null, { customContent: true });

Jon said we don't want to change the notification title to reflect presence. So we don't need this line.

Created attachment 171844 [details] [review]
Show timestamp in expanded chat

Fixed (I hope for the last time, before we add nice times).
It does not use the clock preference for AM/PM (as it is generally different what you want for logs and for clocks), instead uses %X, which is "time in a format appropriate for current locale".

Review of attachment 171844 [details] [review]:

The commit message needs to be updated. Something like:

"When the last message is older than SCROLLBACK_IMMEDIATE_TIME (1 minute), show a timestamp on the right, indicating the time it was sent.

Use the same style for presence changes, but show them on the left."

Sorry that wasn't the final round of changes :). Here are some more we need to take care of. I ended up experimenting some with the code while discussing the style with Jon, so I'm attaching the patch with my changes. Feel free to review it (maybe there are better ways to do certain things, like right-aligning, I was just throwing the code quickly together) and merge it with your patch. Here is what it changes:
1) The timestamps are gray, have smaller font, non-italic, and right-aligned.
2) The presence changes are gray, have smaller font, non-italic, and left-aligned.
3) We use the actual timestamp of the received messages, so that the pending messages that were received earlier have the right timestamp that also gets printed immediately.
4) We update the title of the notification if we are notifying on presence change, which happens when the presence changed to online or offline. We need that to make the banner notification look right (I was wrong before when I said we didn't need it). We update the title to just the person's name on any presence change we don't notify on or on the new message from that person.

One other change that we should ideally make before we commit is use a different timestamp format. %X results in the seconds being printed, which we don't want. I don't see a problem with using '12-hour' vs. '24-hour' flag used for the panel clock. It doesn't mean that the timestamp will have the same format as the clock. It just means that we'll use the format for 12 vs. 24 hours that the user likes, which is right for the timestamps.

In addition to implementing the friendly time updates, the next improvement for the timestamps would be to merge all the messages that were sent before a timestamp was added in a single bubble and add the right-aligned timestamp on a line of its own inside that bubble. That's what Jon suggested.

Btw, I don't think I've seen you on the GNOME Shell IRC channel. It's #gnome-shell on irc.gnome.org . It's a good way to discuss some things and many of the GNOME Shell developers are usually present there, so feel free to join at the times when you don't mind being distracted :).

Created attachment 171929 [details] [review]
Updates to how timestamps are displayed

This patch is meant to be merged with the patch that implements timestamps in
chat and makes the following changes:
1) The timestamps are gray, have smaller font, non-italic, and right-aligned.
2) The presence changes are gray, have smaller font, non-italic, and
left-aligned.
3) We use the actual timestamp of the received messages, so that the pending
messages that were received earlier have the right timestamp that also gets
printed immediately.
4) We update the title of the notification if we are notifying on presence
change, which happens when the presence changed to online or offline. We need
that to make the banner notification look right. We update the title to just
the person's name on any presence change we don't notify on or on the new
message from that person.

(In reply to comment #18)
> One other change that we should ideally make before we commit is use a
> different timestamp format. %X results in the seconds being printed, which we
> don't want. I don't see a problem with using '12-hour' vs. '24-hour' flag used
> for the panel clock. It doesn't mean that the timestamp will have the same
> format as the clock. It just means that we'll use the format for 12 vs. 24
> hours that the user likes, which is right for the timestamps.

The problem is: what if the clock format is 'unix'? Do we show the unix timestamp? Or what if the format is custom and already includes the date?
> 
> In addition to implementing the friendly time updates, the next improvement for
> the timestamps would be to merge all the messages that were sent before a
> timestamp was added in a single bubble and add the right-aligned timestamp on a
> line of its own inside that bubble. That's what Jon suggested.

I'm not sure I understand this. Do you want to collapse consecutive messages? This has some problems, detailed in comment #1
Or do you want to collapse only old messages? Or what?

Created attachment 174022 [details] [review]
Show timestamp in expanded chat

When the last message is older than SCROLLBACK_IMMEDIATE_TIME (5
minutes), show a timestamp in the middle, indicating the time it
was sent.

Created attachment 175216 [details] [review]
Show timestamp in expanded chat

Show timestamp in expanded chat

When the last message is older than SCROLLBACK_IMMEDIATE_TIME (5
minutes), show a timestamp in the middle, indicating the time it
was sent.

Review of attachment 175216 [details] [review]:

This patch is just your patch rebased to the latest change on master which adds 'markup' argument to addBody() in messageTray.js .

This is ok to commit with the minor changes noted below (commit message and unnecessary {}). However, it'd be nice to get rid of the seconds in the timestamp as well.

The commit message for this patch should be changed to:
"Show timestamps in expanded chat

When the last message is older than SCROLLBACK_IMMEDIATE_TIME (1 minute), show
a timestamp on the right, indicating the time it was sent.

Use the same style for presence changes, but show them on the left."

::: js/ui/telepathyClient.js
@@ +608,3 @@
+        if (this._timestampTimeoutId) {
+            Mainloop.source_remove(this._timestampTimeoutId);
+        }

No {}

@@ +649,3 @@
+
+        /* Translators: this is a time format string followed by a date */
+        let timeLabel = this.addBody(lastMessageDate.toLocaleFormat(_("Sent at %X on %A")), false, { expand: true, x_fill: false, x_align: St.Align.END });

Can we parse out the seconds from %X if they are there?

(In reply to comment #23)
> Review of attachment 175216 [details] [review]:
> @@ +649,3 @@
> +
> +        /* Translators: this is a time format string followed by a date */
> +        let timeLabel = this.addBody(lastMessageDate.toLocaleFormat(_("Sent at
> %X on %A")), false, { expand: true, x_fill: false, x_align: St.Align.END });
> 
> Can we parse out the seconds from %X if they are there?

Probably not, but actually that is used only as a fallback, translators are expected to use their native way of representing times.
(We use %X instead of %H:%M so that is meets user expectations even when the message is in English)

Pushed with suggested changes (including a comment for translators).
Not closing as this bug includes other chat improvements.

>  * Group all uninterrupted lines from the same person together if sent within a
> few minutes

Discussed in #640271, I'm working on a patch.

>  * When a new message comes in after a period of more than a few minutes we
> should show a timestamp of some kind to show the previous message is old
>  * show log of older messages from same person
>  * if scrollback goes too far have an action to view full conversation log
>  * support links

These are done.

>  * Support smileys :)

Bug #651875

> Entry:
>  * support copy and paste

Done.

>  * highlight/select is the same as text color

Bug #643768

> Style:
>  * round off corners of grouping rectangle (the gradient thing)

Not sure what you mean here, but I assume it's done.

Marking INVALID, feel free to reverse.