Last Comment Bug 662807 - Error bubbles for multiline messages in console look silly
: Error bubbles for multiline messages in console look silly
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- trivial (vote)
: Firefox 8
Assigned To: Rob Campbell [:rc] (:robcee)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-08 08:33 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2011-07-25 06:45 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot (192.09 KB, image/png)
2011-06-08 08:33 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details
silly-bubbles (1.65 KB, patch)
2011-07-15 00:49 PDT, Rob Campbell [:rc] (:robcee)
dao+bmo: review-
Details | Diff | Review
[checked-in] silly-bubbles-2 (4.45 KB, patch)
2011-07-17 14:28 PDT, Rob Campbell [:rc] (:robcee)
dao+bmo: review+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2011-06-08 08:33:04 PDT
Created attachment 538024 [details]
Screenshot

See screenshot.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-07-15 00:49:38 PDT
Created attachment 546113 [details] [diff] [review]
silly-bubbles
Comment 2 Dão Gottwald [:dao] 2011-07-15 08:43:24 PDT
Comment on attachment 546113 [details] [diff] [review]
silly-bubbles

The right solution here is to put the label in a container with align="start".
Comment 3 Rob Campbell [:rc] (:robcee) 2011-07-15 10:40:27 PDT
d'oh. I was hoping something simple like this would be sufficient. Adding a container (a div? any suggestions on which container I should use?) will require modifying some of the HUDService.jsm.
Comment 4 Rob Campbell [:rc] (:robcee) 2011-07-17 14:28:50 PDT
Created attachment 546442 [details] [diff] [review]
[checked-in] silly-bubbles-2

here's a second attempt using an hbox container aligned:start.

I think this might need a little padding on the right as some of the unittests seem to have the repeat bubble crushed right against the sourcelink at the right, but need to do a little more testing.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-07-22 07:34:09 PDT
ping for review?
Comment 6 Dão Gottwald [:dao] 2011-07-23 14:33:56 PDT
Comment on attachment 546442 [details] [diff] [review]
[checked-in] silly-bubbles-2

>+    let repeatContainer = aDocument.createElementNS(XUL_NS, "xul:hbox");

s/xul:hbox/hbox/

There's a bug filed on fixing this throughout this code, but nobody's working on it. Anyway, we shouldn't add more broken code for consistency with other broken code.

>   mergeFilteredMessageNode:
>   function ConsoleUtils_mergeFilteredMessageNode(aOriginal, aFiltered) {
>-    // childNodes[3] is the node containing the number of repetitions of a node.
>-    let repeatNode = aOriginal.childNodes[3];
>+    // childNodes[3].firstChild is the node containing the number of repetitions
>+    // of a node.
>+    let repeatNode = aOriginal.childNodes[3].firstChild;
>     if (!repeatNode) {
>       return aOriginal; // no repeat node, return early.
>     }
> 
>     let occurrences = parseInt(repeatNode.getAttribute("value")) + 1;
>     repeatNode.setAttribute("value", occurrences);
>   },

Sigh. All this would be so much cleaner (e.g. aOriginal.setAttribute("repeat", parseInt(aOriginal.getAttribute("repeat")) + 1)) with the messages' internal markup hidden in a binding.
Comment 7 Rob Campbell [:rc] (:robcee) 2011-07-25 05:23:06 PDT
Thanks for the reviews, Dão.

(In reply to comment #6)
> Comment on attachment 546442 [details] [diff] [review] [review]
> silly-bubbles-2
> 
> >+    let repeatContainer = aDocument.createElementNS(XUL_NS, "xul:hbox");
> 
> s/xul:hbox/hbox/
> 
> There's a bug filed on fixing this throughout this code, but nobody's
> working on it. Anyway, we shouldn't add more broken code for consistency
> with other broken code.

ok, will do. I'll dig that bug up and take care of it.

> >   mergeFilteredMessageNode:
> >   function ConsoleUtils_mergeFilteredMessageNode(aOriginal, aFiltered) {
> >-    // childNodes[3] is the node containing the number of repetitions of a node.
> >-    let repeatNode = aOriginal.childNodes[3];
> >+    // childNodes[3].firstChild is the node containing the number of repetitions
> >+    // of a node.
> >+    let repeatNode = aOriginal.childNodes[3].firstChild;
> >     if (!repeatNode) {
> >       return aOriginal; // no repeat node, return early.
> >     }
> > 
> >     let occurrences = parseInt(repeatNode.getAttribute("value")) + 1;
> >     repeatNode.setAttribute("value", occurrences);
> >   },
> 
> Sigh. All this would be so much cleaner (e.g.
> aOriginal.setAttribute("repeat", parseInt(aOriginal.getAttribute("repeat"))
> + 1)) with the messages' internal markup hidden in a binding.

It would be. I'm not overly fond of these childNodes[n] lookups for this stuff.

I'll file a follow-up to convert our messages to XBL.
Comment 8 Rob Campbell [:rc] (:robcee) 2011-07-25 06:45:14 PDT
Comment on attachment 546442 [details] [diff] [review]
[checked-in] silly-bubbles-2

http://hg.mozilla.org/mozilla-central/rev/a6ae7b82861d

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