fix :mark and :delmarks (AKA return of the marks) and add range support to

:delmarks

chrome/content/vimperator/bookmarks.js

@@ -352,8 +352,10 @@ function Marks() //{{{
     ////////////////////////////////////////////////////////////////////////////////
     ////////////////////// PRIVATE SECTION /////////////////////////////////////////
     /////////////////////////////////////////////////////////////////////////////{{{
-    var marks = {};
+
-    var pendingMarks = [];
+    var local_marks = {};
+    var url_marks = {};
+    var pending_jumps = [];
     var appcontent = document.getElementById("appcontent");
     if (appcontent)
         appcontent.addEventListener("load", onPageLoad, true);
@@ -361,152 +363,156 @@ function Marks() //{{{
     function onPageLoad(event)
     {
         var win = event.originalTarget.defaultView;
-        for (var i = 0, length = pendingMarks.length; i < length; i++)
+        for (var i = 0, length = pending_jumps.length; i < length; i++)
         {
-            var mark = pendingMarks[i];
+            var mark = pending_jumps[i];
             if (win.location.href == mark.location)
             {
                 win.scrollTo(mark.position.x * win.scrollMaxX, mark.position.y * win.scrollMaxY);
-                pendingMarks.splice(i, 1);
+                pending_jumps.splice(i, 1);
                 return;
             }
         }
     }
 
-    function remove(mark)
+    function removeLocalMark(mark)
     {
-        var ok = false;
+        if (mark in local_marks)
-        if (mark.match(/^[A-Z0-9]$/))
         {
-            if (mark in marks)
+            var win = window.content;
+            for (var i = 0; i < local_marks[mark].length; i++)
             {
-                delete marks[mark];
+                if (local_marks[mark][i].location == win.location.href)
-                ok = true;
-            }
-        }
-        else if (mark.match(/^[a-z]$/))
-        {
-            if (mark in marks)
-            {
-                var win = vimperator.getCurrentBuffer(), length = marks[mark].length;
-                for (var i = 0; i < length; i++)
                 {
-                    if (marks[mark][i].location == win.location.href)
+                    vimperator.log("Deleting local mark: " + mark + " | " + local_marks[mark][i].location + " | (" + local_marks[mark][i].position.x + ", " + local_marks[mark][i].position.y + ") | tab: " + vimperator.tabs.index(local_marks[mark][i].tab));
-                    {
+                    local_marks[mark].splice(i, 1);
-                        marks[mark].splice(i, 1);
+                    if (local_marks[mark].length == 0)
-                        ok = true;
+                        delete local_marks[mark];
-                        break;
+                    break;
-                    }
                 }
             }
         }
+    }
 
-        if (!ok)
+    function removeURLMark(mark)
+    {
+        if (mark in url_marks)
         {
-            vimperator.echoerr("E20: Mark not set");
+            vimperator.log("Deleting URL mark: " + mark + " | " + url_marks[mark].location + " | (" + url_marks[mark].position.x + ", " + url_marks[mark].position.y + ") | tab: " + vimperator.tabs.index(url_marks[mark].tab));
-            return false;
+            delete url_marks[mark];
         }
-        return ok;
+    }
+
+    function isLocalMark(mark)
+    {
+        return /^[a-z]$/.test(mark);
+    }
+
+    function isURLMark(mark)
+    {
+        return /^[A-Z0-9]$/.test(mark);
     }
 
     /////////////////////////////////////////////////////////////////////////////}}}
     ////////////////////// PUBLIC SECTION //////////////////////////////////////////
     /////////////////////////////////////////////////////////////////////////////{{{
+
+    // TODO: add support for frameset pages
     this.add = function(mark)
     {
-        var win = vimperator.getCurrentBuffer();
+        var win = window.content;
+
+        if (win.document.body.localName.toLowerCase() == "frameset")
+        {
+            vimperator.echo("marks support for frameset pages not implemented yet");
+            return;
+        }
+
         var x = win.scrollMaxX ? win.pageXOffset / win.scrollMaxX : 0;
         var y = win.scrollMaxY ? win.pageYOffset / win.scrollMaxY : 0;
         var position = { x: x, y: y };
-        if (mark.match(/^[A-Z0-9]$/))
+        if (isURLMark(mark))
-            marks[mark] = { location: win.location.href, position: position, tab: vimperator.tabs.getTab() };
-        else if (mark.match(/^[a-z]$/))
         {
-            if (!marks[mark])
+            vimperator.log("Adding URL mark: " + mark + " | " + win.location.href + " | (" + position.x + ", " + position.y + ") | tab: " + vimperator.tabs.index(vimperator.tabs.getTab()));
-                marks[mark] = [];
+            url_marks[mark] = { location: win.location.href, position: position, tab: vimperator.tabs.getTab() };
-            marks[mark].push({ location: win.location.href, position: position });
+        }
+        else if (isLocalMark(mark))
+        {
+            // remove any previous mark of the same name for this location
+            removeLocalMark(mark);
+            if (!local_marks[mark])
+                local_marks[mark] = [];
+            vimperator.log("Adding local mark: " + mark + " | " + win.location.href + " | (" + position.x + ", " + position.y + ")");
+            local_marks[mark].push({ location: win.location.href, position: position });
         }
-        else
-            return false;
-
-        return this;
     }
 
-    // TODO: add support for mark ranges (p-z)
     this.remove = function(marks_str, special)
     {
         if (special)
         {
-            var win = vimperator.getCurrentBuffer();
+            // :delmarks! only deletes a-z marks
-            for (var i in marks)
+            for (var mark in local_marks)
-            {
+                removeLocalMark(mark);
-                if (i.match(/^[A-Z0-9]$/))
-                    continue;
-
-                var length = marks[i].length;
-                for (var j = 0; j < length; j++)
-                {
-                    if (marks[i][j].location == win.location.href)
-                    {
-                        marks[i].splice(j, 1);
-                        ok = true;
-                        break;
-                    }
-                }
-            }
-            return this;
         }
-        marks_str.split(/ /).forEach(function(mark)
+        else
         {
-            if (mark.length == 1)
+            var pattern = new RegExp("[" + marks_str.replace(/\s+/g, '') + "]");
-                remove(mark);
+            for (mark in url_marks)
-            else
+            {
-                for (var i = 0; i < mark.length; i++)
+                if (pattern.test(mark))
-                {
+                    removeURLMark(mark);
-                    remove(mark.substr(i, 1));
+            }
-                }
+            for (mark in local_marks)
-        });
+            {
-        return this;
+                if (pattern.test(mark))
+                    removeLocalMark(mark);
+            }
+        }
     }
 
     this.jumpTo = function(mark)
     {
         var ok = false;
-        if (mark.match(/^[A-Z0-9]$/))
+        if (isURLMark(mark))
         {
-            var slice = marks[mark];
+            var slice = url_marks[mark];
             if (slice && slice.tab && slice.tab.linkedBrowser)
             {
                 if (!slice.tab.parentNode)
                 {
-                    pendingMarks.push(slice);
+                    pending_jumps.push(slice);
+                    // NOTE: this obviously won't work on generated pages using
+                    // non-unique URLs, like Vimperator's help :(
                     openURLsInNewTab(slice.location, true);
                     return;
                 }
                 var index = vimperator.tabs.index(slice.tab);
-                if (index)
+                if (index != -1)
                 {
                     vimperator.tabs.select(index);
                     var win = slice.tab.linkedBrowser.contentWindow;
                     if (win.location.href != slice.location)
                     {
-                        pendingMarks.push(slice);
+                        pending_jumps.push(slice);
                         win.location.href = slice.location;
                         return;
                     }
+                    vimperator.log("Jumping to URL mark: " + mark + " | " + slice.location + " | (" + slice.position.x + ", " + slice.position.y + ") | tab: " + vimperator.tabs.index(slice.tab));
                     win.scrollTo(slice.position.x * win.scrollMaxX, slice.position.y * win.scrollMaxY);
                     ok = true;
                 }
             }
         }
-        else if (mark.match(/^[a-z]$/))
+        else if (isLocalMark(mark))
         {
-            var win = vimperator.getCurrentBuffer();
+            var win = window.content;
-            var slice = marks[mark] || [];
+            var slice = local_marks[mark] || [];
             for (var i = 0; i < slice.length; i++)
             {
                 if (win.location.href == slice[i].location)
                 {
+                    vimperator.log("Jumping to local mark: " + mark + " | " + slice[i].location + " | (" + slice[i].position.x + ", " + slice[i].position.y + ")");
                     win.scrollTo(slice[i].position.x * win.scrollMaxX, slice[i].position.y * win.scrollMaxY);
                     ok = true;
                 }
@@ -514,17 +520,14 @@ function Marks() //{{{
         }
 
         if (!ok)
-        {
+            vimperator.echoerr("E20: Mark not set"); // FIXME: move up?
-            vimperator.echoerr("E20: Mark not set");
+
-            return false;
+        return ok;
-        }
-        return this;
     }
 
-    // TODO: show marks like vim does (when the multiline echo impl is done) or in the preview windwo right now
+    // TODO: show marks like vim does (when the multiline echo impl is done) or in the preview window right now
     this.list = function()
     {
-        return this;
     }
     //}}}
 } //}}}

chrome/content/vimperator/commands.js

@@ -329,12 +329,53 @@ function Commands() //{{{
         }
     ));
     addDefaultCommand(new Command(["delm[arks]"],
-        function(args, special) { vimperator.marks.remove(args, special); },
+        function(args, special) {
+            if (!special && !args)
+            {
+                vimperator.echoerr("E471: Argument required");
+                return;
+            }
+            if (special && args)
+            {
+                vimperator.echoerr("E474: Invalid argument");
+                return
+            }
+            var matches;
+            if (matches = args.match(/(?:(?:^|[^a-zA-Z0-9])-|-(?:$|[^a-zA-Z0-9])|[^a-zA-Z0-9 -]).*/))
+            {
+                // TODO: this currently differs from Vim's behaviour which
+                // deletes any valid marks in the arg list, up to the first
+                // invalid arg, as well as giving the error message. Do we want
+                // to match this behaviour?
+                vimperator.echoerr("E475: Invalid argument: " + matches[0]);
+                return;
+            }
+            // check for illegal ranges - only allow a-z A-Z 0-9
+            if (matches = args.match(/[a-zA-Z0-9]-[a-zA-Z0-9]/))
+            {
+                for (var i = 0; i < matches.length; i++)
+                {
+                    var start = matches[i][0];
+                    var end   = matches[i][2];
+                    if (/[a-z]/.test(start) != /[a-z]/.test(end) ||
+                        /[A-Z]/.test(start) != /[A-Z]/.test(end) ||
+                        /[0-9]/.test(start) != /[0-9]/.test(end) ||
+                        start > end)
+                    {
+                        vimperator.echoerr("E475: Invalid argument: " + args.match(new RegExp(matches[i] + ".*"))[0]);
+                        return;
+                    }
+                }
+            }
+
+            vimperator.marks.remove(args, special);
+        },
         {
             usage: ["delm[arks]! {marks}"],
             short_help: "Delete the specified marks {a-zA-Z}",
             help: "Marks are presented as a list. Example:<br/>" +
                 "<code class=\"command\">:delmarks Aa b p</code> will delete marks A, a, b and p<br/>" +
+                "<code class=\"command\">:delmarks b-p</code> will delete all marks in the range b to p<br/>" +
                 "<code class=\"command\">:delmarks!</code> will delete all marks for the current buffer",
         }
 
@@ -450,7 +491,23 @@ function Commands() //{{{
         }
     ));
     addDefaultCommand(new Command(["ma[rk]"],
-        function(args) { vimperator.marks.add(args) },
+        function(args) {
+            if (!args) {
+                vimperator.echoerr("E471: Argument required");
+                return;
+            }
+            if (args.length > 1) {
+                vimperator.echoerr("E488: Trailing characters");
+                return;
+            }
+            if (!/[a-zA-Z]/.test(args))
+            {
+                vimperator.echoerr("E191: Argument must be a letter or forward/backward quote");
+                return;
+            }
+
+            vimperator.marks.add(args);
+        },
         {
             usage: ["ma[rk] {arg}"],
             short_help: "Mark current location within the webpage",
@@ -458,8 +515,8 @@ function Commands() //{{{
     ));
     addDefaultCommand(new Command(["marks"],
         function(args) {
-            vimperator.echo("marks not implemented yet");
+            //vimperator.echo("marks not implemented yet");
-            //vimperator.marks.list(args)
+            vimperator.marks.list(args)
         },
         {
             usage: ["marks {arg}"],

chrome/content/vimperator/vimperator.js

@@ -309,12 +309,6 @@ function Vimperator() //{{{
         content.focus();
     }
 
-
-    this.getCurrentBuffer = function()
-    {
-        return document.commandDispatcher.focusedWindow;
-    }
-
     /**
      * logs any object to the javascript error console
      * also prints all properties of thie object
@@ -918,7 +912,7 @@ function Tabs() //{{{
     // @returns the index of the currently selected tab starting with 0
     this.index = function(tab)
     {
-        if(tab)
+        if (tab)
         {
             var length = getBrowser().mTabs.length;
             for (var i = 0; i < length; i++)
@@ -926,7 +920,7 @@ function Tabs() //{{{
                 if (getBrowser().mTabs[i] == tab)
                     return i;
             }
-            return false;
+            return -1;
         }
 
         return getBrowser().tabContainer.selectedIndex;