580400 - Console should display dividers to separate messages

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Walton (:pcwalton)]

Attachment: Mockup.

The console should display headers to separate messages spaced a certain interval apart, as shown in the attached UI mockup. This provides a visual clue as to which messages are likely logically associated with the same user action.

Comment 1

[Patrick Walton (:pcwalton)]

Attachment: Patch. Requires patch in bug 578658.

Comment 2

[Patrick Walton (:pcwalton)]

This has no styling yet; forthcoming.

Comment 3

[David Dahl :ddahl]

For some reason, the patch this depends on was not in anyone's review queue. In the future, it is best not to make your patch depend on a patch that  is not reviewed. I have asked Mihai to break his patch up into logical units as it is a bit large (although good work), and it is a refactoring I was not anticipating working on until later. We have many more pressing bugs to work on.

Comment 4

[Patrick Walton (:pcwalton)]

Attachment: Patch, based on mozilla-central.

This patch is no longer based on any unreviewed code. It will conflict with bug 580820; I will rebase whichever patch is reviewed later on the one that is reviewed first.

Comment 5

[Patrick Walton (:pcwalton)]

This patch is part of the Firefox 4 Console UI. The current UI for the console
is simply a placeholder and has not yet been designed from a UX perspective
(see bug 559481). Because I am requesting blocking status for the Console UI,
and this is a critical part of the Console user experience, I am requesting
that this bug block the Firefox 4 release as well.

Comment 6

[David Dahl :ddahl]

Comment on attachment 459249 [details] [diff] [review]
Patch, based on mozilla-central.


>+XPCOMUtils.defineLazyServiceGetter(this, "scriptableDateFormat",
>+                                   "@mozilla.org/intl/scriptabledateformat;1",
>+                                   "nsIScriptableDateFormat");
>+
wowee! scriptableDatFormat

>+// The amount of time in milliseconds that must pass between messages to
>+// trigger the display of a new header.
>+const NEW_HEADER_DELAY = 5000;
sounds like a good default. again - something to perhaps make configurable once we work out a prefs pane or something like that.

>   /**
>+   * Builds and appends a header to the console if enough time has passed since
>+   * the last message.
>+   *
>+   * @param IDOMNode aConsoleNode
s/IDOMNode/nsIDOMNode/

>+  appendHeaderIfNecessary:
>+  function HS_appendHeaderIfNecessary(aConsoleNode, aTimestamp)
>+  {
>+    let hudBox = aConsoleNode;
some might suggest you not create the local hudBox

>+    while (hudBox != null && hudBox.getAttribute("class") !== "hud-box")
>+      hudBox = hudBox.parentNode;
>+    if (hudBox == null)
>+      throw new Error(ERRORS.CONSOLE_NODE_NOT_IN_HUD_BOX);
nit: please use braces with all ifs. 

>+
>+    let delta = aTimestamp - hudBox.lastTimestamp;
>+    hudBox.lastTimestamp = aTimestamp;
>+    if (delta < NEW_HEADER_DELAY)
>+      return;   // No header needed.
nit: more braces, just like "more cowbell", though less funny.

I am sorry if you have to rebase this again, but I have basically r+'d all of mihai's patches.

r+ with changes / possible re-base

Comment 7

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 3.

The "hudBox" variable is actually needed, since the original value of "aConsoleNode" is used later (line 1251). Other than that, this patch addresses the issues raised.

Comment 8

[Johnathan Nightingale [:johnath]]

Nice to have, not a hard blocker, would approve a reviewed patch.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

(that was an accident, resetting flag)

Comment 10

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 4.

This new version of the patch replaces headers with dividers that don't contain timestamps. This is in response to Aza's UI feedback that information shouldn't be in more than one place. Since we have full timestamps on each message, we don't need to include that information in the header as well. However, it's still useful to the developer to separate logically distinct groups of messages.

This patch also adds unit tests.

Comment 11

[Dave Townsend [:mossop]]

Note that I won't have time to review this until mid next week at the earliest.

Comment 12

[Patrick Walton (:pcwalton)]

Changed review request.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

We need to do this better, but I don't think Gavin will be able to get to it any quicker. Might want to poke someone on IRC and see if they have time; Vlad, Dolske, Dao are all other possibles.

Comment 14

[Patrick Walton (:pcwalton)]

Comment on attachment 461336 [details] [diff] [review]
Proposed patch, version 4.

Canceling review because this is incompatible with bug 583359. New patch forthcoming.

Comment 15

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 5.

New patch surrounds chronologically-related messages in a group.

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 462572 [details] [diff] [review]
Proposed patch, version 5.

Can you please expand diff context to 8 and attach a screenshot of how it looks with the patch?

Comment 17

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 5 (with extra unified diff context).

New version adds extra unified diff context.

Comment 18

[Patrick Walton (:pcwalton)]

Attachment: Screenshot.

Screenshot attached. Note the divider in the middle.

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 463400 [details] [diff] [review]
Proposed patch, version 5 (with extra unified diff context).


>+    let lastGroup = this.appendGroupIfNecessary(aConsoleNode,
>+      aMessage.timestamp);

line up params, or if that's over 80 chars, wrap after the equals.

>     if (aFilterString) {
>       var filtered = this.filterLogMessage(aFilterString, aMessageNode);
>       if (filtered) {
>         // we have successfully filtered a message, we need to log it
>-        aConsoleNode.appendChild(aMessageNode);
>+        lastGroup.appendChild(aMessageNode);
>         aMessageNode.scrollIntoView(false);
>       }

in the rename you lost the self-documenting bit: "Node". can you add it back in?

>+  appendGroupIfNecessary:
>+  function HS_appendGroupIfNecessary(aConsoleNode, aTimestamp)
>+  {
>+    let hudBox = aConsoleNode;
>+    while (hudBox != null && hudBox.getAttribute("class") !== "hud-box") {
>+      hudBox = hudBox.parentNode;
>+    }
>+    if (hudBox == null) {
>+      throw new Error(ERRORS.CONSOLE_NODE_NOT_IN_HUD_BOX);
>+    }

how could that come about?

>   writeOutput: function JST_writeOutput(aOutputMessage, aIsInput)
>   {
>+    let lastGroup = HUDService.appendGroupIfNecessary(this.outputNode,
>+      Date.now());

ditto on lining up the params.

>+.hud-outer-group:not(:first-child) {
>+    background-image: -moz-linear-gradient(left, #ffffff, #c0c0c0, #ffffff);
>+    padding-top: 1px;
>+    margin-top: 12px;
>+}

a couple of comments about the separator:

1. i think it's way too subtle. needs to stand out more for the eye to pick out the groupings easily.

2. if you use a xul separator instead of your custom one, it'll be properly themed across all platforms.

Comment 20

[Dietrich Ayala (:dietrich)]

(In reply to comment #17)
> New version adds extra unified diff context.

Should probably just set your diff command to do 8 lines by default:

https://developer.mozilla.org/en/creating_a_patch

my [diff] in .hgrc looks like:

[diff]
git = 1
showfunc = 1
unified = 8

Comment 21

[Patrick Walton (:pcwalton)]

Attachment: [checked-in] Proposed patch, version 6.

New version of the patch addresses the review comments and uses XUL separators instead of manual styles.

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 463628 [details] [diff] [review]
[checked-in] Proposed patch, version 6.

r=me, thanks!

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 463628 [details] [diff] [review]
[checked-in] Proposed patch, version 6.

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