Open debug logs in a tab

RESOLVED FIXED in 1.6

Status

--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nhnt11, Assigned: nhnt11)

Tracking

(Depends on: 4 bugs)

trunk
x86
Other
Dependency tree / graph

Details

Attachments

(5 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
*** Original post on bio 2196 at 2013-10-03 13:33:00 UTC ***

Now that we have the ability to put arbitrary content in tabs, we should show debug logs in a tab instead of "Copy debug log". This eliminates the need for an external text editor and would allow us to format the logs into a more readable format.
(Assignee)

Comment 1

5 years ago
Created attachment 8379251 [details] [diff] [review]
WIP

This is a WIP. The "Copy debug log" menu item now opens a tab that displays the debug log with some basic formatting.

Todo:
1. Better formatting - fonts, colors (?), etc
2. Add a dropdown menu from which you can choose to view another account's log.
3. (l10n) Change "Copy debug log" to "Show debug log"
4. ???
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8379251 - Flags: feedback?(florian)
Attachment #8379251 - Flags: feedback?(benediktp)
Attachment #8379251 - Flags: feedback?(aleth)
(Assignee)

Comment 2

5 years ago
Created attachment 8379252 [details]
Screenshot of WIP
Comment on attachment 8379251 [details] [diff] [review]
WIP

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

Exciting! :-)

::: im/content/accounts.js
@@ +222,5 @@
>        account.connect();
>    },
>    copyDebugLog: function am_copyDebugLog() {
>      let account = this.accountList.selectedItem.account;
> +    let win = Services.wm.getMostRecentWindow("Messenger:convs");

There's indeed some code duplication here with imWindows.jsm. We need to find a way to factor this code.

::: im/content/debugLog.html
@@ +1,1 @@
> +<html>

I think you'll at least want to add the doctype and charset.

::: im/content/debugLogPanel.xml
@@ +24,5 @@
> +        <![CDATA[
> +          let account = Services.accounts.getAccountById(aAccountId);
> +          this.tab.setAttribute("label", "Debug log for " + account.name);
> +          let doc = this.browser.contentDocument;
> +          let table = doc.createElement("table");

I'm not sure a <table> is the best solution here. Have you tried using <ul>/<li>?

::: im/content/jar.mn
@@ +34,5 @@
>  	content/instantbird/conversation.xml
>  	content/instantbird/conv.xml
>  	content/instantbird/credits.xhtml
> +	content/instantbird/debugLogPanel.xml
> +	content/instantbird/debugLog.html

This _may_ want to live in chat/ if we find a good place to display it in the Thunderbird UI.
Attachment #8379251 - Flags: feedback?(florian) → feedback+
Comment on attachment 8379251 [details] [diff] [review]
WIP

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

::: im/content/accounts.js
@@ +220,5 @@
>      // Reconnect the account if an exception was added.
>      if (params.exceptionAdded)
>        account.connect();
>    },
>    copyDebugLog: function am_copyDebugLog() {

I'd suggest we still want a way to COPY debug logs, e.g. for copying into bug reports, etc.
(Assignee)

Comment 5

5 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> > +<html>
> 
> I think you'll at least want to add the doctype and charset.

Ah, meant to do that but forgot.

> > +          let table = doc.createElement("table");
> 
> I'm not sure a <table> is the best solution here. Have you tried using
> <ul>/<li>?

I used a table so that the log message would stay aligned (and not overflow below the timestamp).

(In reply to Patrick Cloke [:clokep] from comment #4)

> I'd suggest we still want a way to COPY debug logs, e.g. for copying into
> bug reports, etc.

Yup, but I was thinking of adding a way to do that from the tab itself (like a "Copy to clipboard" button or something?)

Comment 6

5 years ago
Comment on attachment 8379251 [details] [diff] [review]
WIP

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

A favicon would be nice ;) (hopefully there is something already existing that is vaguely suitable).

Are you intending the account context menu to have Show debug log in addition to Copy or will this replace Copy (with the Copy button living in the log pane)?

Some ideas to play around with:
Print the timestamp and LOG/WARN/ERROR in a different color (grey) and a smaller font to save space. Possibly on a separate line above the actual log entry.
Is there a simple way to distinguish incoming/outgoing messages?

::: im/content/accounts.js
@@ +228,5 @@
> +      if (!aWindow)
> +        return false;
> +      let logPanel = aWindow.document.createElementNS(
> +        "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> +        "debugLogPanel");

Can't you use Conversations.showTab("debugLogPanel") and showTab("newTab")?

@@ +230,5 @@
> +      let logPanel = aWindow.document.createElementNS(
> +        "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> +        "debugLogPanel");
> +      if (!aWindow.getTabBrowser().addPanel(logPanel))
> +        return false;

Does this imply there can be only one debug log panel? Can we change this?
Attachment #8379251 - Flags: feedback?(aleth) → feedback+
(Assignee)

Comment 7

5 years ago
(In reply to aleth from comment #6)

> Are you intending the account context menu to have Show debug log in
> addition to Copy or will this replace Copy (with the Copy button living in
> the log pane)?

I'm inclined towards the latter but I'd like to hear everyone's opinions.

> @@ +230,5 @@
> > +      let logPanel = aWindow.document.createElementNS(
> > +        "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
> > +        "debugLogPanel");
> > +      if (!aWindow.getTabBrowser().addPanel(logPanel))
> > +        return false;
> 
> Does this imply there can be only one debug log panel? Can we change this?

Ah, I should've been a bit more weary while copy pasting the code that opens an awesometab.

Comment 8

5 years ago
(In reply to Nihanth Subramanya from comment #7)
> > Does this imply there can be only one debug log panel? Can we change this?
> 
> Ah, I should've been a bit more weary while copy pasting the code that opens
> an awesometab.

Looks like showTab() needs another parameter (and the logPanel needs an init method) ;)
Comment on attachment 8379251 [details] [diff] [review]
WIP

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

aleth and flo already gave good feedback. I'll attach an example on how you can show your messages in one way while keeping a nice format when being copied. See next comment.
Attachment #8379251 - Flags: feedback?(benediktp) → feedback+
Created attachment 8383849 [details]
Formatting copied messages differently from what is shown
(Assignee)

Comment 11

4 years ago
Created attachment 8512437 [details] [diff] [review]
Patch

I found some time to push this forward. This new patch introduces a toolbar at the top that lets you switch accounts, and buttons to refresh as well as copy the log.

Also, the way to open the panel is now using a "/debug" command within a conversation. I've removed the code that opens it from the Accounts window for now, but in the next patch I want to change the "Copy Debug Log" option to "Show Debug Log" and find a way to open the panel without too much duplication.

One thing I want to note is that some LOG messages are displayed with a trailing newline. I found this was necessary to ensure the copied log is identical to what you would get with the current "Copy Debug Log" option, but maybe there's a way to hide the newline in UI?
Attachment #8379251 - Attachment is obsolete: true
Attachment #8379252 - Attachment is obsolete: true
Attachment #8512437 - Flags: review?(florian)
Attachment #8512437 - Flags: review?(aleth)
(Assignee)

Comment 12

4 years ago
Created attachment 8512439 [details]
Screenshot of latest patch

Here's what it looks like right now on OS X. I haven't yet tested the theming for other platforms (maybe I should have requested f?).

BTW, as you can see I haven't tweaked any of the styling as per the suggestions above and am not really too motivated to do that at the moment :(
Maybe it can be a follow up?

Comment 13

4 years ago
Comment on attachment 8512437 [details] [diff] [review]
Patch

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

Looking forward to this!

Perfecting the CSS for other OS can be left for followups I think.

>I've removed the code that opens it from the Accounts window for now, but in the next patch I want to change the "Copy Debug Log" option to "Show Debug Log" and find a way to open the panel without too much duplication.

We could keep both as entries in the account context menu.

>One thing I want to note is that some LOG messages are displayed with a trailing newline. I found this was necessary to ensure the copied log is identical to what you would get with the current "Copy Debug Log" option, but maybe there's a way to hide the newline in UI?

You could check the message doesn't already end in a newline before adding another? Or look at the prpl line creating the LOG message and avoid the newline there?

::: im/content/debugLogPanel.xml
@@ +98,5 @@
> +          let account = Services.accounts.getAccountById(accId);
> +          this.tab.setAttribute("label", "Debug log for " + account.name);
> +          let doc = this.browser.contentDocument;
> +          let table = doc.createElement("table");
> +          table.style = "width:100%";

nit: space

@@ +99,5 @@
> +          this.tab.setAttribute("label", "Debug log for " + account.name);
> +          let doc = this.browser.contentDocument;
> +          let table = doc.createElement("table");
> +          table.style = "width:100%";
> +          doc.body.innerHTML = "";

Why is this needed?

@@ +134,5 @@
> +                level = "DEBUG";
> +                rowClass = "debug";
> +              }
> +              formattedMsg = level + " (@ " + m.sourceLine +
> +                     " " + m.sourceName + ":" + m.lineNumber + ")\n" +

Is the indentation correct here or just wrong on bugzilla?

::: im/content/menus.js
@@ +16,5 @@
>  var menus = {
>    supportsCommand: function(aCmd)
>      aCmd == "cmd_addbuddy" || aCmd == "cmd_joinchat" || aCmd == "cmd_newtab",
>    isCommandEnabled: function(aCmd) {
> +    return true;

wat?

::: im/modules/ibCore.jsm
@@ +169,5 @@
> +          });
> +          return true;
> +        }
> +        // Try to show the debug logs in win, and open a new window if it didn't work.
> +        if (!showDebugLogs(win)) {

Better: if (showDebugLogs) return true;

::: im/themes/debugLog.css
@@ +36,5 @@
> +}
> +
> +td.time {
> +  white-space:nowrap;
> +  vertical-align:top;

nit: spaces

::: im/themes/debugLogPanel.css
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +%ifdef XP_UNIX
> +%ifndef XP_MACOSX
> +%define XP_LINUX

Appears to be unused.
Attachment #8512437 - Flags: review?(aleth)
Attachment #8512437 - Flags: review-
Attachment #8512437 - Flags: feedback+
(Assignee)

Comment 14

4 years ago
Created attachment 8525010 [details] [diff] [review]
Patch v2

Addressed review comments.
Attachment #8512437 - Attachment is obsolete: true
Attachment #8512437 - Flags: review?(florian)
Attachment #8525010 - Flags: review?(aleth)

Comment 15

4 years ago
Comment on attachment 8525010 [details] [diff] [review]
Patch v2

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

::: im/content/accounts.js
@@ +258,5 @@
>    },
> +  showDebugLog: function am_showDebugLog() {
> +    if (!"Core" in window)
> +      Cu.import("resource:///modules/ibCore.jsm");
> +    Core.showTab("debugLogPanel", aPanel => {

Since this is calling Core anyway, you can deduplicate this function by adding a showDebugLog method in Core.

::: im/modules/ibCore.jsm
@@ +94,5 @@
>        name: "about",
>        get helpString() self.bundle("aboutCommand.help"),
>        usageContext: Ci.imICommand.CMD_CONTEXT_ALL,
>        priority: Ci.imICommand.CMD_PRIORITY_DEFAULT,
> +      run: (aMsg, aConv) => {

If you are making this change, does that mean you can also get rid of the "let self = this" a few lines earlier for consistency?

@@ +189,5 @@
> +  showTab: function(aPanelName, aCallback) {
> +    // Try to get the most recent conversation window. If no such window exists,
> +    // win will be null.
> +    let win = Services.wm.getMostRecentWindow("Messenger:convs");
> +    // Tries to open a debugLogPanel in the specified window.

This comment is wrong (at least I hope it's just the comment) - it could be an about page etc.
Attachment #8525010 - Flags: review?(aleth) → review-
(Assignee)

Comment 16

4 years ago
Created attachment 8525801 [details] [diff] [review]
Patch v3

Sorry about that comment.
I can't get rid of the self variable because self.bundle(...) is being used from a getter, and I couldn't find a way to use arrow functions with getters.
Attachment #8525010 - Attachment is obsolete: true
Attachment #8525801 - Flags: review?(aleth)

Comment 17

4 years ago
Comment on attachment 8525801 [details] [diff] [review]
Patch v3

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

Thanks for adding this!
Attachment #8525801 - Flags: review?(aleth) → review+

Updated

4 years ago
Severity: normal → enhancement
Keywords: checkin-needed
Comment on attachment 8525801 [details] [diff] [review]
Patch v3

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

Some drive-by comments; don't block this on me doing a full review, I probably won't have time in the near future.

::: im/content/debugLog.html
@@ +1,1 @@
> +<html>

Should this have a license header? A charset declaration to avoid warnings?

::: im/modules/ibCore.jsm
@@ +94,5 @@
>        name: "about",
>        get helpString() self.bundle("aboutCommand.help"),
>        usageContext: Ci.imICommand.CMD_CONTEXT_ALL,
>        priority: Ci.imICommand.CMD_PRIORITY_DEFAULT,
> +      run: (aMsg, aConv) => {

Why the arrow function here?

@@ +103,5 @@
>            Services.io.newChannelFromURI(Services.io.newURI(url, null, null));
>          } catch(e) {
>            if (e.result == Components.results.NS_ERROR_MALFORMED_URI) {
>              Services.conversations.getUIConversation(aConv).systemMessage(
>                self.bundle("aboutCommand.invalidPageMessage", page));

'self' is used here.

@@ +109,5 @@
>            }
>            Components.utils.reportError(e); // Log unexpected errors.
>            return false;
>          }
> +        this.showTab("aboutPanel", aPanel => aPanel.showAboutPage(page));

and 'this' here.

Comment 19

4 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> ::: im/modules/ibCore.jsm
> @@ +94,5 @@
> >        name: "about",
> >        get helpString() self.bundle("aboutCommand.help"),
> >        usageContext: Ci.imICommand.CMD_CONTEXT_ALL,
> >        priority: Ci.imICommand.CMD_PRIORITY_DEFAULT,
> > +      run: (aMsg, aConv) => {
> 
> Why the arrow function here?
> 
> @@ +103,5 @@
> >            Services.io.newChannelFromURI(Services.io.newURI(url, null, null));
> >          } catch(e) {
> >            if (e.result == Components.results.NS_ERROR_MALFORMED_URI) {
> >              Services.conversations.getUIConversation(aConv).systemMessage(
> >                self.bundle("aboutCommand.invalidPageMessage", page));
> 
> 'self' is used here.
> 
> @@ +109,5 @@
> >            }
> >            Components.utils.reportError(e); // Log unexpected errors.
> >            return false;
> >          }
> > +        this.showTab("aboutPanel", aPanel => aPanel.showAboutPage(page));
> 
> and 'this' here.

Since this has confused both flo and me now, it's probably best if you swap the arrow functions in run() for self references, unless there is a good reason not to.

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 20

4 years ago
Created attachment 8526007 [details] [diff] [review]
Patch v4
Attachment #8525801 - Attachment is obsolete: true
Attachment #8526007 - Flags: review?(aleth)
Comment on attachment 8526007 [details] [diff] [review]
Patch v4

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

::: im/content/debugLog.html
@@ +2,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!DOCTYPE html>
> + <html>

Nit: no indent on this line please :).
(Assignee)

Comment 22

4 years ago
Created attachment 8526010 [details] [diff] [review]
Patch v5
Attachment #8526007 - Attachment is obsolete: true
Attachment #8526007 - Flags: review?(aleth)
Attachment #8526010 - Flags: review?(aleth)

Comment 23

4 years ago
Comment on attachment 8526010 [details] [diff] [review]
Patch v5

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

Thanks! We can leave details like disabling when the debug log is empty and favicons for a followup, as long as nothing breaks when the debug log is empty.
Attachment #8526010 - Flags: review?(aleth) → review+

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6

Updated

4 years ago
Depends on: 1102792
I got an error when trying to open this on Windows:

Timestamp: 11/21/2014 6:40:50 AM
Error: ReferenceError: Core is not defined
Source File: chrome://instantbird/content/accounts.js
Line: 262
Comment on attachment 8526010 [details] [diff] [review]
Patch v5

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

::: im/content/accounts.js
@@ +256,5 @@
>      Cc["@mozilla.org/widget/clipboardhelper;1"]
>        .getService(Ci.nsIClipboardHelper).copyString(text);
>    },
> +  showDebugLog: function am_showDebugLog() {
> +    if (!"Core" in window)

I'm fairly certain this is wrong. ! has a higher precedence than in [1], so this evaluates to:
(!"Core") in window

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence

Comment 27

4 years ago
(In reply to Patrick Cloke [:clokep] from comment #26)
> I'm fairly certain this is wrong. ! has a higher precedence than in [1], so
> this evaluates to:
> (!"Core") in window

Good catch, yes, that's definitely wrong. Sorry I missed this on review.
(Assignee)

Comment 29

4 years ago
Created attachment 8528360 [details] [diff] [review]
Fix a typo

This fixes a typo that snuck in. Sorry about this.
r=aleth over IRC.
Attachment #8528360 - Flags: review+
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 31

4 years ago
Created attachment 8528946 [details] [diff] [review]
Package CSS files for aero

Makes things usable on Windows.
r=aleth over IRC.
Attachment #8528946 - Flags: review+
(Assignee)

Updated

4 years ago
Depends on: 1105712
(Assignee)

Updated

4 years ago
Depends on: 1105716
(Assignee)

Updated

4 years ago
Depends on: 1105717

Updated

4 years ago
Depends on: 1105798
(Assignee)

Updated

4 years ago
Depends on: 1108557
You need to log in before you can comment on or make changes to this bug.