significant rewrite of the $location service

- don't update browser before and after eval instead
  - sync location properties before eval
  - sync location properties and update browser after eval
- added tests
- symplified the code
- removed $location.toString() because it was not idempotent and useless

This resolves the issue with issuing two $route.onHashChange calls
when the $location was updated with a hashPath that needs to be encoded

5 files changed, 150 insertions, 108 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0b965c4f..5968768b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -52,6 +52,8 @@
 
 - `angular.foreach` was renamed to `angular.forEach` to make the api consistent.
 
+- The `toString` method of the `angular.service.$location` service was removed.
+
 
 # <angular/> 0.9.8 astral-projection (2010-12-23) #
 
diff --git a/src/Browser.js b/src/Browser.js
index eb6afb3c..671ec1cc 100644
--- a/src/Browser.js
+++ b/src/Browser.js
@@ -232,6 +232,7 @@ function Browser(window, document, body, XHR, $log) {
    * {@link angular.service.$location $location service} to monitor hash changes in angular apps.
    *
    * @param {function(event)} listener Listener function to be called when url hash changes.
+   * @return {function()} Returns the registered listener fn - handy if the fn is anonymous.
    */
   self.onHashChange = function(listener) {
     if ('onhashchange' in window) {
@@ -245,6 +246,7 @@ function Browser(window, document, body, XHR, $log) {
         }
       });
     }
+    return listener;
   }
 
   //////////////////////////////////////////////////////////////
diff --git a/src/services.js b/src/services.js
index b9325721..573eca21 100644
--- a/src/services.js
+++ b/src/services.js
@@ -70,23 +70,18 @@ angularServiceInject("$document", function(window){
  */
 angularServiceInject("$location", function($browser) {
   var scope = this,
-      location = {toString:toString, update:update, updateHash: updateHash},
-      lastBrowserUrl = $browser.getUrl(),
-      lastLocationHref,
-      lastLocationHash;
-
-  $browser.onHashChange(function() {
-    update(lastBrowserUrl = $browser.getUrl());
-    updateLastLocation();
+      location = {update:update, updateHash: updateHash},
+      lastLocation = {};
+
+  $browser.onHashChange(function() { //register
+    update($browser.getUrl());
+    copy(location, lastLocation);
     scope.$eval();
-  });
+  })(); //initialize
 
-  this.$onEval(PRIORITY_FIRST, updateBrowser);
+  this.$onEval(PRIORITY_FIRST, sync);
   this.$onEval(PRIORITY_LAST, updateBrowser);
 
-  update(lastBrowserUrl);
-  updateLastLocation();
-
   return location;
 
   // PUBLIC METHODS
@@ -107,7 +102,7 @@ angularServiceInject("$location", function($browser) {
    * scope.$location.update({host: 'www.google.com', protocol: 'https'});
    * scope.$location.update({hashPath: '/path', hashSearch: {a: 'b', x: true}});
    *
-   * @param {(string|Object)} href Full href as a string or hash object with properties
+   * @param {(string|Object)} href Full href as a string or object with properties
    */
   function update(href) {
     if (isString(href)) {
@@ -163,62 +158,55 @@ angularServiceInject("$location", function($browser) {
     update(hash);
   }
 
-  /**
-   * @workInProgress
-   * @ngdoc method
-   * @name angular.service.$location#toString
-   * @methodOf angular.service.$location
-   * 
-   * @description
-   * Returns string representation - href
-   */
-  function toString() {
-    updateLocation();
-    return location.href;
-  }
 
   // INNER METHODS
 
   /**
-   * Update location object
+   * Synchronizes all location object properties.
    *
    * User is allowed to change properties, so after property change,
    * location object is not in consistent state.
    *
+   * Properties are synced with the following precedence order:
+   *
+   * - `$location.href`
+   * - `$location.hash`
+   * - everything else
+   *
    * @example
    * scope.$location.href = 'http://www.angularjs.org/path#a/b'
    * immediately after this call, other properties are still the old ones...
    *
    * This method checks the changes and update location to the consistent state
    */
-  function updateLocation() {
-    if (location.href == lastLocationHref) {
-      if (location.hash == lastLocationHash) {
+  function sync() {
+    if (!equals(location, lastLocation)) {
+      if (location.href != lastLocation.href) {
+        update(location.href);
+        return;
+      }
+      if (location.hash != lastLocation.hash) {
+        var hash = parseHash(location.hash);
+        updateHash(hash.path, hash.search);
+      } else {
         location.hash = composeHash(location);
+        location.href = composeHref(location);
       }
-      location.href = composeHref(location);
+      update(location.href);
     }
-    update(location.href);
   }
 
-  /**
-   * Update information about last location
-   */
-  function updateLastLocation() {
-    lastLocationHref = location.href;
-    lastLocationHash = location.hash;
-  }
 
   /**
    * If location has changed, update the browser
    * This method is called at the end of $eval() phase
    */
   function updateBrowser() {
-    updateLocation();
+    sync();
 
-    if (location.href != lastLocationHref) {    	
-      $browser.setUrl(lastBrowserUrl = location.href);
-      updateLastLocation();
+    if ($browser.getUrl() != location.href) {
+      $browser.setUrl(location.href);
+      copy(location, lastLocation);
     }
   }
 
diff --git a/test/angular-mocks.js b/test/angular-mocks.js
index 65dfb12f..e601d9ab 100644
--- a/test/angular-mocks.js
+++ b/test/angular-mocks.js
@@ -77,6 +77,8 @@ function MockBrowser() {
         }
       }
     );
+
+    return listener;
   };
 
 
diff --git a/test/servicesSpec.js b/test/servicesSpec.js
index 8918f415..eebcf7dc 100644
--- a/test/servicesSpec.js
+++ b/test/servicesSpec.js
@@ -114,7 +114,8 @@ describe("service", function(){
       $location = scope.$service('$location');
     });
 
-    it("update should update location object immediately", function() {
+
+    it("should update location object immediately when update is called", function() {
       var href = 'http://host:123/p/a/t/h.html?query=value#path?key=value&flag&key2=';
       $location.update(href);
       expect($location.href).toEqual(href);
@@ -140,43 +141,28 @@ describe("service", function(){
     });
 
 
-    it('toString() should return actual representation', function() {
-      var href = 'http://host:123/p/a/t/h.html?query=value#path?key=value&flag&key2=';
-      $location.update(href);
-      expect($location.toString()).toEqual(href);
-      scope.$eval();
-
-      $location.host = 'new';
-      $location.path = '';
-      expect($location.toString()).toEqual('http://new:123?query=value#path?key=value&flag&key2=');
-    });
-
-    it('toString() should not update browser', function() {
-      var url = $browser.getUrl();
-      $location.update('http://www.angularjs.org');
-      expect($location.toString()).toEqual('http://www.angularjs.org');
-      expect($browser.getUrl()).toEqual(url);
-    });
-
     it('should update browser at the end of $eval', function() {
-      var url = $browser.getUrl();
+      var origBrowserUrl = $browser.getUrl();
       $location.update('http://www.angularjs.org/');
       $location.update({path: '/a/b'});
-      expect($location.toString()).toEqual('http://www.angularjs.org/a/b');
-      expect($browser.getUrl()).toEqual(url);
+      expect($location.href).toEqual('http://www.angularjs.org/a/b');
+      expect($browser.getUrl()).toEqual(origBrowserUrl);
       scope.$eval();
       expect($browser.getUrl()).toEqual('http://www.angularjs.org/a/b');
     });
 
+
     it('should update hashPath and hashSearch on hash update', function(){
       $location.update('http://server/#path?a=b');
-      scope.$eval();
-      $location.update({hash: ''});
+      expect($location.hashPath).toEqual('path');
+      expect($location.hashSearch).toEqual({a:'b'});
 
+      $location.update({hash: ''});
       expect($location.hashPath).toEqual('');
       expect($location.hashSearch).toEqual({});
     });
 
+
     it('should update hash on hashPath or hashSearch update', function() {
       $location.update('http://server/#path?a=b');
       scope.$eval();
@@ -185,29 +171,37 @@ describe("service", function(){
       expect($location.hash).toEqual('');
     });
 
-    it('should update hashPath and hashSearch on hash property change', function(){
+
+    it('should update hashPath and hashSearch on $location.hash change upon eval', function(){
       $location.update('http://server/#path?a=b');
       scope.$eval();
+
       $location.hash = '';
+      scope.$eval();
 
-      expect($location.toString()).toEqual('http://server/');
+      expect($location.href).toEqual('http://server/');
       expect($location.hashPath).toEqual('');
       expect($location.hashSearch).toEqual({});
     });
 
-    it('should update hash on hashPath or hashSearch property change', function() {
+
+    it('should update hash on $location.hashPath or $location.hashSearch change upon eval',
+        function() {
       $location.update('http://server/#path?a=b');
       scope.$eval();
       $location.hashPath = '';
       $location.hashSearch = {};
 
-      expect($location.toString()).toEqual('http://server/');
+      scope.$eval();
+
+      expect($location.href).toEqual('http://server/');
       expect($location.hash).toEqual('');
     });
 
-    it('should update hash before any processing', function(){
-      scope = compile('<div>');
-      scope.$location = scope.$service('$location');
+
+    it('should sync $location upon eval before watches are fired', function(){
+      scope.$location = scope.$service('$location'); //publish to the scope for $watch
+
       var log = '';
       scope.$watch('$location.hash', function(){
         log += this.$location.hashPath + ';';
@@ -217,48 +211,102 @@ describe("service", function(){
       log = '';
       scope.$location.hash = '/abc';
       scope.$eval();
+      expect(scope.$location.hash).toEqual('/abc');
       expect(log).toEqual('/abc;');
     });
 
-    it('udpate() should accept hash object and update only given properties', function() {
-      $location.update("http://host:123/p/a/t/h.html?query=value#path?key=value&flag&key2=");
-      $location.update({host: 'new', port: 24});
 
-      expect($location.host).toEqual('new');
-      expect($location.port).toEqual(24);
-      expect($location.protocol).toEqual('http');
-      expect($location.href).toEqual("http://new:24/p/a/t/h.html?query=value#path?key=value&flag&key2=");
-    });
+    describe('sync', function() {
+      it('should update hash with escaped hashPath', function() {
+        $location.hashPath = 'foo=bar';
+        scope.$eval();
+        expect($location.hash).toBe('foo%3Dbar');
+      });
 
-    it('updateHash() should accept one string argument to update path', function() {
-      $location.updateHash('path');
-      expect($location.hash).toEqual('path');
-      expect($location.hashPath).toEqual('path');
-    });
 
-    it('updateHash() should accept one hash argument to update search', function() {
-      $location.updateHash({a: 'b'});
-      expect($location.hash).toEqual('?a=b');
-      expect($location.hashSearch).toEqual({a: 'b'});
-    });
+      it('should give $location.href the highest precedence', function() {
+        $location.hashPath = 'hashPath';
+        $location.hashSearch = {hash:'search'};
+        $location.hash = 'hash';
+        $location.port = '333';
+        $location.host = 'host';
+        $location.href = 'https://hrefhost:23/hrefpath';
+
+        scope.$eval();
+
+        expect($location).toEqualData({href: 'https://hrefhost:23/hrefpath',
+                                       protocol: 'https',
+                                       host: 'hrefhost',
+                                       port: '23',
+                                       path: '/hrefpath',
+                                       search: {},
+                                       hash: '',
+                                       hashPath: '',
+                                       hashSearch: {}
+                                      });
+      });
 
-    it('updateHash() should accept path and search both', function() {
-      $location.updateHash('path', {a: 'b'});
-      expect($location.hash).toEqual('path?a=b');
-      expect($location.hashSearch).toEqual({a: 'b'});
-      expect($location.hashPath).toEqual('path');
+
+      it('should give $location.hash second highest precedence', function() {
+        $location.hashPath = 'hashPath';
+        $location.hashSearch = {hash:'search'};
+        $location.hash = 'hash';
+        $location.port = '333';
+        $location.host = 'host';
+        $location.path = '/path';
+
+        scope.$eval();
+
+        expect($location).toEqualData({href: 'http://host:333/path#hash',
+                                       protocol: 'http',
+                                       host: 'host',
+                                       port: '333',
+                                       path: '/path',
+                                       search: {},
+                                       hash: 'hash',
+                                       hashPath: 'hash',
+                                       hashSearch: {}
+                                      });
+      });
     });
-    
-    it('should remove # if hash is empty', function() {
-      $location.update('http://www.angularjs.org/index.php#');
-      expect($location.href).toEqual('http://www.angularjs.org/index.php');
+
+    describe('update()', function() {
+      it('should accept hash object and update only given properties', function() {
+        $location.update("http://host:123/p/a/t/h.html?query=value#path?key=value&flag&key2=");
+        $location.update({host: 'new', port: 24});
+
+        expect($location.host).toEqual('new');
+        expect($location.port).toEqual(24);
+        expect($location.protocol).toEqual('http');
+        expect($location.href).toEqual("http://new:24/p/a/t/h.html?query=value#path?key=value&flag&key2=");
+      });
+
+      it('should remove # if hash is empty', function() {
+        $location.update('http://www.angularjs.org/index.php#');
+        expect($location.href).toEqual('http://www.angularjs.org/index.php');
+      });
     });
-    
-    it('should not change browser\'s url with empty hash', function() {
-      $browser.setUrl('http://www.angularjs.org/index.php#');
-      spyOn($browser, 'setUrl');
-      $browser.poll();
-      expect($browser.setUrl).not.toHaveBeenCalled();
+
+
+    describe('updateHash()', function() {
+      it('should accept single string argument to update path', function() {
+        $location.updateHash('path');
+        expect($location.hash).toEqual('path');
+        expect($location.hashPath).toEqual('path');
+      });
+
+      it('should accept single object argument to update search', function() {
+        $location.updateHash({a: 'b'});
+        expect($location.hash).toEqual('?a=b');
+        expect($location.hashSearch).toEqual({a: 'b'});
+      });
+
+      it('should accept path string and search object arguments to update both', function() {
+        $location.updateHash('path', {a: 'b'});
+        expect($location.hash).toEqual('path?a=b');
+        expect($location.hashSearch).toEqual({a: 'b'});
+        expect($location.hashPath).toEqual('path');
+      });
     });
   });