Added :bmark -id, changed updating semantincs, update URLs in cache, and reported updates as updates rather than additions.

5 files changed, 75 insertions, 51 deletions

diff --git a/common/content/bookmarks.js b/common/content/bookmarks.js
index f6832d5a..b4a01295 100644
--- a/common/content/bookmarks.js
+++ b/common/content/bookmarks.js
@@ -63,49 +63,54 @@ var Bookmarks = Module("bookmarks", {
      *      Otherwise, if a bookmark for the given URL exists it is
      *      updated instead.
      *      @optional
-     * @returns {boolean} True if the bookmark was added or updated
-     *      successfully.
+     * @returns {boolean} True if the bookmark was updated, false if a
+     *      new bookmark was added.
      */
     add: function add(unfiled, title, url, keyword, tags, force) {
         // FIXME
         if (isObject(unfiled))
-            var { unfiled, title, url, keyword, tags, post, charset, force } = unfiled;
-
-        try {
-            let uri = util.createURI(url);
-            if (!force && bookmarkcache.isBookmarked(uri))
-                for (var bmark in bookmarkcache)
-                    if (bmark.url == uri.spec) {
-                        if (title)
-                            bmark.title = title;
+            var { id, unfiled, title, url, keyword, tags, post, charset, force } = unfiled;
+
+        let uri = util.createURI(url);
+        if (id != null)
+            var bmark = bookmarkcache.bookmarks[id];
+        else if (!force) {
+            if (keyword && Set.has(bookmarkcache.keywords, keyword))
+                bmark = bookmarkcache.keywords[keyword];
+            else if (bookmarkcache.isBookmarked(uri))
+                for (bmark in bookmarkcache)
+                    if (bmark.url == uri.spec)
                         break;
-                    }
+        }
 
-            if (tags) {
-                PlacesUtils.tagging.untagURI(uri, null);
-                PlacesUtils.tagging.tagURI(uri, tags);
-            }
-            if (bmark == undefined)
-                bmark = bookmarkcache.bookmarks[
-                    services.bookmarks.insertBookmark(
-                         services.bookmarks[unfiled ? "unfiledBookmarksFolder" : "bookmarksMenuFolder"],
-                         uri, -1, title || url)];
-            if (!bmark)
-                return false;
-
-            if (charset !== undefined)
-                bmark.charset = charset;
-            if (post !== undefined)
-                bmark.post = post;
-            if (keyword)
-                bmark.keyword = keyword;
+        if (tags) {
+            PlacesUtils.tagging.untagURI(uri, null);
+            PlacesUtils.tagging.tagURI(uri, tags);
         }
-        catch (e) {
-            util.reportError(e);
-            return false;
+
+        let updated = !!bmark;
+        if (bmark == undefined)
+            bmark = bookmarkcache.bookmarks[
+                services.bookmarks.insertBookmark(
+                     services.bookmarks[unfiled ? "unfiledBookmarksFolder" : "bookmarksMenuFolder"],
+                     uri, -1, title || url)];
+        else {
+            if (title)
+                bmark.title = title;
+            if (!uri.equals(bmark.uri))
+                bmark.uri = uri;
         }
 
-        return true;
+        util.assert(bmark);
+
+        if (charset !== undefined)
+            bmark.charset = charset;
+        if (post !== undefined)
+            bmark.post = post;
+        if (keyword)
+            bmark.keyword = keyword;
+
+        return updated;
     },
 
     /**
@@ -163,6 +168,7 @@ var Bookmarks = Module("bookmarks", {
             let extra = "";
             if (title != url)
                 extra = " (" + title + ")";
+
             this.add({ unfiled: true, title: title, url: url });
             dactyl.echomsg({ domains: [util.getHost(url)], message: _("bookmark.added", url + extra) });
         }
@@ -438,9 +444,13 @@ var Bookmarks = Module("bookmarks", {
         commands.add(["bma[rk]"],
             "Add a bookmark",
             function (args) {
+                dactyl.assert(!args.bang || args["-id"] == null,
+                              _("bookmark.bangOrID"));
+
                 let opts = {
                     force: args.bang,
                     unfiled: false,
+                    id: args["-id"],
                     keyword: args["-keyword"] || null,
                     charset: args["-charset"],
                     post: args["-post"],
@@ -449,13 +459,13 @@ var Bookmarks = Module("bookmarks", {
                     url: args.length === 0 ? buffer.uri.spec : args[0]
                 };
 
-                if (bookmarks.add(opts)) {
-                    let extra = (opts.title == opts.url) ? "" : " (" + opts.title + ")";
-                    dactyl.echomsg({ domains: [util.getHost(opts.url)], message: _("bookmark.added", opts.url + extra) },
-                                   1, commandline.FORCE_SINGLELINE);
-                }
-                else
-                    dactyl.echoerr(_("bookmark.cantAdd", opts.title.quote()));
+                let updated = bookmarks.add(opts);
+                let action  = updated ? "updated" : "added";
+
+                let extra   = (opts.title == opts.url) ? "" : " (" + opts.title + ")";
+
+                dactyl.echomsg({ domains: [util.getHost(opts.url)], message: _("bookmark." + action, opts.url + extra) },
+                               1, commandline.FORCE_SINGLELINE);
             }, {
                 argCount: "?",
                 bang: true,
@@ -477,6 +487,11 @@ var Bookmarks = Module("bookmarks", {
                         type: CommandOption.STRING,
                         completer: function (context) completion.charset(context),
                         validator: Option.validateCompleter
+                    },
+                    {
+                        names: ["-id"],
+                        description: "The ID of the bookmark to update",
+                        type: CommandOption.INT
                     }
                 ]
             });
@@ -553,6 +568,7 @@ var Bookmarks = Module("bookmarks", {
                 if (bmarks.length == 1) {
                     let bmark = bmarks[0];
 
+                    options["-id"] = bmark.id;
                     options["-title"] = bmark.title;
                     if (bmark.charset)
                         options["-charset"] = bmark.charset;
diff --git a/common/locale/en-US/marks.xml b/common/locale/en-US/marks.xml
index 375667bc..75aaf29a 100644
--- a/common/locale/en-US/marks.xml
+++ b/common/locale/en-US/marks.xml
@@ -52,6 +52,10 @@
                 sites that require query parameters in encodings other than
                 UTF-8 (short name <em>-c</em>).
             </dd>
+            <dt>-id</dt>
+            <dd>
+                The numerical ID of a bookmark to update.
+            </dd>
             <dt>-keyword</dt>
             <dd>
                 A keyword which may be used to open the bookmark via
@@ -88,7 +92,7 @@
         <p>
             If <oa>!</oa> is present, a new bookmark is always
             added. Otherwise, the first bookmark matching
-            <oa>url</oa> is updated.
+            <oa>id</oa>, <oa>keyword</oa>, or <oa>url</oa> is updated.
         </p>
     </description>
 </item>
diff --git a/common/locale/en-US/messages.properties b/common/locale/en-US/messages.properties
index 65494f82..c0d53b03 100644
--- a/common/locale/en-US/messages.properties
+++ b/common/locale/en-US/messages.properties
@@ -33,10 +33,12 @@ bookmark.noMatching-2 = No bookmarks matching tags %S and string %S
 bookmark.noMatchingTags-1 = No bookmarks matching tags %S
 bookmark.noMatchingString-1 = No bookmarks matching string %S
 bookmark.none = No bookmarks set
+bookmark.bangOrID = Only one of ! or -id may be given
 bookmark.cantAdd-1 = Could not add bookmark %S
 bookmark.allDeleted = All bookmarks deleted
 bookmark.removed-1 = Removed bookmark: %S
 bookmark.added-1 = Added bookmark: %S
+bookmark.updated-1 = Updated bookmark: %S
 bookmark.deleted-1 = %S bookmark(s) deleted
 bookmark.prompt.deleteAll = This will delete all bookmarks. Would you like to continue? (yes/[no]):
 
diff --git a/common/modules/bookmarkcache.jsm b/common/modules/bookmarkcache.jsm
index f030e750..ea377e39 100644
--- a/common/modules/bookmarkcache.jsm
+++ b/common/modules/bookmarkcache.jsm
@@ -20,6 +20,12 @@ update(Bookmark.prototype, {
     ].filter(function (item) item[1]),
 
     get uri() util.newURI(this.url),
+    set uri(uri) {
+        let tags = this.tags;
+        this.tags = null;
+        services.bookmarks.changeBookmarkURI(this.id, uri);
+        this.tags = tags;
+    },
 
     encodeURIComponent: function _encodeURIComponent(str) {
         if (!this.charset || this.charset === "UTF-8")
@@ -28,15 +34,9 @@ update(Bookmark.prototype, {
         return escape(conv.ConvertFromUnicode(str) + conv.Finish());
     }
 })
+Bookmark.prototype.members.uri = Bookmark.prototype.members.url;
 Bookmark.setter = function (key, func) this.prototype.__defineSetter__(key, func);
-Bookmark.setter("url", function (val) {
-    if (isString(val))
-        val = util.newURI(val);
-    let tags = this.tags;
-    this.tags = null;
-    services.bookmarks.changeBookmarkURI(this.id, val);
-    this.tags = tags;
-});
+Bookmark.setter("url", function (val) { this.uri = isString(val) ? util.newURI(val) : val; });
 Bookmark.setter("title", function (val) { services.bookmarks.setItemTitle(this.id, val); });
 Bookmark.setter("post", function (val) { bookmarkcache.annotate(this.id, bookmarkcache.POST, val); });
 Bookmark.setter("charset", function (val) { bookmarkcache.annotate(this.id, bookmarkcache.CHARSET, val); });
diff --git a/common/modules/dom.jsm b/common/modules/dom.jsm
index 05311d9f..d27876bc 100644
--- a/common/modules/dom.jsm
+++ b/common/modules/dom.jsm
@@ -741,7 +741,9 @@ var DOM = Class("DOM", {
             let win = this.document.defaultView;
 
             if (force || !(rect && rect.bottom <= win.innerHeight && rect.top >= 0 && rect.left < win.innerWidth && rect.right > 0))
-                elem.scrollIntoView(alignWithTop !== undefined ? alignWithTop
+                elem.scrollIntoView(alignWithTop !== undefined ? alignWithTop :
+                                    rect.bottom < 0            ? true         :
+                                    rect.top > win.innerHeight ? false
                                                                : Math.abs(rect.top) < Math.abs(win.innerHeight - rect.bottom));
         });
     },