Closed Bug 415960 Opened 16 years ago Closed 16 years ago

bookmark tags edit control should provide autocomplete

Categories

(Firefox :: Bookmarks & History, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: asouzis, Assigned: dietrich)

References

Details

(Keywords: verified1.9.1)

Attachments

(3 files, 12 obsolete files)

17.04 KB, image/png
Details
1.47 KB, patch
Details | Diff | Splinter Review
17.44 KB, patch
asaf
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008020304 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008020304 Minefield/3.0b3pre

It would be nice if autocomplete worked when editing tags in the Edit Bookmark dialog. I have a patch.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Attached patch v1 (obsolete) — Splinter Review
this is identical to the code I contributed to Tweez (https://addons.mozilla.org/en-US/firefox/addon/6353) except for some cleanup and some renaming to naively conform to Mozilla naming conversions. Tested with the Edit Bookmark and Library dialogs.
Attachment #301702 - Flags: review?
Attachment #301702 - Flags: review? → review?(dietrich)
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Hardware: Macintosh → All
I'm only using "," as the tag delimiter, what about other punctuation?

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

fantastic, thanks for patching it into Firefox! a few general fixes/comments below.

style nits:

- keep line length to 80 chars
- space after "//" in comments and before/after operators
- 2 space indent
- fix misaligned indent in a few places


>Index: src/nsTagAutoCompleteSearch.js
>===================================================================
>RCS file: src/nsTagAutoCompleteSearch.js
>diff -N src/nsTagAutoCompleteSearch.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ src/nsTagAutoCompleteSearch.js	6 Feb 2008 19:10:17 -0000
>@@ -0,0 +1,248 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ *   Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ * 
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is .
>+ *

should put something, "Tag Autocomplete" maybe?

>+
>+//copied from http://developer.mozilla.org/en/docs/How_to_implement_custom_autocomplete_search_component
>+//pretty much only startSearch was changed
>+ 

unnecessary comment, please remove

>+// Implements nsIAutoCompleteSearch
>+function TagAutoCompleteSearch() {
>+}
>+
>+TagAutoCompleteSearch.prototype = {
>+  /*
>+   * Search for a given string and notify a listener (either synchronously
>+   * or asynchronously) of the result
>+   *
>+   * @param searchString - The string to search for
>+   * @param searchParam - An extra parameter
>+   * @param previousResult - A previous result to use for faster searchinig
>+   * @param listener - A listener to notify when the search is complete
>+   */
>+  startSearch: function(searchString, searchParam, result, listener) {
>+      var searchResults = Cc["@mozilla.org/browser/tagging-service;1"].getService(Ci.nsITaggingService).allTags;

please cache the service, a la:

http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/utils.js#157

>+// Factory
>+var TagAutoCompleteSearchFactory = {
>+  singleton: null,
>+  createInstance: function (aOuter, aIID) {
>+    if (aOuter != null)
>+      throw Components.results.NS_ERROR_NO_AGGREGATION;
>+    if (this.singleton == null)
>+      this.singleton = new TagAutoCompleteSearch();
>+    return this.singleton.QueryInterface(aIID);
>+  }
>+};
>+

please use the new xpcom utils helper for this:

http://developer.mozilla.org/en/docs/XPCOMUtils.jsm

examples:

http://mxr.mozilla.org/seamonkey/search?string=XPCOMUtils.jsm&find=&findi=&filter=&tree=seamonkey
Attachment #301702 - Flags: review?(dietrich)
(In reply to comment #4)
> I'm only using "," as the tag delimiter, what about other punctuation?
> 

hrm, there's a bug for that, but i'm unable to find it.
> I'm only using "," as the tag delimiter, what about other punctuation?

We should probably also use ; for the subset of users who would rather use a semicolon.  I think those two are probably the only delimiters we want to use though.
+  startSearch: function(searchString, searchParam, result, listener) {
+      var searchResults = Cc["@mozilla.org/browser/tagging-service;1"].getService(Ci.nsITaggingService).allTags;

what's the perf of this with a thousand of tags? can't we provide some way to directly use a db query?
Attached patch v2 (obsolete) — Splinter Review
changes: 
* addressed review comments
* use ; as delimiter
* addressed "thousands of tags" performance concern by making search asynchronous (100 loops per yield) (i don't have thousands of tags but i tested with 2 loops per yield)
Attachment #301702 - Attachment is obsolete: true
Attachment #302119 - Flags: review?
Attachment #302119 - Flags: review? → review?(dietrich)
i don't have read it all, but if startSearch is called every time the string changes, will not this.tagging.allTags get and sort all tags at every change? That was my complain about one thousand tags, certainly the backend does not help (it should provide some way to directly query the tags with a LIKE). if that's not a problem, nevermind me.
(In reply to comment #10)
> i don't have read it all, but if startSearch is called every time the string
> changes, will not this.tagging.allTags get and sort all tags at every change?
> That was my complain about one thousand tags, certainly the backend does not
> help (it should provide some way to directly query the tags with a LIKE). if
> that's not a problem, nevermind me.
> 

The tagging service keeps a live-updating result node for the tag root, so at least .allTags is not going to the db every time (as your approach would). So while this could be faster if done locally using the same approach, lets not pre-optimize just to avoid the hit of xpconnect traversal (yet!).
Is this work here the replacement for bug 387485? Or do the bugs complement each other? I thought there were plans to build on the work Flock did on tag editing and relicensed for Mozilla in January.
(In reply to comment #13)
> Is this work here the replacement for bug 387485? Or do the bugs complement
> each other? I thought there were plans to build on the work Flock did on tag
> editing and relicensed for Mozilla in January.
> 

This bug is specific to tag autocomplete, which is quite dependent on the back-end implementation of tagging.

We may be able to use the editor widget from Flock, but I haven't had a chance to look. Bug 387485 is not blocking, so blockers have been taking priority. I'd really like to see someone step up and integrate the Flock tag editor.
Comment on attachment 302119 [details] [diff] [review]
v2


>Index: src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/src/Makefile.in,v
>retrieving revision 1.25
>diff -u -8 -p -r1.25 Makefile.in
>--- src/Makefile.in	27 Nov 2007 00:08:50 -0000	1.25
>+++ src/Makefile.in	8 Feb 2008 14:39:40 -0000
>@@ -61,13 +61,16 @@ REQUIRES	= \
> 		  htmlparser \
> 		  content \
> 		  places \
> 		  microsummaries \
> 		  $(NULL)
> 
> CPPSRCS = nsPlacesImportExportService.cpp
> 
>-EXTRA_COMPONENTS = nsPlacesTransactionsService.js
>+EXTRA_COMPONENTS = \
>+    nsPlacesTransactionsService.js \
>+    nsTagAutoCompleteSearch.js \
>+    $(NULL)    
> 
> include $(topsrcdir)/config/rules.mk
> 
> XPIDL_FLAGS += -I$(topsrcdir)/browser/components

please move this to toolkit. there's really nothing browser-specific about it, and any toolkit consumer using Places could benefit from this.

>Index: src/nsTagAutoCompleteSearch.js
>===================================================================
>RCS file: src/nsTagAutoCompleteSearch.js
>diff -N src/nsTagAutoCompleteSearch.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ src/nsTagAutoCompleteSearch.js	8 Feb 2008 14:39:40 -0000
>@@ -0,0 +1,261 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ *   Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ * 
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Tag AutocompleteSearch.

did you mean to put a space before "Search"?

>+ *
>+ * The Initial Developer of the Original Code is
>+ * Adam Souzis <adam@souzis.com>
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ * 
>+ * ***** END LICENSE BLOCK ***** */
>+ 
>+const Ci = Components.interfaces;
>+const Cc = Components.classes;
>+const Cr = Components.results;
>+const Cu = Components.utils;
>+
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+// Implements nsIAutoCompleteResult
>+function TagAutoCompleteResult(searchString, searchResult,
>+                                  defaultIndex, errorDescription,
>+                                  results, comments) {

fix indent

>+  this._searchString = searchString;
>+  this._searchResult = searchResult;
>+  this._defaultIndex = defaultIndex;
>+  this._errorDescription = errorDescription;
>+  this._results = results;
>+  this._comments = comments;
>+}
>+
>+TagAutoCompleteResult.prototype = {
>+  _searchString: "",
>+  _searchResult: 0,
>+  _defaultIndex: 0,
>+  _errorDescription: "",
>+  _results: [],
>+  _comments: [],

this doesn't seem necessary since they'll always be overwritten in the constructor.

>+// Implements nsIAutoCompleteSearch
>+function TagAutoCompleteSearch() {
>+}
>+
>+TagAutoCompleteSearch.prototype = {
>+  _stopped : false, 
>+  
>+  get tagging() {
>+    delete this._tagging;
>+    return this._tagging = Cc["@mozilla.org/browser/tagging-service;1"].
>+                          getService(Ci.nsITaggingService);

fix indent

>+  },
>+
>+  get wm() {
>+    delete this._wm;
>+    return this._wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                   .getService(Components.interfaces.nsIWindowMediator);

period at end of line, fix indent

>+  },
>+
>+  /*
>+   * Search for a given string and notify a listener (either synchronously
>+   * or asynchronously) of the result
>+   *
>+   * @param searchString - The string to search for
>+   * @param searchParam - An extra parameter
>+   * @param previousResult - A previous result to use for faster searchinig

typo: searching

>+   * @param listener - A listener to notify when the search is complete
>+   */
>+  startSearch: function(searchString, searchParam, result, listener) {
>+    var searchResults = this.tagging.allTags;
>+    var results = [];
>+    var comments = [];
>+    this._stopped = false;
>+
>+    // only search on characters for the last tag
>+    var index = Math.max(searchString.lastIndexOf(","), 
>+      searchString.lastIndexOf(";"));
>+    var before = ''; 
>+    if (index > -1) {

index != -1 is sufficient (it'll never be less than -1)

>+       before = searchString.slice(0, index+1);
>+       searchString = searchString.slice(index+1);
>+       // skip past whitespace
>+       var m = searchString.match(/\s+/);
>+       if (m) {
>+          before += m[0];
>+          searchString = searchString.slice(m[0].length);
>+       }
>+    }
>+
>+    var self = this;
>+    function doSearch() {
>+      var i = 0;
>+      while (i<searchResults.length) {

nit: space before & after <

>+        if (self._stopped)
>+          yield false;
>+        // for each match, prepend what the user has typed so far 
>+        if (searchResults[i].indexOf(searchString) == 0) {
>+          results.push(before + searchResults[i]);
>+          comments.push(null);
>+        }
>+    
>+        ++i;
>+        //100 loops per yield
>+        if ((i % 100) == 0) {
>+          var newResult = new TagAutoCompleteResult(searchString,
>+            Ci.nsIAutoCompleteResult.RESULT_SUCCESS, 0, "", results, comments);
>+          listener.onSearchResult(self, newResult);
>+            
>+          yield true;
>+        } 
>+      }
>+
>+      var newResult = new TagAutoCompleteResult(searchString,
>+        Ci.nsIAutoCompleteResult.RESULT_SUCCESS, 0, "", results, comments);
>+      listener.onSearchResult(self, newResult);
>+      yield false;
>+    }
>+    
>+    var gen = doSearch();
>+    function driveGenerator() {
>+      if (gen.next()) {
>+        self.wm.getMostRecentWindow("navigator:browser").setTimeout(driveGenerator, 0);	
>+      } else {
>+        gen.close();	
>+      }

should use nsITimer to make this not browser specific.

nit: style in Places JS is no brackets needed unless it's a multiline block.

>+    }
>+    driveGenerator();
>+  },
>+
>+  /*
>+   * Stop an asynchronous search that is in progress
>+   */
>+  stopSearch: function() {
>+    this._stopped = true;
>+  },
>+
>+  // nsISupports
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAutoCompleteSearch]), 
>+
>+  classDescription: "Tag AutoComplete",
>+  contractID: "@mozilla.org/autocomplete/search;1?name=tag-autocomplete",
>+  classID: Components.ID("{1dcc23b0-d4cb-11dc-9ad6-479d56d89593}"),
>+  serviceURL: ""
>+};
>+
>+var component = [TagAutoCompleteSearch];
>+function NSGetModule(compMgr, fileSpec) {
>+try {
>+  return XPCOMUtils.generateModule(component);
>+} catch(e) {
>+  throw e; 
>+}
>+}
>+

what are you expecting to fail here?
Attachment #302119 - Flags: review?(dietrich) → review-
Attached patch v3 (obsolete) — Splinter Review
moved to toolkit, addressed review comments. 
also, now returning RESULT_SUCCESS_ONGOING instead of RESULT_SUCCESS in line 201
Attachment #302119 - Attachment is obsolete: true
Attachment #305272 - Flags: review?
Attachment #305272 - Flags: review? → review?(dietrich)
Attached patch missed file in last patch (obsolete) — Splinter Review
moving the last patch to toolkit led me to miss the changes to editBookmarkOverlay.xul. So here's a patch with just those changes (its identical to the changes in the previous versions of the patch)
Comment on attachment 305272 [details] [diff] [review]
v3


>+
>+  // nsISupports
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAutoCompleteSearch]), 
>+
>+  classDescription: "Tag AutoComplete",
>+  contractID: "@mozilla.org/autocomplete/search;1?name=tag-autocomplete",
>+  classID: Components.ID("{1dcc23b0-d4cb-11dc-9ad6-479d56d89593}"),
>+  serviceURL: ""
>+};
>+
>+function _TimerCallback(callback) {
>+  this._callback = callback;
>+}
>+
>+_TimerCallback.prototype  = {  
>+  QueryInterface: function(iid) {
>+    if (iid.Equals(Ci.nsITimerCallback) ||
>+        iid.Equals(Ci.nsISupports))
>+      return this;
>+    throw Components.results.NS_ERROR_NO_INTERFACE;
>+  },
>+  
>+  notify: function(timer) {
>+    if (this._callback) 
>+      this._callback();
>+  }
>+};

don't need a separate object for this, just add the interface to your XPCOMUtils.generateQI() call above and add notify() to the main object.

a couple of final steps:

1. please add a unit test for this. this late in the game, it'll help to sell this to drivers. example code:

http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/tests/unit/test_history_autocomplete_tags.js

2. i uploaded the patch to the try server, the build name is "bug415960-tag-autocomplete": http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

i'll ping #ux to take it for a spin.
Attachment #305272 - Flags: review?(dietrich)
hrm, tryserver build failed to apply the patch. plus, i forgot that you'd added a supplimental patch w/ the forgotten file. i'll kick off a build w/ the next patch when it's ready.
Attached patch complete patch with unit test (obsolete) — Splinter Review
added unit tests...

not sure if this is a show-stopper, but I noticed that you can't hit enter to select a choice in the autocomplete popup -- something in the editbookmark dialog is capturing the key event before it gets to the autocomplete controller. 

kind of related to that, i'm not turning tabscrolling on, but maybe it would be more usable if that was on?
Attachment #305272 - Attachment is obsolete: true
Attachment #305281 - Attachment is obsolete: true
Attachment #306227 - Flags: review?(dietrich)
(In reply to comment #18)
> (From update of attachment 305272 [details] [diff] [review])
> 
> 
> don't need a separate object for this, just add the interface to your
> XPCOMUtils.generateQI() call above and add notify() to the main object.
> 

forgot to mention that the latest patch also fixes this
(In reply to comment #20)
> Created an attachment (id=306227) [details]
> complete patch with unit test
> 
> added unit tests...

fantastic, thanks

> 
> not sure if this is a show-stopper, but I noticed that you can't hit enter to
> select a choice in the autocomplete popup -- something in the editbookmark
> dialog is capturing the key event before it gets to the autocomplete
> controller. 

yep it's kind of annoying. mano said you can work around it:

[2:43pm] Mano: dietrich: browser-places.js indeed catches enter
[2:43pm] Mano: and already opt out few elements
[2:43pm] Mano: you just need to add that too
[2:44pm] Mano: this field, in its opened state
> 
> kind of related to that, i'm not turning tabscrolling on, but maybe it would be
> more usable if that was on?
> 

hm, only affects while the ac popup is showing right? if so, yeah that'd be fine.
Comment on attachment 306227 [details] [diff] [review]
complete patch with unit test

thanks, r=me. we can file followup bugs to address remaining tweaks and issues discussed above.
Attachment #306227 - Flags: review?(dietrich) → review+
hm, actually i just found a problem: typing a comma as the first character starts AC with a comma prefixed to each value. AC should detect and remove the delimiters.
Comment on attachment 306227 [details] [diff] [review]
complete patch with unit test

minusing, see comment 24.
Attachment #306227 - Flags: review+ → review-
started tryserver build, will be available here, search for this bug #:

https://build.mozilla.org/tryserver-builds/?C=M;O=D
Attached patch v4 (obsolete) — Splinter Review
This version enables the using enter key to select a tag and the tab key for scrolling.  
It also fixes the bug described in comment #24 and generally improves the UI: now it only shows matching tags in results popup, even if the user has entered more than one tag (before it repeated everything the user had entered). This is implemented by setting the matching tag as the result comment, and hiding the result column, so only the comment column appears. The CSS for the comments column probably should be tweaked but I couldn't figure out which CSS rules were applied to it.
Attachment #306227 - Attachment is obsolete: true
Attachment #306997 - Flags: review?(dietrich)
Any chance to have it in FF3 ?
Priority: -- → P2
Target Milestone: --- → Firefox 3.1
Blocks: 437246
Attached patch unrotted (obsolete) — Splinter Review
Attachment #306997 - Attachment is obsolete: true
Attachment #306997 - Flags: review?(dietrich)
(In reply to comment #28)
> Any chance to have it in FF3 ?
No.
Dietrich, what's the status of the latest unrotted patch? Is it ready for your review?
Flags: blocking-firefox3.1?
Version: unspecified → Trunk
https://build.mozilla.org/tryserver-builds/2008-08-12_10:54-dietrich@mozilla.com-bug415960-tag-ac/

Henrik: still targeting 3.1, going to do some further review and ux discussion this week.
That looks great. I've tested your tryserver build and noticed following issues:

1. We should improve the selection of the auto-complete suggestions. It's a bit odd to have to go down the list to select the first suggestion. We already display the suggestion in the textbox, so why not using ',' or enter to select the first suggestion? Comma is our separator for multiple tags to it would be great to have it as selector. Even more people will hit enter while typing the first letters of the tag. Currently the dialog closes when this key is pressed.

2. There is a strange overlap within the textbox. See the attachment. Happened after playing around with tags. If you cannot reproduce this isse I can try to find some STR.

3. If the window is too narrow the auto-complete box is wider than the Library window. This can also be seen within the attachment.

That's all for the moment. Hope that helps to speed it up a bit.
Whiteboard: [needs status update]
hm, since the last time i tried this patch, the autocomplete popup in the bookmarks contextual dialog is *behind* the panel.
(In reply to comment #35)
> hm, since the last time i tried this patch, the autocomplete popup in the
> bookmarks contextual dialog is *behind* the panel.
there were some panel element changes recently that could have caused that maybe?
Yeah, bug 451015. You probably need to add level="top" on the autocomplete panel? Though it looks like you're using the default... I bet we need to have 
http://hg.mozilla.org/mozilla-central/annotate/50ad9a5fd783/toolkit/content/widgets/autocomplete.xml#l104 set it.
Depends on: 454531
Attached patch more (obsolete) — Splinter Review
- a little cleanup
- don't suggest tags that are already in the field
Assignee: nobody → dietrich
Attachment #324651 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(In reply to comment #33)

thanks for testing!

> 1. We should improve the selection of the auto-complete suggestions. It's a bit
> odd to have to go down the list to select the first suggestion. We already
> display the suggestion in the textbox, so why not using ',' or enter to select
> the first suggestion? Comma is our separator for multiple tags to it would be
> great to have it as selector. Even more people will hit enter while typing the
> first letters of the tag. Currently the dialog closes when this key is pressed.

well right now completedefaultindex=true will inline-complete the first selection, and then you can use right-arrow to move to the end. maybe we could smooth this a bit by also adding ", " on right-arrow.

i think comma won't work since it'll cause false selections for substrings. eg: i type "bar," and suddenly i've got "barbarella," since that was first in the suggestion list.

using enter for this would be very inconsistent with it's use everywhere else. enter is more of a finalization action then the select-and-continue action that we want here.

another option could be to use tab for this instead of for scrolling. tab is already used to select the first suggestion in the location bar and search box, so it would be familiar in this context. the workflow of type->tab->type->tab would be pretty smooth, and would remove the need to move your hand over to the arrow keys and back between tags. auto-adding the ", " would make it even smoother.

> 2. There is a strange overlap within the textbox. See the attachment. Happened
> after playing around with tags. If you cannot reproduce this isse I can try to
> find some STR.

STR would be helpful, as i can't reproduce.

> 3. If the window is too narrow the auto-complete box is wider than the Library
> window. This can also be seen within the attachment.

hm, yes i can reproduce this on mac as well. only happens after some editing of tags in the field. haven't figured out exactly what's triggering it yet.
Attached patch more better (obsolete) — Splinter Review
Attachment #338330 - Attachment is obsolete: true
>i think comma won't work since it'll cause false selections for substrings. eg:
>i type "bar," and suddenly i've got "barbarella," since that was first in the
>suggestion list.

You can get around this by having the first item appear inline and selected.  So if you are tying "bar" the field will have ([] = selected text):

bar[baralla]

If you hit backspace or delete, the end of the first time is removed and the auto complete list is no longer displayed for this term.  You can then press comma or enter.

>using enter for this would be very inconsistent with it's use everywhere else.
>enter is more of a finalization action then the select-and-continue action that
>we want here.

Enter normally just selects an item in an auto-complete list and places it in the text field, so the behavior wouldn't be inconsistent.  Also I think users are going to expect to be able to hit enter.
(In reply to comment #42)
> You can get around this by having the first item appear inline and selected. 
> So if you are tying "bar" the field will have ([] = selected text):
> 
> bar[baralla]
> 
> If you hit backspace or delete, the end of the first time is removed and the
> auto complete list is no longer displayed for this term.  You can then press
> comma or enter.

i don't like that the user is forced to execute an extra action in order to keep the text they intended to use. regardless, this is really more of an enhancement (bordering on being a proper tag editor), so we should take this conversation over to bug 387485.

> Enter normally just selects an item in an auto-complete list and places it in
> the text field, so the behavior wouldn't be inconsistent.  Also I think users
> are going to expect to be able to hit enter.

ok, makes sense. i'll fix that in the next patch.
(In reply to comment #40)
> > 3. If the window is too narrow the auto-complete box is wider than the Library
> > window. This can also be seen within the attachment.
> 
> hm, yes i can reproduce this on mac as well. only happens after some editing of
> tags in the field. haven't figured out exactly what's triggering it yet.

ok, here's STR:

1. type in the text field, and a comma (eg: "f,")
2. hit tab repeatedly until you reach the last tag suggestion
3. hit enter once more

after tabbing past the final suggestion, the autocomplete popup goes wide. also, tags are saved at that point so some refocusing is going on.
Great. It saves me time to find the STR again. I'll check again after you posted the next version of the patch.
Hey Dietrich, are we still on track for landing this for beta 1?
(In reply to comment #46)
> Hey Dietrich, are we still on track for landing this for beta 1?

I'd like to fix the issue in comment #44 first, as it's visually egregious.
removing the focus ring outline on the element resolves this problem.
Attached patch w/o focus ring (obsolete) — Splinter Review
- proper fix for not showing previously entered tags
- remove focus ring altogether for now

i'll try to isolate the field outline issue, and file a follow up for that problem.
Attachment #338381 - Attachment is obsolete: true
Attachment #340873 - Flags: review?(mano)
Comment on attachment 340873 [details] [diff] [review]
w/o focus ring

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -123,7 +123,9 @@
>         }
>         else if (aEvent.keyCode == KeyEvent.DOM_VK_RETURN) {
>           // hide the panel unless the folder tree is focused
>-          if (aEvent.target.localName != "tree")
>+          // or tag autocomplete is active  
>+          if (aEvent.target.localName != "tree" || 
>+              !this._element("editBMPanel_tagsField").popupOpen)

I would rather check event.target. Please update the comment.


>             this.panel.hidePopup();
>         }
>         break;
>diff --git a/browser/components/places/content/editBookmarkOverlay.js b/browser/components/places/content/editBookmarkOverlay.js
>--- a/browser/components/places/content/editBookmarkOverlay.js
>+++ b/browser/components/places/content/editBookmarkOverlay.js
>@@ -462,6 +462,10 @@
>     }
>     this._itemId = -1;
>     this._uri = null;
>+  },
>+
>+  onTagsFieldFocus: function EIO_onTagsFieldFocus() {
>+    this._element('tagsField').popup.treecols.firstChild.hidden = true;

What's that? Why do you do it on focus (and every time the field is focused)? Comment inside function if it's right.  Pass the event and use aEvent.target.

>   },
> 
>   onTagsFieldBlur: function EIO_onTagsFieldBlur() {
>diff --git a/browser/components/places/content/editBookmarkOverlay.xul b/browser/components/places/content/editBookmarkOverlay.xul
>--- a/browser/components/places/content/editBookmarkOverlay.xul
>+++ b/browser/components/places/content/editBookmarkOverlay.xul
>@@ -172,6 +172,13 @@
>                  control="editBMPanel_tagsField"
>                  observes="paneElementsBroadcaster"/>
>           <textbox id="editBMPanel_tagsField"
>+                   type="autocomplete"
>+                   autocompletesearch="tag-autocomplete"

Please make that places-tags-autocomplete or anything else which is a little bit more self-descrbing.

>+                   enablehistory="false"

That's the default value.


> EXTRA_JS_MODULES = utils.js
>diff --git a/toolkit/components/places/src/nsTagAutoCompleteSearch.js b/toolkit/components/places/src/nsTagAutoCompleteSearch.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/src/nsTagAutoCompleteSearch.js
>@@ -0,0 +1,260 @@

>+  getValueAt: function(index) {

Name your functions.


>+// Implements nsIAutoCompleteSearch
>+function TagAutoCompleteSearch() {
>+}
>+
>+TagAutoCompleteSearch.prototype = {

Why isn't this part of the nsTaggingService implementation itself? That would simplify the code quite a bit.

>+  _stopped : false, 
>+  
>+  get tagging() {
>+    delete this._tagging;
>+    return this._tagging = Cc["@mozilla.org/browser/tagging-service;1"].
>+      getService(Ci.nsITaggingService);
>+  },

I don't think believe delete this.foo works in this context.

>+   */
>+  startSearch: function(searchString, searchParam, result, listener) {
>+    var searchResults = this.tagging.allTags;

This is way too expensive for us to do it every time a search is initialized. We need to either optimize the all tags getter (and cache it somehow), or maybe even use direct SQL queries, I'm fine with that given that we're in toolkit context.

>+
>+      var newResult = new TagAutoCompleteResult(searchString,
>+        Ci.nsIAutoCompleteResult.RESULT_SUCCESS, 0, "", results, comments);
>+      listener.onSearchResult(self, newResult);
>+      yield false;
>+    }
>+    
>+    var gen = doSearch();
>+    function driveGenerator() {
>+      if (gen.next()) { 
>+        // my you have a lot of tags

Fix this comment.

>+        var timer = Cc["@mozilla.org/timer;1"]
>+          .createInstance(Components.interfaces.nsITimer);
>+        self._callback = driveGenerator;
>+        timer.initWithCallback(self, 0, timer.TYPE_ONE_SHOT);
>+      } else
>+        gen.close();	

Don't use this code style.

>+  classID: Components.ID("{1dcc23b0-d4cb-11dc-9ad6-479d56d89593}"),
>+  serviceURL: ""

What's this used for? I see it in search-suggestion as well, but gavin doesn't know what is it either.
Attachment #340873 - Flags: review?(mano) → review-
(In reply to comment #51)
> >+  classID: Components.ID("{1dcc23b0-d4cb-11dc-9ad6-479d56d89593}"),
> >+  serviceURL: ""
> 
> What's this used for? I see it in search-suggestion as well, but gavin doesn't
> know what is it either.

Probably just junk, I wouldn't hesitate to remove it :)
(In reply to comment #51)
> >+
> >+  onTagsFieldFocus: function EIO_onTagsFieldFocus() {
> >+    this._element('tagsField').popup.treecols.firstChild.hidden = true;
> 
> What's that? Why do you do it on focus (and every time the field is focused)?
> Comment inside function if it's right.  Pass the event and use aEvent.target.
>

The autocomplete control doesn't support having the results value be different from what's displayed. To hack around this I stick what I want to be displayed (the current tag name) into the comments column and hide the results column that contains the actually value (the entire contents of the edit field). 

IIRC, I chose to do this on focus because I can be assured the tree control used by the autocomplete popup has been created -- i think i had problems doing this earlier. And yes, this only has to be done once, not everytime the control gets focus.
Attached patch more (obsolete) — Splinter Review
some fixes. since we already keep the result node for the tag root around, this uses nsINavHistoryResultViewer to invalidate the cached sorted tag list.
Attachment #340873 - Attachment is obsolete: true
Attached patch addresses mano's review comments (obsolete) — Splinter Review
> >+// Implements nsIAutoCompleteSearch
> >+function TagAutoCompleteSearch() {
> >+}
> >+
> >+TagAutoCompleteSearch.prototype = {
> 
> Why isn't this part of the nsTaggingService implementation itself? That would
> simplify the code quite a bit.

not a lot to be shared here, given that they're different components, and singleton vs not. however, should mitigate the inevitable Ts hit by having it all in one file.

> >+  _stopped : false, 
> >+  
> >+  get tagging() {
> >+    delete this._tagging;
> >+    return this._tagging = Cc["@mozilla.org/browser/tagging-service;1"].
> >+      getService(Ci.nsITaggingService);
> >+  },
> 
> I don't think believe delete this.foo works in this context.

replaced w/ shadow getter

> >+   */
> >+  startSearch: function(searchString, searchParam, result, listener) {
> >+    var searchResults = this.tagging.allTags;
> 
> This is way too expensive for us to do it every time a search is initialized.
> We need to either optimize the all tags getter (and cache it somehow), or maybe
> even use direct SQL queries, I'm fine with that given that we're in toolkit
> context.

now caching the sorted tags array, and invalidating it via nsINavHistoryResultViewer.
Attachment #341236 - Attachment is obsolete: true
Attachment #341454 - Flags: review?(mano)
Why is this change from onfcusd to addEventListener?
+
+    // listen for focus events on the tags field
+    this._element("tagsField").addEventListener("focus", this.onTagsFieldFocus, false);

passing this.someFunction is almost always wrong,.
Approving blocking, is an oft requested feature and something we really wanted for 3.0.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Whiteboard: [needs status update]
Dietrich: what happened with the CSS fix I'd suggested?
CSS is fixed, but now a test is failing, and I haven't gotten time to dig into it yet.
Attachment #341454 - Attachment is obsolete: true
Attachment #344320 - Flags: review?(mano)
Attachment #341454 - Flags: review?(mano)
Comment on attachment 344320 [details] [diff] [review]
addresses mano's review comments

r=mano
Attachment #344320 - Flags: review?(mano) → review+
http://hg.mozilla.org/mozilla-central/rev/efe787d28285
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.1b2
Verfied the feature landing with the following builds. Regressions and enhancements should be filed as new bugs and should block this one.
 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081101 Minefield/3.1b2pre ID:20081101020236Mozilla/5.0 (Windows; U; 

Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081101 Minefield/3.1b2pre ID:20081101032525
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus?
BTW: For new bookmarks, I've started adding "key works" (tags) to the bookmark's "name" instead of creating tags for these terms, because that way, I can add an *endless* number of very specific "tags" without cluttering the tags list (which I only use for more general categorization: politics, personal, etc.). Or am I missing something about how to "manage" very large numbers of tags?
(In reply to comment #64)
> etc.). Or am I missing something about how to "manage" very large numbers of
> tags?

I thought there is a filed bug which handles this topic. But I'm still not able to find it. Marco or Dietrich, does one of you know it?
No longer depends on: 462661, 462662, 462663
>Or am I missing something about how to "manage" very large numbers of
>tags?

Not really, the more tags you crate the less logical the tree view UI that we currently have in the Library window is.  For very large tag sets the expectation is that users will be primarily be entering the tag name directly into the location bar, as opposed to browsing through a long list of tags to find it.  We will probably want to rethink the browse-able tag UI in the library in the future.  One feature that didn't make it was a faceted browsing interface (scroll down for the second part of the mockup):
http://people.mozilla.com/~faaborg/files/granParadisoUI/places_OrganizerTags_i5.png
>I thought there is a filed bug which handles this topic. But I'm still not able
>to find it. Marco or Dietrich, does one of you know it?

The overall tracking bug for tag UI was bug 393512, I resolved it at the same time as all of the other meta tracking bugs for mockups since the design might change in the future.
(In reply to comment #65)
> (In reply to comment #64)
> > etc.). Or am I missing something about how to "manage" very large numbers of
> > tags?
> 
> I thought there is a filed bug which handles this topic. But I'm still not able
> to find it. Marco or Dietrich, does one of you know it?

couldn't find one. filed bug 462710.
I seem to have a tag that breaks the autocomplete (I was very surprised to see the bug being marked "fixed 1.9.1").

The error is 
Error: invalid quantifier +$|c++(,|;))
Source File: file:///C:/volatile/Shiretoko/components/nsTaggingService.js
Line: 490

Looking at that line I'd guess my tag "c++" is built into a regex which breaks. Is that a known problem, does someone have a bug number?
(In reply to comment #69)
> I seem to have a tag that breaks the autocomplete (I was very surprised to see
> the bug being marked "fixed 1.9.1").
> 
> The error is 
> Error: invalid quantifier +$|c++(,|;))
> Source File: file:///C:/volatile/Shiretoko/components/nsTaggingService.js
> Line: 490

please file a new bug
filed bug 469102
Depends on: 469102
Test case https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=7468 has been created for regression testing.
Flags: in-litmus? → in-litmus+
No longer depends on: 474987
Depends on: 481767
i've done some amount of tag prefixing by domain:

tech.osx
tech.mozilla
project.this
project.that

autocomplete doesn't seem to handle this scenario very well. i would like to type osx and get "tech.osx" as my first autocomplete suggestion. instead the autocomplete algorithm only seems to work if you start typing from the very start of the tag name. obviously typing "tech" everytime renders autocompleting almost useless, because it takes the same amount of time using the mouse and manually finding and selecting the tag.

1. is this the right bug # to look at this issue?
2. what do you guys, who worked on the original autocomplete, think about this issue - are there some side-effects i havent considered?
I think you should use two tags :
tech, osx
tech, mozilla
I think the auto-complete should use the Location Bar's algorithm: i.e., search *within* words, and order according to "recency" (frequency + recentness). 

Tags are separated by commas (and semicolons?) (but not by periods?), so words with a period in them (without spaces) should be treated as one word.

Leho: I suggest you file a new bug for your suggestion. Note that this is an edge use-case, so it likely won't get a high priority.
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.