Bug 19509 - Database Tables in the Inspector should be sortable
Summary: Database Tables in the Inspector should be sortable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Jessie Berlin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-11 19:49 PDT by Keishi Hattori
Modified: 2010-06-18 20:35 PDT (History)
3 users (show)

See Also:


Attachments
Adds the ability to sort a Database Table in the Web Inspector. (2.92 KB, patch)
2010-05-21 21:08 PDT, Jessie Berlin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2008-06-11 19:49:51 PDT
Database tables in the Inspector should be sortable by clicking on the headers.
Comment 1 Tom 2009-08-17 05:13:03 PDT
It would also be helpful if you could adjust the widths of individual columns, currently you have to adjust the width of the entire window. 

Any news on if this is being looked at?
Comment 2 Jessie Berlin 2010-05-21 21:08:00 PDT
Created attachment 56774 [details]
Adds the ability to sort a Database Table in the Web Inspector.

It is already possible to resize the columns. This patch just adds the ability to sort the table based on the contents of the selected column. Works on both the tables in the Table View and the tables that show up in the Query View.
Comment 3 Darin Adler 2010-06-13 20:53:15 PDT
Comment on attachment 56774 [details]
Adds the ability to sort a Database Table in the Web Inspector.

> +            var comparison;
> +            if (isNaN(item1) || isNaN(item2))
> +                comparison = item1 < item2 ? -1 : (item1 > item2 ? 1 : 0);
> +            else {
> +                // Don't sort numbers alphanumerically.
> +                var number1 = parseFloat(item1);
> +                var number2 = parseFloat(item2);
> +                comparison = number1 < number2 ? -1 : (number1 > number2 ? 1 : 0);
> +            }

I think the comments here are a little strange and the code a bit oblique. I presume the idea here is that if the values are both numeric, we want to compare them as numbers. But I don't see how isNaN accomplishes this. What does that function do exactly? Does it look only at the numeric prefix of a number or will it return false for something like "1a". Also, comparing some pairs with one sort function and other pairs with a different one is going to create trouble for the sort algorithm. Fortunately we fixed this a while back so it couldn't cause a crash in JavaScriptCore by using a tree sort rather than something like qsort that depends on consistent results from sort function.

If you wanted the code to behavior like this I think you'd need to explain why isNaN works to check if things are numeric. Even though NaN is "not a number", I think the usage here is unclear.

But I think there are some possibly better options:

   1) Distinguish numeric columns and non-numeric columns and chose the sort function based on the column type.

   2) Write a comparison function that works like kCFCompareNumerically, although I'm not sure that will give results that are good for columns of floating point numbers.

   3) Scan the column and sort numerically only if every item in the column s a number.

Maybe you can think of something else that will work even better. Also, I would say "Sort numbers based on comparing their values rather than a lexicographical comparison". I think the term "alphanumerically" doesn't describe lexicographical comparison.

r=me as is, but I think the comparison function can be improved
Comment 4 Jessie Berlin 2010-06-18 20:35:02 PDT
(In reply to comment #3)
> (From update of attachment 56774 [details])
> > +            var comparison;
> > +            if (isNaN(item1) || isNaN(item2))
> > +                comparison = item1 < item2 ? -1 : (item1 > item2 ? 1 : 0);
> > +            else {
> > +                // Don't sort numbers alphanumerically.
> > +                var number1 = parseFloat(item1);
> > +                var number2 = parseFloat(item2);
> > +                comparison = number1 < number2 ? -1 : (number1 > number2 ? 1 : 0);
> > +            }
> 
> I think the comments here are a little strange and the code a bit oblique. I presume the idea here is that if the values are both numeric, we want to compare them as numbers. But I don't see how isNaN accomplishes this. What does that function do exactly? Does it look only at the numeric prefix of a number or will it return false for something like "1a". Also, comparing some pairs with one sort function and other pairs with a different one is going to create trouble for the sort algorithm. Fortunately we fixed this a while back so it couldn't cause a crash in JavaScriptCore by using a tree sort rather than something like qsort that depends on consistent results from sort function.
> 
> If you wanted the code to behavior like this I think you'd need to explain why isNaN works to check if things are numeric. Even though NaN is "not a number", I think the usage here is unclear.

I am now using isNaN with the result from Number(item).

> 
> But I think there are some possibly better options:
> 
>    1) Distinguish numeric columns and non-numeric columns and chose the sort function based on the column type.
> 
>    2) Write a comparison function that works like kCFCompareNumerically, although I'm not sure that will give results that are good for columns of floating point numbers.
> 
>    3) Scan the column and sort numerically only if every item in the column s a number.

I went with this option.

> 
> Maybe you can think of something else that will work even better. Also, I would say "Sort numbers based on comparing their values rather than a lexicographical comparison". 

Fixed the comment.

> I think the term "alphanumerically" doesn't describe lexicographical comparison.
> 
> r=me as is, but I think the comparison function can be improved

Thanks for the review! Committed in r61463: http://trac.webkit.org/changeset/61463