Closed Bug 419403 Opened 13 years ago Closed 13 years ago

Pimp My Download Manager Search! (multi word, match any text)

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta4

People

(Reporter: Mardak, Assigned: Mardak)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Attached image screenshot of v1
Instead of only matching the search phrase against the download file name or download source/referrer, we can match against any text shown to the user including the date/time (time, yesterday, weekday, month), download status (canceled, failed, etc.), or file size.

Additionally, instead of matching just the phrase, we can do multi-word search that matches against the site and name at the same time. E.g., "mozilla.org filename".

Attached is a screenshot of the multi-word search matching the name, completed file size, completed time, and referrer eTLD+1.
Attachment #305459 - Flags: ui-review?(madhava)
Attached patch v1 (obsolete) — Splinter Review
This also fixes bug 392536 instead of always removing a just-finished download if we're searching.

sdwilsh: :( No more createFunction.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #305460 - Flags: review?(sdwilsh)
Attached patch v1.1 (obsolete) — Splinter Review
Whoops.. would help to save the file before qrefreshing ;)

I suppose shawn will want a testcase......
Attachment #305460 - Attachment is obsolete: true
Attachment #305461 - Flags: review?(sdwilsh)
Attachment #305460 - Flags: review?(sdwilsh)
Attached patch v1.2 (obsolete) — Splinter Review
Zzz.. it always takes so much longer writing the test than the code to implement the feature. :p
Attachment #305461 - Attachment is obsolete: true
Attachment #305474 - Flags: review?(sdwilsh)
Attachment #305461 - Flags: review?(sdwilsh)
Attached patch v1.3 (obsolete) — Splinter Review
Get rid of the flicker when typing a space at the end (or a space anywhere that already has a space).

Here's a build that doesn't has v1.2 (so it'll flicker when typing a space for a second word). Also, it seems like building the list when searching could go faster, so changing +.75 to +.9. The bottleneck here is adding items to the UI - not querying against the DB.

https://build.mozilla.org/tryserver-builds/2008-02-25_01:34-edward.lee@engineering.uiuc.edu-saveText.pimpDown.unescape.adaptive/

Bug 416446 - Make PluralForm more useful for extensions and delay importing strings for perf
Bug 415460 - searching in places queries does not decode urls
Bug 418961 - "Save Page As" "Text Files" saves file but Downloads window doesn't show completion
Bug 401660 - when showing autocomplete result, show tags that partially match
Bug 416211 - Tagged results don't show bookmark title when matching the tag
Bug 405320 - support "out of order" multiple word tag search in the url bar and in the places organizer
Bug 418920 - Allow multiple word search with tags in addition to title and url
Bug 419068 - Allow tagged pages to mingle with non-tagged autocomplete results
Bug 407946 - emphasize all matching text in title and url, not just the first match in title and url
Bug 415403 - Show matches for all search words for location bar autocomplete
Bug 395735 - Instrument the location bar auto-complete to report accuracy
Bug 395739 - adaptive learning (match entered text to selected item) in url bar autocomplete
Bug 392143 - show keywords as url bar autocomplete choices
Bug 407204 - adjust the title and url text sizes
Bug 406257 - reduce the number of rows in url bar autocomplete from 10 to 6
Bug 419324 - Edit bookmarks dialog doesn't unescape URLs
Bug 419403 - Pimp My Download Manager Search! (multi word, match any text)
Attachment #305474 - Attachment is obsolete: true
Attachment #305552 - Flags: review?(sdwilsh)
Attachment #305474 - Flags: review?(sdwilsh)
Comment on attachment 305552 [details] [diff] [review]
v1.3

>+  // Don't rebuild the download list if the search didn't change unless forced
>+  if (gSearchTerms.join(" ") == prevSearch && !aForceBuild)
check !aForceBuild first - it's less expensive

>+    let isActive = gStmt.getInt32(10);
ugh - we so need to start using named parameters soon.

>+ * @param aItem
>+ *        Download richlist item to check if it matches the current search
nit: richlistitem

>+  // Search through these download attributes that are shown to the user and
s/these/the/

>+  // make it into one big string for easy combined searching
>+  let combinedSearch = "";
>+  for each (let attr in ["target", "status", "dateTime"])
let's make this array a const (perhaps up top?) so it's easy to modify this behavior down the line without ruining much blame (up top makes it easier to modify too).

>+  browser_pimped_search.js \
let's call it something a bit more accurate - "browser_multiword_search.js"

r=sdwilsh
Attachment #305552 - Flags: review?(sdwilsh) → review+
Attached patch v1.4Splinter Review
(In reply to comment #5)
> (From update of attachment 305552 [details] [diff] [review])
> r=sdwilsh
> >+  // Don't rebuild the download list if the search didn't change unless forced
> >+  if (gSearchTerms.join(" ") == prevSearch && !aForceBuild)
> check !aForceBuild first - it's less expensive
I had that before, but I was too lazy to change the comment ;) Fixed.

> >+ * @param aItem
> >+ *        Download richlist item to check if it matches the current search
> nit: richlistitem
De-spaced.

> >+  // Search through these download attributes that are shown to the user and
> s/these/the/
Articleized.

> >+  // make it into one big string for easy combined searching
> >+  let combinedSearch = "";
> >+  for each (let attr in ["target", "status", "dateTime"])
> let's make this array a const (perhaps up top?) so it's easy to modify this
> behavior down the line without ruining much blame (up top makes it easier to
> modify too).
Sure. Put up top with one item per line.

> >+  browser_pimped_search.js \
> let's call it something a bit more accurate - "browser_multiword_search.js"
Fine fine. ;)
Attachment #305552 - Attachment is obsolete: true
Attachment #305644 - Flags: review+
Attachment #305644 - Flags: approval1.9?
Comment on attachment 305644 [details] [diff] [review]
v1.4

a=beltzner for 1.9
Attachment #305644 - Flags: approval1.9? → approval1.9+
Attachment #305459 - Flags: ui-review?(madhava) → ui-review+
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.139; previous revision: 1.138
done
Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.16; previous revision: 1.15
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_multiword_search.js,v
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_multiword_search.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_multiword_search.js,v  <--  browser_multiword_search.js
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta4
Shawn: Is it okay to disable the test on windows for now? I'll file a bug to fix the test for windows.

ifneq ($(OS_ARCH), WINNT)
_BROWSER_FILES += ...
weird that it's only windows.  sure, r=me on that
Blocks: 419691
Flags: in-litmus?
Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008022804 Minefield/3.0b4pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022806 Minefield/3.0b4pre

-and-

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
Depends on: 500978
You need to log in before you can comment on or make changes to this bug.