592045 - Add Live Filter Search to Panorama

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, enhancement, P1)

Description

[Aza Raskin [:aza]]

There are two types of information retrieval: (1) browsing, and (2) searching. The former is for when you know roughly for what you are looking or where to look but not exactly what you are trying to find. The later is for when you are looking for something very specific.

Panorama excels at the first mode of information retrieval but currently lacks the later. By adding a find-as-you-type interface we solve that problem.

A couple design goals are:

* Remain unobtrusive (no new info-clutter)
* Feel extremely fast
* Key-stroke level complete
* Be lenient of typos

Comment 2

[Ed Lee :Mardak]

Attachment: v1

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 471935 [details] [diff] [review]
v1


>+// ----------
>+// Function: String.score
>+// Given a pattern string, returns a score between 0 and 1 of how well
>+// that pattern matches the original string. It mimics the heuristics
>+// of the Mac application launcher Quicksilver.
>+String.prototype.score = function(abbreviation, offset) {

is this file pulled into the browser window scope? if so, just make this a regular function instead of altering String's prototype.

>+  offset = offset || 0; // TODO: I think this is unused... remove

fix the TODO

>+        var c = lowerThis.charCodeAt(index-1);
>+        if(c==32 || c == 9) {

make space-before-and-after-operators consistent throughout the patch.

>+// ##########
>+// Class: TabMatcher
>+// 
>+// A singleton class that keeps track of which tabs
>+// match and do not match a particular search.
>+var TabMatcher = {
>+  // ----------
>+  // Function: term
>+  // Returns the currently-used term. This is simply the value
>+  // of what is in the searchbox.
>+  term: function(){
>+    return iQ("#searchbox").val();
>+  },

space after (), here and elsewhere.

also, name all anonymous functions in the patch, so they are identifiable in debuggers.

>+
>+  // ----------
>+  // Function: matched
>+  // Returns the set of tabs which match the current search term.
>+  // If the term is less than 2 characters in length, it returns
>+  // nothing.
>+  matched: function(){
>+    var term = TabMatcher.term();

s/TabMatcher/this/ for clarity, here and elsewhere. (made me double-take b/c i thought i *was* in TabMatcher)

>+    if( term.length < 2 ) return [];

nit: return on next line (easy to miss same-line returns when scanning code)

>+    
>+    var tabs = TabItems.getItems();
>+    tabs = tabs.filter(function(tab){
>+      var name = tab.nameEl.innerHTML;      
>+      return name.match(term, "i");
>+    });
>+            

nit: extraneous whitespace

up to here, will review the rest tomorrow. feel free to make these changes in the meantime :)

Comment 4

[Ed Lee :Mardak]

Attachment: v1.1

Updated context

Comment 5

[Aza Raskin [:aza]]

Attachment: Version 2!

Comment 6

[Aza Raskin [:aza]]

Requested changes made. Also, thanks to Mardak and Zpao I can use mq to some degree approximating the thing that happens right after noob but before comfortable.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 472022 [details]
Version 2!

>+// ##########
>+// Class: TabMatcher
>+// 
>+// A singleton class that keeps track of which tabs
>+// match and do not match a particular search.
>+var TabMatcher = {

this doesn't actually keep track of anything, just does the search and returns the result, so the documentation needs to be updated to reflect that.

and since it doesn't actually have state of any kind, you should actually make this a proper instanciate-able object instead of a singleton, so that not only can you do your searches with it, but extensions and other internal code can as well. we could move this and the scoring code over to tabs.jsm at some point.

>+  // ----------
>+  // Function: term
>+  // Returns the currently-used term. This is simply the value
>+  // of what is in the searchbox.
>+  term: function term() {
>+    return iQ("#searchbox").val();
>+  },

i think you should pass the search term to the constructor function, instead of locking this class to that one specific UI element. that way not only can extensions use this code, but other internal code can as well

when you make that change, be sure to also document that the matching is case insensitive.

also, this could be a property instead of a method, once you've changed the code so it's passed in from the caller.

>+
>+  // ----------
>+  // Function: matched
>+  // Returns the set of tabs which match the current search term.
>+  // If the term is less than 2 characters in length, it returns
>+  // nothing.
>+  matched: function matched() {

can you document the return type? newcomers will need to know if it's a xul element or TabItem or TabViewTab. and should also do s/set/array/ if you're returning an array.

>+    var term = this.term();
>+    if (term.length < 2)
>+      return [];
>+    
>+    var tabs = TabItems.getItems();
>+    tabs = tabs.filter(function(tab){
>+      var name = tab.nameEl.innerHTML;      
>+      return name.match(term, "i");
>+    });

file a followup bug to allow searching on tab url as well?

>+    
>+    tabs.sort(function sorter(x,y){
>+      var yScore = scorePatternMatch(term, y.nameEl.innerHTML);
>+      var xScore = scorePatternMatch(term, x.nameEl.innerHTML);
>+      return yScore - xScore; 
>+    });

nit: space after comma in parameter lists. you do it elsewhere, so should here too.

>+  // ----------
>+  // Function: unmatched
>+  // Returns the inverse set of tabs that matched does.
>+  unmatched: function unmatched() {

ditto on documenting the tab return type.

>+    var term = this.term();    
>+    var tabs = TabItems.getItems();
>+    
>+    if ( term.length < 2 )
>+      return tabs;

make this check before getting the tabs, like you do in matched()

>+  // ----------
>+  // Function: doSearch
>+  // Performs the search. Lets you provide two functions, one that is called
>+  // on all matched tabs, and one that is called on all unmatched tabs.  
>+  doSearch: function(matchFunc, unmatchFunc){

should document what parameters are passed to the callback functions.

>+    var tabs = TabItems.getItems();

this is unused.

>+// ##########
>+// Class: EventHandlerClass
>+// 
>+// A singleton class that handles all of the
>+// event handlers.
>+function EventHandlerClass() { this.init(); }

this would be easier to read if on separate lines.

also, this should be named such that it's clearly related to the search functionality.

>+    Utils.log( iQ("#searchbutton") );

remove debugging code

>+    // TODO: Also include funky chars
>+    if ( !key.match(/[A-Z0-9]/) || event.altKey || event.ctrlKey || event.metaKey ) return;
>+
>+    // If we are already in an input field, allow typing as normal.
>+    if ( event.target.nodeName == "INPUT" ) return;

move the returns to the next line, so they're easily visible.

>+  // Function: inSearchKeyHandler
>+  // Handles all keypresses while search mode.
>+  inSearchKeyHandler: function (event){
>+    // Escape
>+    if (event.which == 27) hideSearch(event);
>+    if (event.which == 8 && iQ("#searchbox").val().length <= 1) hideSearch(event);
>+
>+    var matches = TabMatcher.matched();
>+    if (event.which == 13 && matches.length > 0){

here and in the rest of this code, should use the DOM event key constants, not the raw number itself. this improves code clarity and saves everyone time because they don't need to go look it up to see what's going on.

the codes are available in the link below. you can access them via Ci.nsIDOMKeyEvent.DOM_VK_*, and iirc, off the event itself (eg event.DOM_VK_* in the case of the code above).

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMKeyEvent.idl

>+
>+  // ----------
>+  // Function: switchToBeforeMode
>+  // Make sure the event handlers are appropriate for
>+  // the before-search mode. 
>+  switchToBeforeMode: function switchToBeforeMode() {
>+    var self = this;
>+    if (this.currentHandler)
>+      iQ(document).unbind("keydown", this.currentHandler);
>+    this.currentHandler = function(event){
>+      self.beforeSearchKeyHandler(event);
>+    };

you could switch to:

this.currentHandler = this.beforeSearchKeyHandler;

which is clearer, and removes the need for |self|.

>+  // ----------
>+  // Function: switchToInMode
>+  // Make sure the event handlers are appropriate for
>+  // the in-search mode.   
>+  switchToInMode: function switchToInMode() {
>+    var self = this;
>+    if (this.currentHandler)
>+      iQ(document).unbind("keydown", this.currentHandler);
>+    this.currentHandler = function(event){
>+      self.inSearchKeyHandler(event);
>+    };

ditto

>+var TabHandlers = {
>+  onMatch: function(tab, index){
>+   tab.setZ(1010);   

nit: remove whitespace (for whole patch)

>+  onUnmatch: function(tab, index){
>+    // TODO: Set back as value per tab.
>+    tab.setZ(500);
>+    tab.removeClass("notMainMatch");

file a followup bug if that doesn't need immediate attention

>+  if ($search.css("display") == "none"){
>+    $search.show();
>+    var mainWindow = gWindow.document.getElementById("main-window");
>+    mainWindow.setAttribute("activetitlebarcolor", "#717171");       
>+        
>+    // Marshal the focusing, otherwise you end up with
>+    // a race condition where only sometimes would the
>+    // first keystroke be registered by the search box.
>+    // When you marshal it never gets registered, so we
>+    // manually 
>+    setTimeout(function focusSearch() {

finish the

>+
>+var EventHandler = new EventHandlerClass();

rename to clearly show it's search related.

>+
>+// Features to add:
>+// (1) Add a visual affordance. I.e., a button to click on that creates the
>+//     search field. When using this, also add text that mentions that they
>+//     can just start typing.
>+// (2) Make sure this looks good on Windows.
>+// (3) Make sure that we don't put the matched tab over the search box
>+// (4) Group all of the highlighted tabs into a group?
>\ No newline at end of file

fix newline

>diff --git a/browser/themes/gnomestripe/browser/jar.mn b/browser/themes/gnomestripe/browser/jar.mn
>--- a/browser/themes/gnomestripe/browser/jar.mn
>+++ b/browser/themes/gnomestripe/browser/jar.mn
>@@ -81,6 +81,7 @@
>   skin/classic/browser/tabview/tabview.css            (tabview/tabview.css)
>   skin/classic/browser/tabview/stack-expander.png     (tabview/stack-expander.png)
>   skin/classic/browser/tabview/tabview.png            (tabview/tabview.png)
>+  skin/classic/browser/tabview/search.png                     (tabview/search.png) 

fix indent

the code looks mostly good, just need the above fixes. r- only for the lack of tests. add an automated test and the changes above, and this'll be solid.

Comment 8

[The 8472]

I suggest that the same mechanism as for switchTo: in the awesomebar should be used to provide a consistent user experience. Both serve the purpose of locating a tab or a set of tabs. It would feel odd if one of them would find something that the other doesn't and vice versa. If necessary switchTo: would have to be extended too in my opinion.

At least tabcandy search should provide a strict superset of the results found by switchTo:, which would be somewhat intuitive considering that it spans multiple groups and windows.

Comment 9

[Aza Raskin [:aza]]

Attachment: Version 3

Changes made.

One comment:

> make this check before getting the tabs, like you do in matched()
Can't make this change because when two or fewer characters have been typed then all of the tabs are by definition not going to match. Thus, we always need to get the list of tabs.

Comment 10

[Raymond Lee [:raymondlee]]

Comment on attachment 472575 [details]
Version 3

I am writing a test for this so please wait for my test before reviewing the patch.  Thanks

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 472575 [details]
Version 3

per raymond, holding off for tests.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: V3 with tests

With tests

Comment 13

[Ian Gilman [:iangilman]]

Comment on attachment 472583 [details] [diff] [review]
V3 with tests

* By setting the z of a TabItem (on match/unmatch) to an absolute value, we're probably setting ourselves up for some z order issues; drag increments z order, so eventually things may get higher than the absolute value. If nothing else, should set the TabItem's z back to where it started, rather than just "500". We can land without this however; please file a follow-up bug. 

* <div id="universalGeometryFrame" style="border: 2px solid red; z-index: -9999; display:none;"></div> got accidentally added to this diff; it has nothing to do with search... please remove it. 

* In the test, we're searching for tabs, but the test hasn't done anything to guarantee that there are tabs; shouldn't we create a couple of tabs for that purpose?

f+ with those two changes and one follow up bug. Raymond, can you take care of all of these and then mark for review for dietrich?

Comment 14

[(no longer active)]

Comment on attachment 472583 [details] [diff] [review]
V3 with tests

I pushed this patch to the try server, and the result was a unanimous success!

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: v4

Fixed the two things Ian mentioned and filed a Bug 593902

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 472915 [details] [diff] [review]
v4

Just stumbling upon the license header here, asking Gerv to comment on that one.

Unrelated, what's the impact of bug 593904?

Comment 17

[Gervase Markham [:gerv]]

Comment on attachment 472915 [details] [diff] [review]
v4

Can you please not put the additional stuff within the tri-license license block, but instead add it in a separate block comment underneath?

Putting it where it is now will either break automated tools, or the tools will break the licence, neither of which are desirable.

Thanks :-)

Gerv

Comment 18

[Raymond Lee [:raymondlee]]

Attachment: v5

Addressed the point in comment #17

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 473070 [details] [diff] [review]
v5


>+// Features to add:
>+// (1) Make sure this looks good on Windows.
>+// (2) Make sure that we don't put the matched tab over the search box
>+// (3) Group all of the highlighted tabs into a group?

file bugs, note them in these comments.

>diff --git a/browser/base/content/tabview/tabview.css b/browser/base/content/tabview/tabview.css
>--- a/browser/base/content/tabview/tabview.css
>+++ b/browser/base/content/tabview/tabview.css

>\ No newline at end of file

add newline please

>diff --git a/browser/themes/gnomestripe/browser/tabview/tabview.css b/browser/themes/gnomestripe/browser/tabview/tabview.css
>--- a/browser/themes/gnomestripe/browser/tabview/tabview.css
>+++ b/browser/themes/gnomestripe/browser/tabview/tabview.css

>\ No newline at end of file

ditto

>diff --git a/browser/themes/pinstripe/browser/tabview/tabview.css b/browser/themes/pinstripe/browser/tabview/tabview.css
>--- a/browser/themes/pinstripe/browser/tabview/tabview.css
>+++ b/browser/themes/pinstripe/browser/tabview/tabview.css

>\ No newline at end of file

ditto

>diff --git a/browser/themes/winstripe/browser/tabview/tabview.css b/browser/themes/winstripe/browser/tabview/tabview.css
>--- a/browser/themes/winstripe/browser/tabview/tabview.css
>+++ b/browser/themes/winstripe/browser/tabview/tabview.css

>\ No newline at end of file

ditto

r=me with these fixed.

Comment 20

[Raymond Lee [:raymondlee]]

Attachment: v5

Comment 21

[Raymond Lee [:raymondlee]]

Comment on attachment 473136 [details] [diff] [review]
v5

r+ from dietrich

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 473136 [details] [diff] [review]
v5

a=beltzner

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 24

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/264f954e402d

Comment 25

[Sylvain Pasche]

>diff --git a/browser/themes/winstripe/browser/jar.mn b/browser/themes/winstripe/browser/jar.mn
>--- a/browser/themes/winstripe/browser/jar.mn
>+++ b/browser/themes/winstripe/browser/jar.mn
>@@ -96,16 +96,17 @@ browser.jar:
>         skin/classic/browser/tabbrowser/tab-arrow-left.png                      (tabbrowser/tab-arrow-left.png)
>         skin/classic/browser/tabbrowser/tabDragIndicator.png                    (tabbrowser/tabDragIndicator.png)
>         skin/classic/browser/tabview/edit-light.png                 (tabview/edit-light.png)
>         skin/classic/browser/tabview/edit.png                       (tabview/edit.png)
>         skin/classic/browser/tabview/new-tab.png                    (tabview/new-tab.png)
>         skin/classic/browser/tabview/tabview.css                    (tabview/tabview.css)
>         skin/classic/browser/tabview/stack-expander.png             (tabview/stack-expander.png)
>         skin/classic/browser/tabview/tabview.png                    (tabview/tabview.png)
>+        skin/classic/browser/tabview/search.png                     (tabview/search.png)

It should also be added to the aero section.

Comment 26

[Tracy Walker [:tracy]]

definitely want a test case in Litmus for tab searching. Flagging it to pick up during Bug Week (9-13 to 9-17)

Comment 27

[Tracy Walker [:tracy]]

Verified with nightly builds

Comment 28

[Tracy Walker [:tracy]]

As part of Bug Week, thanks to kbrosnan for the new test case in Litmus.

Litmus ID 12845