Provide an option to the TableWidget to specify the first column to appear

RESOLVED FIXED in Firefox 34

Status

RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: gl, Assigned: gl)

Tracking

unspecified
Firefox 34
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
If you have an object such as { index: "(index)", 0: "0", 1: "1" }, the order of the object is not respected when iterating through the object. Numeric keys are sorted before non-numeric keys.

As a temporary solution, there should be an extra option to specify the first column for the console.table() use case. Ideally, the index column would be handled internally within the TableWidget.
(Assignee)

Updated

4 years ago
Assignee: nobody → gabriel.luong
(Assignee)

Updated

4 years ago
Summary: Allow TableWidget to specify an initial column → Allow TableWidget to specify a first column
(Assignee)

Updated

4 years ago
Summary: Allow TableWidget to specify a first column → Provide an option to the TableWidget to specify the first column to appear
(Assignee)

Comment 1

4 years ago
Created attachment 8479514 [details] [diff] [review]
1059015.patch
Attachment #8479514 - Flags: review?(bgrinstead)
(Assignee)

Updated

4 years ago
Blocks: 899753
Status: NEW → ASSIGNED
Comment on attachment 8479514 [details] [diff] [review]
1059015.patch

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

::: browser/devtools/shared/test/browser_tableWidget_basic.js
@@ +35,5 @@
>          uniqueId: "col1",
>          emptyText: "This is dummy empty text",
>          highlightUpdated: true,
>          removableColumns: true,
> +        firstColumn: "col1"

Can you make a change to this test such that it would fail without the change to TableWidget.js?  Test is still green even if I comment this line out

::: browser/devtools/shared/widgets/TableWidget.js
@@ +229,4 @@
>     * @param {array} hiddenColumns
>     *        Ids of all the columns that are hidden by default.
>     */
> +  setColumns: function(columns, sortOn = this.sortedOn, firstColumn, hiddenColumns = []) {

It seems like the arguments to this function are getting out of control.  In addition, the fact that you were able to insert one right in the middle before hiddenColumns without breaking any tests makes me think that hiddenColumns is not used anywhere.

I'd say do one of these two things:
1) Set this.firstColumn in the constructor then say firstColumn=this.firstColumn as a default value so you don't also need to pass a sortedOn whenever you want to set columns.
2) Set this.firstColumn in the constructor then remove it as an argument to this function and just use this.firstColumn directly inside the function
Attachment #8479514 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #3)

> ::: browser/devtools/shared/widgets/TableWidget.js
> @@ +229,4 @@
> >     * @param {array} hiddenColumns
> >     *        Ids of all the columns that are hidden by default.
> >     */
> > +  setColumns: function(columns, sortOn = this.sortedOn, firstColumn, hiddenColumns = []) {
> 
> It seems like the arguments to this function are getting out of control.  In
> addition, the fact that you were able to insert one right in the middle
> before hiddenColumns without breaking any tests makes me think that
> hiddenColumns is not used anywhere.

Storage Inspector is using it (now with r+ and ready to land!)

> I'd say do one of these two things:
> 1) Set this.firstColumn in the constructor then say
> firstColumn=this.firstColumn as a default value so you don't also need to
> pass a sortedOn whenever you want to set columns.
> 2) Set this.firstColumn in the constructor then remove it as an argument to
> this function and just use this.firstColumn directly inside the function

The table widget can have different set of columns so removing the firstColumn argument from here does not make sense as a new call to setColumns might have different set of columns (or a totally different column with same name, which was actually not meant to be the first column)

Do note that this is only a temporary solution to the problem that Gabriel is trying to solve. I personally do not like this first column approach :)
(In reply to Girish Sharma [:Optimizer] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> 
> > ::: browser/devtools/shared/widgets/TableWidget.js
> > @@ +229,4 @@
> > >     * @param {array} hiddenColumns
> > >     *        Ids of all the columns that are hidden by default.
> > >     */
> > > +  setColumns: function(columns, sortOn = this.sortedOn, firstColumn, hiddenColumns = []) {
> > 
> > It seems like the arguments to this function are getting out of control.  In
> > addition, the fact that you were able to insert one right in the middle
> > before hiddenColumns without breaking any tests makes me think that
> > hiddenColumns is not used anywhere.
> 
> Storage Inspector is using it (now with r+ and ready to land!)
> 
> > I'd say do one of these two things:
> > 1) Set this.firstColumn in the constructor then say
> > firstColumn=this.firstColumn as a default value so you don't also need to
> > pass a sortedOn whenever you want to set columns.
> > 2) Set this.firstColumn in the constructor then remove it as an argument to
> > this function and just use this.firstColumn directly inside the function
> 
> The table widget can have different set of columns so removing the
> firstColumn argument from here does not make sense as a new call to
> setColumns might have different set of columns (or a totally different
> column with same name, which was actually not meant to be the first column)
> 
> Do note that this is only a temporary solution to the problem that Gabriel
> is trying to solve. I personally do not like this first column approach :)

So what is the real solution - to sort the column object on keys before iterating?  That seems like it would be easier than adding firstColumn.
Or could we just special case the string "index" in the setColumns call in the manner that we are doing it with firstColumn?  So instead of it being an argument / option to constructor just set `let firstColumn = "index"` inside of setColumns?
The real issue is that the key is now part of the object. Take an object:

{
foo: {name: "blah", age: 12},
...
}

When it is fetched via RDP to frontend, it is reconstructed into an object like:

[
{ _index: "foo", name: "blah", age: 12 },
...
]

Note that the key "_index" is now part of the object.

This fails for objects like

{
 foo: {_index: 1, name: "gope"},
}

where the reconstructed object would be:

[
{_index: 1, name: "gope"},
...
]

or

[
{_index: "foo", name: "gope"},
]

depending on what is assigned later.

(Along with the fact that in an object, numeric keys automatically come before)

The solution is to have a method in TableWidget, addAll, which will take in an array of objects, or object of objects, handle figuring out the iteration index internally , without changing the object structure.

So you can finally do

tableWidget.addAll({
 "foo1": {firstName: "foo1", lastName:"bar1"},
 "foo2": {firstName: "foo2", lastName:"bar2"},
 "foo3": {firstName: "foo3", lastName:"bar3"},
})

will render

+------------+-----------------+-------------------+
|  (index)   |    firstName    |    lastName       |
+--------------------------------------------------+
|   foo1     |     foo1        |      bar1         |
+--------------------------------------------------+
|   foo2     |     foo2        |      bar2         |
+--------------------------------------------------+
|   foo3     |     foo3        |      bar3         |
+------------+-----------------+-------------------+

and likewise

tableWidget.addAll([[1,3,5],[2,4,6]])

will render

+------------+-----------------+-------------------+-------------------+
|  (index)   |       0         |        1          |        2          |
+----------------------------------------------------------------------+
|     0      |       1         |        3          |        5          |
+----------------------------------------------------------------------+
|     1      |       2         |        4          |        6          |
+----------------------------------------------------------------------+
(In reply to Girish Sharma [:Optimizer] from comment #4)
> Do note that this is only a temporary solution to the problem that Gabriel
> is trying to solve. I personally do not like this first column approach :)

So do you think we should instead be focused on solving this as mentioned in Comment 7, or proceed with the firstColumn option?
(Assignee)

Comment 9

4 years ago
Created attachment 8480029 [details] [diff] [review]
1059015.patch

https://tbpl.mozilla.org/?tree=Try&rev=feec6ff2001f
Attachment #8479514 - Attachment is obsolete: true
Attachment #8480029 - Flags: review?(bgrinstead)
(Assignee)

Comment 10

4 years ago
(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Girish Sharma [:Optimizer] from comment #4)
> > Do note that this is only a temporary solution to the problem that Gabriel
> > is trying to solve. I personally do not like this first column approach :)
> 
> So do you think we should instead be focused on solving this as mentioned in
> Comment 7, or proceed with the firstColumn option?

Considering the timeline of things, there wouldn't be enough time to implement the ideal solution and fix up console.table() in time for merge. I would prefer to ship now and iterate towards our ideal solution.
Comment on attachment 8480029 [details] [diff] [review]
1059015.patch

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

::: browser/devtools/shared/widgets/TableWidget.js
@@ +77,5 @@
>      this.setupHeadersContextMenu();
>    }
>  
>    if (initialColumns) {
> +    this.setColumns(initialColumns, uniqueId, firstColumn);

This argument has been removed from setColumns, so this either needs to be removed from this call or the argument needs to be readded
Attachment #8480029 - Flags: review?(bgrinstead)
(Assignee)

Comment 12

4 years ago
Created attachment 8480049 [details] [diff] [review]
1059015.patch

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Comment on attachment 8480029 [details] [diff] [review]
> 1059015.patch
> 
> Review of attachment 8480029 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/widgets/TableWidget.js
> @@ +77,5 @@
> >      this.setupHeadersContextMenu();
> >    }
> >  
> >    if (initialColumns) {
> > +    this.setColumns(initialColumns, uniqueId, firstColumn);
> 
> This argument has been removed from setColumns, so this either needs to be
> removed from this call or the argument needs to be readded

Got catch! Fixed.
Attachment #8480029 - Attachment is obsolete: true
Attachment #8480049 - Flags: review?(bgrinstead)
Attachment #8480049 - Flags: review?(bgrinstead) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f2088b0b8f7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Flags: qe-verify-

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.