955312 - Date information should be available for messages

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, enhancement)

Description

[Instantbot]

*** Original post on bio 1879 by mali <dev AT maliayas.com> at 2013-02-19 17:14:00 UTC ***

Sometimes, conversation tabs may be kept open for several days. In this case, the current GUI shows only the time information of the message, but not date.

Since date is important for such cases, it should be displayed along with time in the popup. Both for messages and status messages.

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 1879 at 2013-02-19 17:37:17 UTC ***

Marking as new.

Comment 2

[Instantbot]

Attachment: Screen capture

*** Original post on bio 1879 as attmnt 2311 by starman617 AT gmail.com at 2013-04-01 17:38:00 UTC ***

I like the fact that hovering the mouse brings up the time. I'd like to see the date here, too. I think the original submitter of this enhancement request is talking about the same thing so I'm adding my vote. :D

Comment 3

[aleth [:aleth]]

Attachment: datetooltips.diff

Currently we have only the time in message tooltips, this adds the date as well. I didn't add it to system messages as we currently don't have time tooltips for these either.

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8531636 [details] [diff] [review]
datetooltips.diff

Review of attachment 8531636 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall!

FYI I used https://trac.adium.im/wiki/MessageStyleKeywords to see if something like this exists already.

::: chat/modules/imThemes.jsm
@@ +356,5 @@
>      return date.toLocaleTimeString();
>    },
>    timestamp: function(aMsg) aMsg.time,
>    shortTime: function(aMsg) (new Date(aMsg.time * 1000)).toLocaleTimeString(),
> +  date: function(aMsg) {

Let's call this datetime.

Comment 5

[aleth [:aleth]]

Attachment: datetooltips.diff v2

I also checked our own wiki page for these substitutions.

Comment 6

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/3ae40fc65693

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8531772 [details] [diff] [review]
datetooltips.diff v2

Review of attachment 8531772 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/imThemes.jsm
@@ +352,3 @@
>    time: function(aMsg, aFormat) {
>      let date = new Date(aMsg.time * 1000);
>      if (aFormat)

Assuming toLocaleFormat didn't provide what you need, why haven't you just added a specific format here that would have a different handling?

@@ +362,5 @@
> +    let sdf = ScriptableDateFormat;
> +    return sdf.FormatDateTime("", sdf.dateFormatShort,
> +                              sdf.timeFormatNoSeconds, date.getFullYear(),
> +                              date.getMonth() + 1, date.getDate(),
> +                              date.getHours(), date.getMinutes(), 0);

Why the "0" here? I think we previously advertised these tooltips as the way to know at which second a message arrived in bubbles, because by default we only display hours:minutes in bubbles.

Comment 8

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Comment on attachment 8531772 [details] [diff] [review]
> datetooltips.diff v2
> 
> Review of attachment 8531772 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/modules/imThemes.jsm
> @@ +352,3 @@
> >    time: function(aMsg, aFormat) {
> >      let date = new Date(aMsg.time * 1000);
> >      if (aFormat)
> 
> Assuming toLocaleFormat didn't provide what you need, why haven't you just
> added a specific format here that would have a different handling?

It's hard to get localization right for dates across OS. This is what we ended up using in the log viewer where we had a few followups until it was correct iirc, so I used it for consistency. I didn't use a custom format string so as not to break compatibility with adium. Their format strings don't produce properly localized output as far as I could tell, neither does toLocaleFormat ("ordering of the day and month and other localization tasks are not handled automatically since you have control over the order in which they occur. You should take care that the format string is localized properly according to the user's system settings.").

> > +                              date.getHours(), date.getMinutes(), 0);
> 
> Why the "0" here? I think we previously advertised these tooltips as the way
> to know at which second a message arrived in bubbles, because by default we
> only display hours:minutes in bubbles.

That's easy to add back in a followup.

Comment 9

[aleth [:aleth]]

Attachment: addseconds.diff

Followup to restore seconds. We can always back the whole thing out again if the tooltips turn out to be too "heavy" with the date.

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8532048 [details] [diff] [review]
addseconds.diff

Review of attachment 8532048 [details] [diff] [review]:
-----------------------------------------------------------------

OK, let's add this back.

Comment 11

[aleth [:aleth]]

Attachment: todayisspecial.diff

Turns out implementing your idea of not showing the date for today's messages was easy given the existing tooltip code. So here's another followup that does that.

This also means that tooltips would be added for all message styles, not just for those that explicitly support them.

It doesn't obsolete the previous patches as we need those for the log viewer.

Comment 12

[aleth [:aleth]]

Attachment: todayisspecial.diff v2

Uses a shorter way of checking whether a Date is today.

Comment 13

[aleth [:aleth]]

Attachment: todayisspecial.diff v3

Bah, it's one of those files with odd indentation :-/

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8532302 [details] [diff] [review]
todayisspecial.diff v3

Review of attachment 8532302 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/content/imtooltip.xml
@@ +458,5 @@
>  
> +         // Are we over a message?
> +         let node = elt;
> +         do {
> +           if ("_originalMsg" in node) {

Any reason for not doing:
for (let node = elt; node; node = node.parentNode) {
  if (!node._originalMsg)
     continue;

@@ +463,5 @@
> +             // It's a message, so add a date/time tooltip.
> +             let date = new Date(node._originalMsg.time * 1000);
> +             let text;
> +             if ((new Date()).toDateString() == date.toDateString())
> +               text = date.toLocaleTimeString();

nit: {} on the first half if the else part has {}.

@@ +473,5 @@
> +                 sdf.timeFormatSeconds, date.getFullYear(),
> +                 date.getMonth() + 1, date.getDate(),
> +                 date.getHours(), date.getMinutes(), date.getSeconds());
> +             }
> +             elt.setAttribute("title", text);

Shouldn't this be done only if there's no existing title attribute?

(and then, how do we remove the auto-generated title attribute automatically once the popup is hidden, so that we update it the next time?)

Comment 15

[aleth [:aleth]]

Attachment: todayisspecial.diff v4

(In reply to Florian Quèze [:florian] [:flo] from comment #14)
> > +         let node = elt;
> > +         do {
> > +           if ("_originalMsg" in node) {
> 
> Any reason for not doing:
> for (let node = elt; node; node = node.parentNode) {
>   if (!node._originalMsg)
>      continue;

For some irrational reason I found the do...while version easier to read. But you're right, the for loop has less indentation and is shorter.

> Shouldn't this be done only if there's no existing title attribute?
> (and then, how do we remove the auto-generated title attribute automatically
> once the popup is hidden, so that we update it the next time?)

No, this already overrides any existing title attribute, to address the problem of showing the tooltip for a message the second time, with the second time being after some time has passed and it is tomorrow.

Comment 16

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #15)

> No, this already overrides any existing title attribute,

Isn't this a regression?

On IRC you won't see title attributes often, but on a twitter timeline you have a useful title with the real name for @mentions of people the user doesn't follow.

Comment 17

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> (In reply to aleth [:aleth] from comment #15)
> 
> > No, this already overrides any existing title attribute,
> 
> Isn't this a regression?
> 
> On IRC you won't see title attributes often, but on a twitter timeline you
> have a useful title with the real name for @mentions of people the user
> doesn't follow.

Hmm, or maybe you are going to tell me that there's no way for such an element inside the message (which gets wrapped in at least a <span class="ib-msg-txt"> element) to have the _originalMsg thing?

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8532489 [details] [diff] [review]
todayisspecial.diff v4

r=me assuming comment 17 is what you would have said.

Comment 19

[aleth [:aleth]]

Comment on attachment 8532489 [details] [diff] [review]
todayisspecial.diff v4

Review of attachment 8532489 [details] [diff] [review]:
-----------------------------------------------------------------

No, you're absolutely right this regresses the special twitter tooltips which I didn't know existed.

Comment 20

[Florian Quèze [:florian]]

Really? Why do these <a> tags have _originalMsg set?

Comment 21

[aleth [:aleth]]

Attachment: todayisspecial.diff v5

This patch does what you think it does ;)

Comment 22

[aleth [:aleth]]

Attachment: todayisspecial.diff v6

Since log messages also carry an _originalMsg, we can remove all the hardwired titles from the message styles and the associated code, as long as we use imTooltip in the logviewer as well.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8532628 [details] [diff] [review]
todayisspecial.diff v6

Review of attachment 8532628 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I haven't tried the patch myself. Do we also need some changes in mail/components/im/content?

Comment 24

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #23)
> Looks good, but I haven't tried the patch myself. Do we also need some
> changes in mail/components/im/content?

The required changes for TB are already in this patch.

Comment 25

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/a96669a6a6fc