Closed Bug 955641 Opened 11 years ago Closed 10 years ago

Open debug logs in a tab

Categories

(Instantbird Graveyard :: Account manager, enhancement)

Product:
Instantbird Graveyard
Component:
Account manager
Platform:
x86
Other
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

Attachments

(5 files, 6 obsolete files)

Formatting copied messages differently from what is shown
774 bytes, text/html
Details
Screenshot of latest patch
313.21 KB, image/png
Details
Patch v5
21.64 KB, patch
aleth
: review+
Details | Diff | Splinter Review
Fix a typo
866 bytes, patch
nhnt11
: review+
Details | Diff | Splinter Review
Package CSS files for aero
1.21 KB, patch
nhnt11
: review+
Details | Diff | Splinter Review 
*** 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.
Attached patch WIP (obsolete) — — Splinter Review 
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)
Attached image Screenshot of WIP (obsolete) — 

    
    

    
  
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.

    
    

    
  
(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 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+

    
    

    
  
(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.

    
    

    
  
(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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  


  
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 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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
Addressed review comments.

        Attachment #8512437 -
        Attachment is obsolete: true

        Attachment #8512437 -
        Flags: review?(florian)

        Attachment #8525010 -
        Flags: review?(aleth)

    
    

    
  
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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
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 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+

    
  
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.

    
    

    
  
(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.

    
  
Keywords: checkin-needed

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v4 (obsolete)
        — 
          — Splinter Review
      

    


  

        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 :).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v5
        — 
          — Splinter Review
      

    


  

        Attachment #8526007 -
        Attachment is obsolete: true

        Attachment #8526007 -
        Flags: review?(aleth)

        Attachment #8526010 -
        Flags: review?(aleth)

    
    

    
  
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+

    
  
Keywords: checkin-needed

    
  
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6

    
  
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

    
    

    
  
(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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix a typo
        — 
          — Splinter Review
      

    


  
This fixes a typo that snuck in. Sorry about this.
r=aleth over IRC.

        Attachment #8528360 -
        Flags: review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
  
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED

    
    

    
  


  
Makes things usable on Windows.
r=aleth over IRC.

        Attachment #8528946 -
        Flags: review+
Depends on: 1105712
Depends on: 1105716
Depends on: 1105717

    
  
Depends on: 1105798
Depends on: 1108557

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: