Closed Bug 954668 Opened 10 years ago Closed 10 years ago

JSON log viewer conversation bubbles should have the same colors as when the conversation happened

Categories

(Instantbird Graveyard :: Other, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

(Whiteboard: [1.2-blocking])

Attachments

(1 file, 5 obsolete files)

*** Original post on bio 1236 at 2012-01-15 20:45:00 UTC ***

As MUCs seem fine, this is probably something to do with applying the incoming/outgoing styles. Note the colour used (for both incoming and outgoing messages) seems to depend on the conversation partner.
Summary: JSON log viewer conversation bubbles all have the same colour → JSON log viewer conversation bubbles all have the same colour for libpurple IM conversations
Severity: normal → blocker
Whiteboard: [1.2-wanted]
Target Milestone: --- → 1.2
Severity: blocker → normal
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1236 as attmnt 1414 at 2012-04-29 20:12:00 UTC ***

- Sets conv.isChat properly for conversations fetched from logs and adds the corresponding conditional to the log viewer.
Attachment #8353166 - Flags: review?(florian)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1236 as attmnt 1415 at 2012-04-29 20:16:00 UTC ***

- Obligatory second patch with nit fix.
Attachment #8353167 - Flags: review?(florian)
Comment on attachment 8353166 [details] [diff] [review]
Patch

*** Original change on bio 1236 attmnt 1414 at 2012-04-29 20:16:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353166 - Attachment is obsolete: true
Attachment #8353166 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1236 as attmnt 1416 at 2012-04-29 21:08:00 UTC ***

- Use .parent to get directory (thanks Mic!)
Attachment #8353168 - Flags: review?(florian)
Comment on attachment 8353167 [details] [diff] [review]
Patch

*** Original change on bio 1236 attmnt 1415 at 2012-04-29 21:08:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353167 - Attachment is obsolete: true
Attachment #8353167 - Flags: review?(florian)
Comment on attachment 8353168 [details] [diff] [review]
Patch

*** Original change on bio 1236 attmnt 1416 at 2012-04-29 21:45:35 UTC ***

- I don't like this approach of looking at the directory name.
- The twitter special case feels wrong, and would have to be removed anyway once we add support for direct messages.

The approach I would suggest is saving the isChat value with the other metadata saved in the first line of the log. Whether we should look for ".chat" in the directory name to workaround the lack of this isChat value for log files created with 1.2a1pre nightly builds could be discussed I guess, but my feeling about it is that the answer is "no".

Sorry for not saying this before you had time to attach 2 iterations of the patch after the first one :-/.
Attachment #8353168 - Flags: review?(florian) → review-
*** Original post on bio 1236 at 2012-04-29 21:52:34 UTC ***

Unassigning myself as these patches won't do, as per comment #4.

It seems this requires a discussion of the level of backwards compatibility that will be supported.

I don't know the full story of the twitter special case, I just noticed it's existence and followed suit.
Status: ASSIGNED → NEW
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1236 as attmnt 1419 at 2012-04-29 23:01:00 UTC ***

Adds an isChat flag to the log file header, while keeping the previous checks for backwards compatibility. 

Given the actual code of getLogsForConversation, http://lxr.instantbird.org/instantbird/source/chat/components/src/logger.js#452 , I don't see how a workaround based on that would be any simpler than this. But maybe I am missing something. Personally I'm also not too keen on things which appear to work but might break in the future. 

The alternative might be to just forget about backwards compatibility. That would simplify this patch, but you'd still need the twitter special case in the existing code to actually find existing 1.1 twitter log files :(
Comment on attachment 8353168 [details] [diff] [review]
Patch

*** Original change on bio 1236 attmnt 1416 at 2012-04-29 23:01:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353168 - Attachment is obsolete: true
*** Original post on bio 1236 at 2012-04-29 23:11:46 UTC ***

(In reply to comment #6)

> Given the actual code of getLogsForConversation,
> http://lxr.instantbird.org/instantbird/source/chat/components/src/logger.js#452
> , I don't see how a workaround based on that would be any simpler than this.

Adding another parameter to the _enumerateLogs method and the LogEnumerator constructor isn't pretty, but checking for the parent directory of the opened log file is really not great, as parsing the file differently based on where it's stored would be annoying if we ever want to reuse the same code for an importer (it's already an exposed API with the getLogFromFile method of the logger).
*** Original post on bio 1236 at 2012-04-29 23:41:38 UTC ***

Btw forgot to mention I didn't request review for the patch in 1419 because it will break for the case of future twitter DMs, as per comment 4. It's just there to save it for future reference.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1236 as attmnt 1420 at 2012-04-30 10:00:00 UTC ***

So, I don't see a good way to add isChat to be passed along _enumerateLogs, to the logEnumerator constructor, back to the caller, and then to the log viewer without it getting really messy. I guess it's probably better to just live with existing 1.2a1pre chat logs being less colourful (or write a sed script to add the isChat to the header).
Attachment #8353172 - Flags: review?(florian)
Comment on attachment 8353171 [details] [diff] [review]
Patch

*** Original change on bio 1236 attmnt 1419 at 2012-04-30 10:00:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353171 - Attachment is obsolete: true
*** Original post on bio 1236 at 2012-06-01 10:22:33 UTC ***

(In reply to comment #9)
> Created attachment 8353172 [details] [diff] [review] (bio-attmnt 1420) [details]
> Patch
> 
> So, I don't see a good way to add isChat to be passed along _enumerateLogs, to
> the logEnumerator constructor, back to the caller, and then to the log viewer
> without it getting really messy. I guess it's probably better to just live with
> existing 1.2a1pre chat logs being less colourful (or write a sed script to add
> the isChat to the header).

I wouldn't really mind as it wasn't a proper release from which these logs were generated. 

Could it be that |function LogConversation(aLineInputStream, aIsChat)| is a leftover from an earlier patch? aIsChat is not used anywhere in the latest patch.
Attached patch PatchSplinter Review
*** Original post on bio 1236 as attmnt 1543 at 2012-06-01 11:34:00 UTC ***

(In reply to comment #10)
> Could it be that |function LogConversation(aLineInputStream, aIsChat)| is a
> leftover from an earlier patch? aIsChat is not used anywhere in the latest
> patch.

Yes, good catch.
Attachment #8353298 - Flags: review?(florian)
Comment on attachment 8353172 [details] [diff] [review]
Patch

*** Original change on bio 1236 attmnt 1420 at 2012-06-01 11:34:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353172 - Attachment is obsolete: true
Attachment #8353172 - Flags: review?(florian)
*** Original post on bio 1236 at 2012-06-29 11:27:15 UTC ***

(In reply to comment #11)
> Created attachment 8353298 [details] [diff] [review] (bio-attmnt 1543) [details]

I *think* the isChat getter would return "undefined" if there's a log that is missing the isChat field. That would be the case for all our nightly logs since we introduced the JSON logs. I'd be glad if they'd still work.

Maybe defining the getter as |get isChat() this._isChat == true| would work. That would return false in worst case if I'm not mistaken.
*** Original post on bio 1236 at 2012-06-29 12:29:14 UTC ***

(In reply to comment #12)
> (In reply to comment #11)
> > Created attachment 8353298 [details] [diff] [review] (bio-attmnt 1543) [details]
> 
> I *think* the isChat getter would return "undefined" if there's a log that is
> missing the isChat field. That would be the case for all our nightly logs since
> we introduced the JSON logs. I'd be glad if they'd still work.
> 
> Maybe defining the getter as |get isChat() this._isChat == true| would work.
> That would return false in worst case if I'm not mistaken.

The current patch actually doesn't cause any problems. Existing MUC logs get displayed conversation-style without any errors/warnings.
*** Original post on bio 1236 at 2012-07-31 15:08:19 UTC ***

If we want to change the JSON log format, it's blocking 1.2.
Whiteboard: [1.2-wanted] → [1.2-blocking]
Comment on attachment 8353298 [details] [diff] [review]
Patch

*** Original change on bio 1236 attmnt 1543 at 2012-08-04 22:14:19 UTC ***

I think this patch contains all we need for the 1.2 release, that is so that people who upgrade from 1.1 to 1.2 never notice anything broken.

For users of 1.2a1pre, I'm not completely sure of what we should do, but it doesn't matter much now because these users will be updated to nightlies, so if we decide to add a hack for backward compatibility, we can easily do it later.

I think the solution of offering a sed (or whatever language) script to fix the existing logs created by 1.2a1pre makes sense though.
Attachment #8353298 - Flags: review?(florian) → review+
*** Original post on bio 1236 at 2012-08-04 22:36:22 UTC ***

Rephrased the summary as it was quite misleading.
Summary: JSON log viewer conversation bubbles all have the same colour for libpurple IM conversations → JSON log viewer conversation bubbles should have the same colors as when the conversation happened
*** Original post on bio 1236 at 2012-08-04 22:41:18 UTC ***

(In reply to comment #16)
> Rephrased the summary as it was quite misleading.

Ah no, it was directly related to libpurple that doesn't set the "who" of messages correctly for private conversations, I misread it. Anyway, the new phrasing describes what the patch actually does.

We should probably also fix the libpurple issue, as we are logging incorrect "who" values in lots of logs.

https://hg.instantbird.org/instantbird/rev/70243c558e3b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.