Closed Bug 859039 Opened 7 years ago Closed 6 years ago

Allow sorting by column (status, method, file, domain, type, size etc.)

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(2 files, 7 obsolete files)

Simple ascending/descending sort by clicking on the menu headers.
Summary: [netmonitor] Allow sorting by column (method, file, domain, type, size etc.) → [netmonitor] Allow sorting by column (status, method, file, domain, type, size etc.)
Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.
Component: Developer Tools → Developer Tools: Netmonitor
Summary: [netmonitor] Allow sorting by column (status, method, file, domain, type, size etc.) → Allow sorting by column (status, method, file, domain, type, size etc.)
Duplicate of this bug: 862859
Priority: -- → P2
Attached patch v1 (obsolete) — Splinter Review
WIP.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attached patch v2 (obsolete) — Splinter Review
WIP 2.
Attachment #743665 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
WIP 3. Almost there.
Attachment #744089 - Attachment is obsolete: true
Attached patch v4 (obsolete) — Splinter Review
There. Needs a test.
Attachment #744140 - Attachment is obsolete: true
Blocks: 867945
See Also: → 859046
Attached patch v5 (obsolete) — Splinter Review
Closer. Preserving sorting across content reloads. New items are always sorted accordingly.
Attachment #744549 - Attachment is obsolete: true
Attached patch v6 (obsolete) — Splinter Review
Added tests.
Attachment #744693 - Attachment is obsolete: true
Attachment #745074 - Flags: review?(rcampbell)
Attached patch v6.1 (obsolete) — Splinter Review
Added another test because paranoia.
Attachment #745074 - Attachment is obsolete: true
Attachment #745074 - Flags: review?(rcampbell)
Attachment #745143 - Flags: review?(rcampbell)
Depends on: 859041
After your suggestion of having a play with this code, the first thing that jumps out at me is the styling. The Round label on the column headers feels a little out-of-place to me. We've gone for a fairly flat theme through-out and this breaks that. Also, using the recessed/extruded style to indicate sort order is unintuitive.

I'd prefer seeing an indicator added to the header label with a down or up arrow. Boring, I know...
(In reply to Rob Campbell [:rc] (:robcee) from comment #11)

> I'd prefer seeing an indicator added to the header label with a down or up
> arrow. Boring, I know...

I'd agree, however some labels are much to narrow for an additional arrow to fit. What do you think?
(In reply to Victor Porof [:vp] from comment #13)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #11)
> 
> > I'd prefer seeing an indicator added to the header label with a down or up
> > arrow. Boring, I know...
> 
> I'd agree, however some labels are much to narrow for an additional arrow to
> fit. What do you think?

only two of the columns (in English) look like they're too tight. Maybe... add a minimum width to them? I'm open to ideas.
(In reply to Rob Campbell [:rc] (:robcee) from comment #14)
> 
> only two of the columns (in English) look like they're too tight. Maybe...
> add a minimum width to them? I'm open to ideas.

That would make the space occupied by the columns themselves too large. You don't want useless padding around those green circles for example, do you? I don't.
(In reply to Victor Porof [:vp] from comment #15)
> 
> That would make the space occupied by the columns themselves too large. You
> don't want useless padding around those green circles for example, do you? I
> don't.

Since this patch contains string changes, I would suggest dealing with such issues during the Aurora cycle and talking with shorlander about it. I really want to land this before the uplift.
Attached patch v6.2Splinter Review
Addressed comments, removed border-radius.
Attachment #745143 - Attachment is obsolete: true
Attachment #745143 - Flags: review?(rcampbell)
Attachment #747009 - Flags: review?(rcampbell)
Comment on attachment 747009 [details] [diff] [review]
v6.2

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

this is clean. Not much to add here.

::: browser/devtools/debugger/debugger-toolbar.js
@@ +944,5 @@
>      // Hide all the groups with no visible children.
>      view.node.hideEmptyGroups();
>  
>      // Ensure the currently selected item is visible.
> +    view.node.ensureSelectionIsVisible({ withGroup: true });

ok.

::: browser/devtools/debugger/test/head.js
@@ +182,5 @@
>        dbg.panelWin.gClient.addOneTimeListener("resumed", function() {
>          info("Debugger has started");
>          dbg._view.Variables.lazyEmpty = false;
>          dbg._view.Variables.lazyAppend = false;
> +        dbg._view.Variables.lazyExpand = false;

lazy or just relaxed?

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +349,5 @@
> +   *
> +   * @param string aType
> +   *        Either "status", "method", "file", "domain", "type" or "size".
> +   */
> +  sortBy: function NVRM_sortBy(aType) {

you're adding this function name here only to take it away later. tsk.

@@ +368,5 @@
> +        target.setAttribute("tooltiptext", L10N.getStr("networkMenu.sortedAsc"));
> +      } else if (target.getAttribute("sorted") == "ascending") {
> +        target.setAttribute("sorted", direction = "descending");
> +        target.setAttribute("tooltiptext", L10N.getStr("networkMenu.sortedDesc"));
> +      } else {

// sorted == descending

@@ +384,5 @@
> +      case "status":
> +        if (direction == "ascending") {
> +          this.sortContents(this._byStatus);
> +        } else {
> +          this.sortContents((a, b) => !this._byStatus(a, b));

! a > b ==> a <= b

because math.

@@ +572,5 @@
>        if (selectedItem && selectedItem.attachment.id == id) {
>          NetMonitorView.NetworkDetails.populate(selectedItem.attachment);
>        }
>      }
> +

is that a space?

::: browser/devtools/netmonitor/test/html_sorting-test-page.html
@@ +27,5 @@
> +          get("sjs_sorting-test-server.sjs", 5, function() {
> +            get("sjs_sorting-test-server.sjs", 2, function() {
> +              get("sjs_sorting-test-server.sjs", 4, function() {
> +                get("sjs_sorting-test-server.sjs", 3, function() {
> +                  // Done.

// halp!

::: browser/devtools/shared/widgets/BreadcrumbsWidget.jsm
@@ +227,5 @@
>      this._target.appendChild(aContents);
>    },
>  
> +  window: null,
> +  document: null,

not clear why these changes are in here. Is this just opportunistic cleanup?

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +200,5 @@
>    /**
> +   * Specifies if nodes in this view may be expanded lazily.
> +   * @see Scope.prototype.expand
> +   */
> +  lazyExpand: true,

I have a pair of pants like that for wearing around the house.

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +714,5 @@
> +   * @param MenuItem aSecond
> +   *        The second menu item to be swapped.
> +   */
> +  swapItems: function MC_swapItems(aFirst, aSecond) {
> +    if (aFirst == aSecond) { // We're just dandy, thank you.

:)

@@ +746,5 @@
> +    } else if (selectedTarget == secondTarget) {
> +      selectedIndex = j;
> +    }
> +
> +    // 3. Silently nuke both items, nobody needs to know about this.

s/nuke/transmogrify/
Attachment #747009 - Flags: review?(rcampbell) → review+
Depends on: 871059
https://hg.mozilla.org/mozilla-central/rev/9a6679593bce
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Depends on: 874363
Depends on: 874419
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.