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

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

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.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1059015.patch

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

https://tbpl.mozilla.org/?tree=Try&rev=6189d4b0feb6

Comment 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[Girish Sharma [:Optimizer]]

(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 :)

Comment 5

[Brian Grinstead [:bgrins]]

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

Comment 6

[Brian Grinstead [:bgrins]]

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?

Comment 7

[Girish Sharma [:Optimizer]]

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

Comment 8

[Brian Grinstead [:bgrins]]

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

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1059015.patch

https://tbpl.mozilla.org/?tree=Try&rev=feec6ff2001f

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

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

[Brian Grinstead [:bgrins]]

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

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

https://tbpl.mozilla.org/?tree=Try&rev=bb72325b9d9a

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/fx-team/rev/2f2088b0b8f7

Comment 15

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/2f2088b0b8f7