Closed Bug 955312 Opened 6 years ago Closed 5 years ago

Date information should be available for messages

Categories

(Instantbird :: Conversation, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: aleth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 7 obsolete files)

*** 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.
*** Original post on bio 1879 at 2013-02-19 17:37:17 UTC ***

Marking as new.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Attached image 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
Summary: Date information should be accessible for messages and status messages → Date information should be available for messages and status messages
Attached patch datetooltips.diff (obsolete) — Splinter Review
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.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attachment #8531636 - Flags: review?(clokep)
Blocks: 1099067
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.
Attachment #8531636 - Flags: review?(clokep) → review-
I also checked our own wiki page for these substitutions.
Attachment #8531636 - Attachment is obsolete: true
Attachment #8531772 - Flags: review?(clokep)
Attachment #8531772 - Flags: review?(clokep) → review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
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.
(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.
Attached patch addseconds.diff (obsolete) — Splinter Review
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.
Attachment #8532048 - Flags: review?(florian)
Comment on attachment 8532048 [details] [diff] [review]
addseconds.diff

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

OK, let's add this back.
Attachment #8532048 - Flags: review?(florian) → review+
Attached patch todayisspecial.diff (obsolete) — Splinter Review
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.
Attachment #8532240 - Flags: review?(florian)
Summary: Date information should be available for messages and status messages → Date information should be available for messages
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch todayisspecial.diff v2 (obsolete) — Splinter Review
Uses a shorter way of checking whether a Date is today.
Attachment #8532240 - Attachment is obsolete: true
Attachment #8532240 - Flags: review?(florian)
Attachment #8532299 - Flags: review?(florian)
Attached patch todayisspecial.diff v3 (obsolete) — Splinter Review
Bah, it's one of those files with odd indentation :-/
Attachment #8532299 - Attachment is obsolete: true
Attachment #8532299 - Flags: review?(florian)
Attachment #8532302 - Flags: review?(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?)
Attached patch todayisspecial.diff v4 (obsolete) — Splinter Review
(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.
Attachment #8532302 - Attachment is obsolete: true
Attachment #8532302 - Flags: review?(florian)
Attachment #8532489 - Flags: review?(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.
(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 on attachment 8532489 [details] [diff] [review]
todayisspecial.diff v4

r=me assuming comment 17 is what you would have said.
Attachment #8532489 - Flags: review?(florian) → review+
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.
Attachment #8532489 - Flags: review+ → review-
Really? Why do these <a> tags have _originalMsg set?
Attached patch todayisspecial.diff v5 (obsolete) — Splinter Review
This patch does what you think it does ;)
Attachment #8532489 - Attachment is obsolete: true
Attachment #8532604 - Flags: review?(florian)
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.
Attachment #8532048 - Attachment is obsolete: true
Attachment #8532604 - Attachment is obsolete: true
Attachment #8532604 - Flags: review?(florian)
Attachment #8532628 - Flags: review?(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?
Attachment #8532628 - Flags: review?(florian) → review+
(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.
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.