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

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

Simple ascending/descending sort by clicking on the menu headers.

Comment 1

[Victor Porof [:vporof][:vp]]

Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v1

WIP.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v2

WIP 2.

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: v3

WIP 3. Almost there.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v4

There. Needs a test.

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: v5

Closer. Preserving sorting across content reloads. New items are always sorted accordingly.

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: v6

Added tests.

Comment 9

[Victor Porof [:vporof][:vp]]

Attachment: v6.1

Added another test because paranoia.

Comment 10

[Victor Porof [:vporof][:vp]]

https://tbpl.mozilla.org/?tree=Try&rev=00c5b88f7297
https://tbpl.mozilla.org/?tree=Try&rev=32359d763d91

Comment 11

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

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

Comment 12

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

Attachment: Header Columns

Comment 13

[Victor Porof [:vporof][:vp]]

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

Comment 14

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

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

Comment 15

[Victor Porof [:vporof][:vp]]

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

Comment 16

[Victor Porof [:vporof][:vp]]

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

Comment 17

[Victor Porof [:vporof][:vp]]

Attachment: v6.2

Addressed comments, removed border-radius.

Comment 18

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

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/

Comment 19

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/9a6679593bce

Comment 20

[Joe Walker [:jwalker]]

https://hg.mozilla.org/mozilla-central/rev/9a6679593bce