Add Live Filter Search to Panorama

VERIFIED FIXED in Firefox 4.0b7

Status

enhancement
P1
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: aza, Assigned: aza)

Tracking

Trunk
Firefox 4.0b7
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [in-litmus-bug-week])

Attachments

(1 attachment, 8 obsolete attachments)

Assignee

Description

9 years ago
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
Assignee

Updated

9 years ago
Assignee: nobody → aza
Duplicate of this bug: 592015

Comment 2

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Attachment #471935 - Flags: review?(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

9 years ago
Posted patch v1.1 (obsolete) — Splinter Review
Updated context
Attachment #471992 - Flags: review?(dietrich)

Updated

9 years ago
Attachment #471935 - Attachment is obsolete: true
Attachment #471935 - Flags: review?(dietrich)
Assignee

Comment 5

9 years ago
Posted file Version 2! (obsolete) —
Attachment #472022 - Flags: review?(dietrich)
Assignee

Updated

9 years ago
Attachment #471992 - Flags: review?(dietrich)
Assignee

Comment 6

9 years ago
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.
Attachment #472022 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #472022 - Flags: review?(dietrich) → review-
Target Milestone: --- → Firefox 4.0b6
Severity: normal → enhancement
Priority: -- → P1

Comment 8

9 years ago
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.
Assignee

Comment 9

9 years ago
Posted file Version 3 (obsolete) —
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.
Attachment #471992 - Attachment is obsolete: true
Attachment #472022 - Attachment is obsolete: true
Attachment #472575 - Flags: review?(dietrich)
Attachment #472575 - Flags: feedback?(ian)
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 on attachment 472575 [details]
Version 3

per raymond, holding off for tests.
Attachment #472575 - Flags: review?(dietrich)
Posted patch V3 with tests (obsolete) — Splinter Review
With tests
Attachment #472575 - Attachment is obsolete: true
Attachment #472583 - Flags: feedback?(ian)
Attachment #472575 - Flags: feedback?(ian)
Blocks: 594094
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?
Attachment #472583 - Flags: feedback?(ian) → feedback+

Comment 14

9 years ago
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!
Posted patch v4 (obsolete) — Splinter Review
Fixed the two things Ian mentioned and filed a Bug 593902
Attachment #472583 - Attachment is obsolete: true
Attachment #472915 - Flags: review?(dietrich)
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?
Attachment #472915 - Flags: feedback?(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
Blocks: 593902
Blocks: 593904
Posted patch v5 (obsolete) — Splinter Review
Addressed the point in comment #17
Attachment #473070 - Flags: review?(dietrich)
Attachment #472915 - Attachment is obsolete: true
Attachment #472915 - Flags: review?(dietrich)
Attachment #472915 - Flags: feedback?(gerv)
Blocks: 593905
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.
Attachment #473070 - Flags: review?(dietrich) → review+
Blocks: 594429
Blocks: 594433
Blocks: 594434
Posted patch v5 (obsolete) — Splinter Review
Attachment #473070 - Attachment is obsolete: true
Attachment #473136 - Flags: approval2.0?
Comment on attachment 473136 [details] [diff] [review]
v5

r+ from dietrich
Comment on attachment 473136 [details] [diff] [review]
v5

a=beltzner
Attachment #473136 - Flags: approval2.0? → approval2.0+
Attachment #473136 - Attachment is obsolete: true

Comment 24

9 years ago
http://hg.mozilla.org/mozilla-central/rev/264f954e402d
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 25

9 years ago
>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.

Updated

9 years ago
Depends on: 595533
definitely want a test case in Litmus for tab searching. Flagging it to pick up during Bug Week (9-13 to 9-17)
Verified with nightly builds
Status: RESOLVED → VERIFIED
Flags: in-litmus?
As part of Bug Week, thanks to kbrosnan for the new test case in Litmus.

Litmus ID 12845
Flags: in-litmus? → in-litmus+
Whiteboard: [in-litmus-bug-week]

Updated

9 years ago
No longer depends on: 595533
Duplicate of this bug: 587334
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.