Closed Bug 955257 Opened 6 years ago Closed 6 years ago

Add a startDate attribute to prplIConversation

Categories

(Chat Core :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wnayes, Assigned: wnayes)

Details

Attachments

(2 files, 3 obsolete files)

*** Original post on bio 1824 at 2012-11-23 16:33:00 UTC ***

Keeping the start date of a conversation will simplify the import wizard patch. In a previous review (Bug 954928 (bio 1495)) it was also mentioned that implementing this would allow for a small fix in the theme code.

> What would you think of making startDate an attribute of prplIConversation?
> (another benefit is it would let us finally have a correct implementation of
> timeOpened in headerFooterReplacements (chat/modiles/imThemes.jsm).

Since there are benefits to having this change alone it makes sense to separate it from the importing code patch.
*** Original post on bio 1824 as attmnt 2125 at 2012-11-23 16:37:00 UTC ***

Here is what I have so far for this change. My main issue now is that it seems I have to dig deeper and deeper into purplexpcom and libpurple, which may or may not be necessary.

Otherwise I have covered all the other changes that would be needed (to the best of my knowledge) so that this value is being used when applicable.
Attachment #8353886 - Flags: feedback?(florian)
Assignee: nobody → wnayes
Status: NEW → ASSIGNED
Comment on attachment 8353886 [details] [diff] [review]
Initial implementation of startDate

*** Original change on bio 1824 attmnt 2125 at 2012-11-23 17:10:42 UTC ***

Is there a use case for calling getNewLogFileName without the aDate parameter? If not, you don't need the "date" variable.

PRTime isn't a JS Date object (I think it's just an int64 value). You will need to create a date object from PRTime value, and a PRTime value from the Date object in jsProtoHelper.

Looks promising otherwise! :)

(In reply to comment #1)
> My main issue now is that it seems
> I have to dig deeper and deeper into purplexpcom and libpurple, which may or
> may not be necessary.

You don't have to touch libpurple, purplexpcom should be enough.

You need to add a protected PRTime member (mStartDate?) to purpleConversation in purpleConversation.h, and initialize it in the constructor to PR_Now(). (That's the same thing in C++ for purplexpcom that you did in JS in jsProtoHelper).
Attachment #8353886 - Flags: feedback?(florian) → feedback+
Attached patch Working startDate attribute (obsolete) — Splinter Review
*** Original post on bio 1824 as attmnt 2126 at 2012-11-23 21:10:00 UTC ***

Thanks for the feedback! Everything seems to be working now.

> Is there a use case for calling getNewLogFileName without the aDate parameter?
> If not, you don't need the "date" variable.

The SystemLog handler also calls this method, and it only requires a new Date() for the filename. I kept the date variable for this reason; I think it's a safe fallback to handle an undefined aDate in getNewLogFileName, but having SystemLog call with a (new Date) would work too.

I have the startDate attribute returning PRTime integer values, rather than Dates, as that seems to ensure the type is always the same. If I'm understanding correctly, the purpleConversation.cpp is a C++ implementation of prplIConversation and the GenericConversationPrototype is a JS implementation. Keeping PRTime as an integer also seems to be the approach taken for imILog in logger.js.
Attachment #8353887 - Flags: review?(florian)
Comment on attachment 8353886 [details] [diff] [review]
Initial implementation of startDate

*** Original change on bio 1824 attmnt 2125 at 2012-11-23 21:10:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353886 - Attachment is obsolete: true
*** Original post on bio 1824 at 2012-11-24 16:43:59 UTC ***

(In reply to comment #3)
> Created attachment 8353887 [details] [diff] [review] (bio-attmnt 2126) [details]

What's the Math.round call for in:
+    this._date = Math.round(new Date() * 1000);
?
*** Original post on bio 1824 at 2012-11-24 19:20:02 UTC ***

(In reply to comment #4)
> What's the Math.round call for in:
> +    this._date = Math.round(new Date() * 1000);
> ?

I know there were other rounding calls for similar situations when handling Dates. I can't say for sure why other than that there could have been a need based on the existing code. :)

http://lxr.instantbird.org/instantbird/source/chat/components/src/logger.js#314
http://lxr.instantbird.org/instantbird/source/chat/protocols/twitter/twitter.js#318
http://lxr.instantbird.org/instantbird/source/chat/modules/jsProtoHelper.jsm#280
(The last link also confuses me as 1 millisecond = 1000 microseconds and new Date() outputs times in milliseconds, but if it works it works :) )
*** Original post on bio 1824 at 2012-11-24 19:37:39 UTC ***

(In reply to comment #5)
> I know there were other rounding calls for similar situations when handling
> Dates. I can't say for sure why other than that there could have been a need
> based on the existing code. :)

But then again only division could cause decimal place issues, so I guess there isn't a need in this case :). I am still confused as to why Messages convert the way they do...
*** Original post on bio 1824 at 2012-11-24 20:58:38 UTC ***

(In reply to comment #6)
> (In reply to comment #5)
> > I know there were other rounding calls for similar situations when handling
> > Dates. I can't say for sure why other than that there could have been a need
> > based on the existing code. :)
> 
> But then again only division could cause decimal place issues, so I guess there
> isn't a need in this case :).

Exactly, rounding makes sense after a division, not after a multiplication of 2 integers.
Comment on attachment 8353887 [details] [diff] [review]
Working startDate attribute

*** Original change on bio 1824 attmnt 2126 at 2012-11-24 21:01:29 UTC ***

This looks good to me. r=me with comment 4 addressed (and I will want to test this before check-in with both a js protocol and a libpurple protocol, but I assume you tested it already! :-)).
Attachment #8353887 - Flags: review?(florian) → review+
Comment on attachment 8353887 [details] [diff] [review]
Working startDate attribute

*** Original change on bio 1824 attmnt 2126 at 2012-11-30 01:09:58 UTC ***

After testing, it turns out the timeOpened replacement works (on so the startDate attribute is correct) for both js protocol conversations and libpurple protocol conversations, and the date is kept after putting the conversation on hold and redisplaying it (as expected).

However, this is broken when displaying a log file, and also broken in the fake conversation displayed in the Themes pane of the pref window.
Attachment #8353887 - Flags: review-
*** Original post on bio 1824 as attmnt 2144 at 2012-11-30 01:11:00 UTC ***

This is what I used to test attachment 8353887 [details] [diff] [review] (bio-attmnt 2126). In broken cases it displays "Conversation started at Invalid Date" at the top of the conversation.
*** Original post on bio 1824 as attmnt 2148 at 2012-12-01 01:43:00 UTC ***

This fixed the issues in the log viewer and the preferences theme preview window. The startDate attribute was added to the mock conversation in messagestyle.js and to the imILogConversation interface.
Attachment #8353910 - Flags: review?(florian)
Comment on attachment 8353887 [details] [diff] [review]
Working startDate attribute

*** Original change on bio 1824 attmnt 2126 at 2012-12-01 01:43:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353887 - Attachment is obsolete: true
*** Original post on bio 1824 at 2012-12-01 14:31:20 UTC ***

Comment on attachment 8353910 [details] [diff] [review] (bio-attmnt 2148)
Corrected log viewer and preference conversation

>@@ -336,16 +336,17 @@ function LogConversation(aLineInputStrea
>     let line = {value: ""};
>     let more = inputStream.readLine(line);
> 
>     if (!line.value)
>       throw "bad log file";
> 
>     if (firstFile) {
>       let data = JSON.parse(line.value);
>+      this.startDate = new Date(data.date) * 1000;

Why the "new Date" call?

>diff --git a/instantbird/content/preferences/messagestyle.js b/instantbird/content/preferences/messagestyle.js

> Conversation.prototype = {
>   __proto__: jsProtoHelper.GenericConvIMPrototype,
>+  get startDate() this._date,

Why is this needed? Don't we already inherit the same getter from jsProtoHelper?
*** Original post on bio 1824 as attmnt 2149 at 2012-12-01 21:06:00 UTC ***

>+  get startDate() this._date,

I removed the startDate getter from the preview conversation prototype. My first attempt at the fix was to set this.startDate right in the constructor, but that didn't work.

>>       let data = JSON.parse(line.value);
>>+      this.startDate = new Date(data.date) * 1000;

> Why the "new Date" call?

I tried without and was unable to get a value other than "6:00:00 PM" for any of the log timeOpened values. From what I could find, JSON doesn't parse into Date objects and the data.date value is a String.
Attachment #8353911 - Flags: review?(florian)
Comment on attachment 8353910 [details] [diff] [review]
Corrected log viewer and preference conversation

*** Original change on bio 1824 attmnt 2148 at 2012-12-01 21:06:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353910 - Attachment is obsolete: true
Attachment #8353910 - Flags: review?(florian)
Comment on attachment 8353911 [details] [diff] [review]
Removed unnecessary prototype getter

*** Original change on bio 1824 attmnt 2149 at 2012-12-05 23:19:47 UTC ***

Thanks!
Attachment #8353911 - Flags: review?(florian) → review+
*** Original post on bio 1824 at 2012-12-05 23:38:43 UTC ***

Thanks Will!

http://hg.instantbird.org/instantbird/rev/12e06551204f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.