Closed Bug 580400 Opened 12 years ago Closed 12 years ago

Console should display dividers to separate messages

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 -, status2.0 wanted)

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: pcwalton, Assigned: pcwalton)

References

Details

(Whiteboard: [kd4b4])

Attachments

(3 files, 6 obsolete files)

Attached image 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.
Blocks: consoleui
Attachment #458838 - Flags: review?
Depends on: 578658
This has no styling yet; forthcoming.
Attachment #458838 - Flags: review? → review?(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.
Attached patch Patch, based on mozilla-central. (obsolete) — Splinter Review
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.
Attachment #458838 - Attachment is obsolete: true
Attachment #459249 - Flags: feedback?(ddahl)
Attachment #458838 - Flags: review?(ddahl)
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.
Severity: enhancement → normal
blocking2.0: --- → ?
No longer depends on: 578658
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
Attachment #459249 - Flags: feedback?(ddahl) → feedback+
Attached patch Proposed patch, version 3. (obsolete) — Splinter Review
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.
Attachment #459249 - Attachment is obsolete: true
Attachment #459602 - Flags: review?(gavin.sharp)
Nice to have, not a hard blocker, would approve a reviewed patch.
blocking2.0: ? → -
status2.0: --- → wanted
blocking2.0: - → ?
(that was an accident, resetting flag)
blocking2.0: ? → -
Attached patch Proposed patch, version 4. (obsolete) — Splinter Review
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.
Attachment #459602 - Attachment is obsolete: true
Attachment #461336 - Flags: feedback?(ddahl)
Attachment #459602 - Flags: review?(gavin.sharp)
Summary: Console should display headers to separate messages → Console should display dividers to separate messages
Attachment #461336 - Flags: feedback?(ddahl) → feedback+
Attachment #461336 - Flags: review?(dtownsend)
Note that I won't have time to review this until mid next week at the earliest.
Attachment #461336 - Flags: review?(dtownsend) → review?(gavin.sharp)
Changed review request.
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.
Whiteboard: [kd4b4]
Comment on attachment 461336 [details] [diff] [review]
Proposed patch, version 4.

Canceling review because this is incompatible with bug 583359. New patch forthcoming.
Attachment #461336 - Attachment is obsolete: true
Attachment #461336 - Flags: review?(gavin.sharp)
Attached patch Proposed patch, version 5. (obsolete) — Splinter Review
New patch surrounds chronologically-related messages in a group.
Attachment #462572 - Flags: feedback?(ddahl)
Attachment #462572 - Flags: feedback?(ddahl) → feedback+
Attachment #462572 - Flags: review?(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?
New version adds extra unified diff context.
Attachment #463400 - Flags: review?(dietrich)
Attached image Screenshot.
Screenshot attached. Note the divider in the middle.
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.
Attachment #463400 - Flags: review?(dietrich) → review-
(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
New version of the patch addresses the review comments and uses XUL separators instead of manual styles.
Attachment #462572 - Attachment is obsolete: true
Attachment #463400 - Attachment is obsolete: true
Attachment #463628 - Flags: review?(dietrich)
Attachment #462572 - Flags: review?(dietrich)
Comment on attachment 463628 [details] [diff] [review]
[checked-in] Proposed patch, version 6.

r=me, thanks!
Attachment #463628 - Flags: review?(dietrich) → review+
Attachment #463628 - Flags: approval2.0?
Attachment #463628 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Comment on attachment 463628 [details] [diff] [review]
[checked-in] Proposed patch, version 6.

http://hg.mozilla.org/mozilla-central/rev/d729c1359ae0
Attachment #463628 - Attachment description: Proposed patch, version 6. → [checked-in] Proposed patch, version 6.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.