Closed Bug 955162 Opened 7 years ago Closed 7 years ago

Port the log tree and concatenated daily logs from TB

Categories

(Instantbird :: Other, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

()

Details

Attachments

(4 files, 8 obsolete files)

*** Original post on bio 1732 at 2012-10-18 18:55:00 UTC ***

Just that.
Severity: normal → enhancement
Summary: Port bug 787149 and the tree-style log selector from TB → Port the log tree and concatenated daily logs from TB
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1732 as attmnt 2208 at 2013-01-26 18:08:00 UTC ***

Changes/enhancements from the TB version:
- Can handle legacy txt logs.
- Select first entry in the log tree on opening the log window even if the first entry is not a group.
- Putting all the logs older than 2 weeks in a single group was not making the best use of the log tree. Instead, they are now grouped by month. The strings for this come from the FF history sidebar.
- Insert a ruler between sessions when viewing a concatenated daily log.
- Add these rulers to section scroll.
- No longer returns no log for the selected day when a the log file for that day is broken, but only if all of them are bad.

I only didn't r? because there is an error string required (either from logger.js or convbrowser - both are in /chat) and I wasn't sure which string bundle is appropriate to use for this. (There are also s/#loglist/#logTree changes missing for CSS files from other OS, and the appropriate jar.mn additions.)
Attachment #8353971 - Flags: feedback?(florian)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
*** Original post on bio 1732 at 2013-01-27 14:24:43 UTC ***

(In reply to comment #1)

> - Insert a ruler between sessions when viewing a concatenated daily log.

Isn't this something that Jb kept asking for Thunderbird?
*** Original post on bio 1732 at 2013-01-27 14:35:27 UTC ***

(In reply to comment #2)
> Isn't this something that Jb kept asking for Thunderbird?

Great - unless it's taken out by hand, it will be in TB after the next merge in the other direction ;)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1732 as attmnt 2209 at 2013-01-27 14:48:00 UTC ***

Some additions to the Bubbles style for the new ruler.
Attachment #8353972 - Flags: feedback?(florian)
Comment on attachment 8353971 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2208 at 2013-01-27 14:48:55 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353971 - Attachment is obsolete: true
Attachment #8353971 - Flags: feedback?(florian)
*** Original post on bio 1732 as attmnt 2210 at 2013-01-28 19:25:00 UTC ***

Two possible ways of styling the time bubble text around the new ruler. (Neither gives the correct impression in all circumstances, but I don't think we have to worry about that too much at this point.)
*** Original post on bio 1732 as attmnt 2211 at 2013-01-28 19:25:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Duplicate of this bug: 954567
Duplicate of this bug: 953735
Comment on attachment 8353972 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2209 at 2013-03-24 19:14:12 UTC ***

Sorry for the delay. I didn't do a full detailed review, but this seems promising :). I gave feedback on IRC: http://log.bezut.info/instantbird/130324#m375
Attachment #8353972 - Flags: feedback?(florian) → feedback+
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1732 as attmnt 2301 at 2013-03-27 12:29:00 UTC ***

Fixes review comments, includes the time text changes to Bubbles for the sessionstart-ruler and adds jar.mn changes. Not sure about where/if the new file in /modules should be listed.
Attachment #8354067 - Flags: review?(florian)
Comment on attachment 8353972 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2209 at 2013-03-27 12:29:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353972 - Attachment is obsolete: true
*** Original post on bio 1732 at 2013-03-27 12:34:22 UTC ***

Comment on attachment 8354067 [details] [diff] [review] (bio-attmnt 2301)
Patch

>diff --git a/modules/iteratorUtils.jsm b/modules/iteratorUtils.jsm
This should probably go into instantbird/modules/ and you'll need to modify [1]

[1] http://lxr.instantbird.org/instantbird/source/instantbird/modules/Makefile.in#12
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1732 as attmnt 2302 at 2013-03-27 12:47:00 UTC ***

Thanks clokep. (Sorry about the paths that are still wrong, but at least all the changes are there.)
Attachment #8354068 - Flags: review?(florian)
Comment on attachment 8354067 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2301 at 2013-03-27 12:47:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354067 - Attachment is obsolete: true
Attachment #8354067 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1732 as attmnt 2303 at 2013-03-27 13:31:00 UTC ***

Mic suggested using Cu/Ci/Cc in viewlog.js.
Attachment #8354069 - Flags: review?(florian)
Comment on attachment 8354068 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2302 at 2013-03-27 13:31:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354068 - Attachment is obsolete: true
Attachment #8354068 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1732 as attmnt 2305 at 2013-03-27 15:31:00 UTC ***

Removing iteratorUtils was a good call, completely painless.
Attachment #8354071 - Flags: review?(florian)
Comment on attachment 8354069 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2303 at 2013-03-27 15:31:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354069 - Attachment is obsolete: true
Attachment #8354069 - Flags: review?(florian)
*** Original post on bio 1732 at 2013-03-28 11:34:00 UTC ***

(In reply to comment #14)
> Removing iteratorUtils was a good call, completely painless.
(Of course the reason it was easy is that utilities.js is a very stripped down iteratorUtils ;) When including iteratorUtils I had assumed we would drop utilities.js for it in the next step.)
Comment on attachment 8354071 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2305 at 2013-04-05 22:13:26 UTC ***

I haven't done a real review of jsTreeView.js, I assume it comes from Tb and we want to keep it (mostly) unchanged.

>+  /**
>+   * Display a list of logs into a tree, and optionally handle a default selection.
>+   *
>+   * @param aLogs An nsISimpleEnumerator of imILog.
>+   * @param aShouldSelect Either a boolean (true means select the first log
>+   * of the list, false or undefined means don't mess with the selection) or a log
>+   * item that needs to be selected.
>+   * @returns true if there's at least one log in the list, false if empty.
>+   */
>+  _showLogList: function(aLogs, aShouldSelect) {

This is called only once, the return value isn't used, and aShouldSelect is always true, so I think you can simplify this code a lot, and even inline it (there doesn't seem to be a reason for this to be a separate function in Instantbird).


A few questions:
- have you checked that this is displayed correctly with non-Bubbles message themes?
- Is there anything that I should be afraid of when merging these chat/ changes into c-c for Tb?

Looks like great work otherwise!

(Note: I still haven't actually tried it.)
Attachment #8354071 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1732 as attmnt 2325 at 2013-04-06 12:12:00 UTC ***

Fixes bug 955357 (bio 1920) en passant, as I was changing that part of the code anyway with this patch.

(In reply to comment #16)
> I haven't done a real review of jsTreeView.js, I assume it comes from Tb and we
> want to keep it (mostly) unchanged.
It's unchanged from the TB version.

> >+  _showLogList: function(aLogs, aShouldSelect) {
> This is called only once, the return value isn't used, and aShouldSelect is
> always true, so I think you can simplify this code a lot, and even inline it
> (there doesn't seem to be a reason for this to be a separate function in
> Instantbird).
I kept _showLogList separate in case some of the changes/bugfixes I made there and in the following are also wanted for TB. I generally tried to keep the IB-specific changes as isolated as possible, to make future merges in both directions easier. 

> - have you checked that this is displayed correctly with non-Bubbles message
> themes?
Yes. (The sessionstart-ruler just gets the default hr style, which is good enough for a default I think.)

I think improvements to the CSS for the various message styles (should they be required) can be done as followups.

> - Is there anything that I should be afraid of when merging these chat/ changes
> into c-c for Tb?
No. TB will get the sessionstart-ruler that way of course, and one may wish to improve the way that looks for the TB message style. The changes to the log tree and "time bubbles" would have to be ported by hand of course.
Attachment #8354091 - Flags: review?(florian)
Comment on attachment 8354071 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2305 at 2013-04-06 12:12:45 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354071 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1732 as attmnt 2326 at 2013-04-06 13:01:00 UTC ***

Inlines the required part of _showLogList, as discussed on IRC.
Attachment #8354092 - Flags: review?(florian)
Comment on attachment 8354091 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2325 at 2013-04-06 13:01:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354091 - Attachment is obsolete: true
Attachment #8354091 - Flags: review?(florian)
Attached patch PatchSplinter Review
*** Original post on bio 1732 as attmnt 2327 at 2013-04-06 13:19:00 UTC ***

Improve comment.
Attachment #8354093 - Flags: review?(florian)
Comment on attachment 8354092 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2326 at 2013-04-06 13:19:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354092 - Attachment is obsolete: true
Attachment #8354092 - Flags: review?(florian)
*** Original post on bio 1732 at 2013-04-06 21:57:43 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/157e2bdda936 after unbitrotting.

Let's make sure to test this in tomorrow's nightly and file bugs if there are issues!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
Comment on attachment 8354093 [details] [diff] [review]
Patch

*** Original change on bio 1732 attmnt 2327 at 2013-04-06 22:35:30 UTC ***

This didn't apply cleanly:
- there's an annoying Mac ifdef in viewlogs.xul that you patch doesn't have
- for some reason the tabs in jar.mn files have been replaced by spaces in your patch.

Unfortunately I noticed after pushing that there's an ugly white background on Mac (see http://i1.minus.com/iqdGu8oBzouC1.png) caused by http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/tree.css#31
Attachment #8354093 - Flags: review?(florian) → review+
*** Original post on bio 1732 as attmnt 2329 at 2013-04-06 22:38:00 UTC ***

I'm not really satisfied by adding this duplication, but I couldn't find anything better :(.

Here's a screenshot with this additional patch: http://i2.minus.com/iGTMfIts8QYcC.png It also shows the sessionstart rulers. I look forward to trying this tomorrow with real logs! :-)
Attachment #8354095 - Flags: review?
Comment on attachment 8354095 [details] [diff] [review]
Follow-up to fix Mac tree background

*** Original change on bio 1732 attmnt 2329 at 2013-04-07 21:03:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354095 - Flags: review? → review?(aleth)
*** Original post on bio 1732 at 2013-04-08 10:00:12 UTC ***

Comment on attachment 8354095 [details] [diff] [review] (bio-attmnt 2329)
Follow-up to fix Mac tree background

>+/* Unfortunately the previous rule also overrides the backgrounds of
>+selected rows, so set them again like they are in tree.css. */
>+treechildren::-moz-tree-row(selected) {
>+  background-color: -moz-mac-secondaryhighlight;
>+}

Shouldn't there be a Mac-specific ifdef around these additions? (There certainly should be around the -moz-mac-... because it's unclear how it will behave on other OS).

I can't see a neater way of doing this either, though I would ask if
treechildren::-moz-tree-row:not([selected]) {
  background-color: transparent;
}
doesn't save you one of the CSS rules (I'm not confident about the behaviour of moz-tree-row()).
*** Original post on bio 1732 at 2013-04-08 10:40:52 UTC ***

(In reply to comment #23)
> Comment on attachment 8354095 [details] [diff] [review] (bio-attmnt 2329) [details]
> Follow-up to fix Mac tree background
> 
> >+/* Unfortunately the previous rule also overrides the backgrounds of
> >+selected rows, so set them again like they are in tree.css. */
> >+treechildren::-moz-tree-row(selected) {
> >+  background-color: -moz-mac-secondaryhighlight;
> >+}
> 
> Shouldn't there be a Mac-specific ifdef around these additions? (There
> certainly should be around the -moz-mac-... because it's unclear how it will
> behave on other OS).

There's not enough context to see it in the patch, but it's all inside an existing ifdef XP_MACOSX block.

> treechildren::-moz-tree-row:not([selected]) {

I would be surprised if it worked, but I'll try.
Comment on attachment 8354095 [details] [diff] [review]
Follow-up to fix Mac tree background

*** Original change on bio 1732 attmnt 2329 at 2013-04-08 11:13:25 UTC ***

(In reply to comment #24)
> There's not enough context to see it in the patch, but it's all inside an
> existing ifdef XP_MACOSX block.
I missed that, sorry.

> > treechildren::-moz-tree-row:not([selected]) {
> I would be surprised if it worked, but I'll try.
If it should work, change it if you agree it's an improvement.
Attachment #8354095 - Flags: review?(aleth) → review+
*** Original post on bio 1732 at 2013-04-09 21:41:15 UTC ***

(In reply to comment #24)

> > treechildren::-moz-tree-row:not([selected]) {
> 
> I would be surprised if it worked, but I'll try.

Causes this:
Warning: Found trailing token after pseudo-element, which must be the last part of a selector:  ':'.  Ruleset ignored due to bad selector.
Source File: chrome://instantbird/skin/viewlog.css
You need to log in before you can comment on or make changes to this bug.