fix($state): reloadOnSearch should not affect non-search param changes.

The handling of `reloadOnSearch: false` caused ui-router to avoid
reloading the state in more cases than it should. The designed and
documented behavior of this flag is to avoid state reload when the
URL search string changes. It was also avoiding state reload when
the URL path (or any non-search parameters to the state) changed,
and even when state reload was explicitly requested.

This change

- flips the name of shouldTriggerReload (and the accompanying guard
  boolean, skipTriggerReloadCheck) to match the direction of the
  logic: shouldSkipReload and allowSkipReloadCheck

- teaches shouldSkipReload to look at the types of the differing
  parameters, and only skip the reload if the only parameters that
  differ were search parameters

- pulls the test for options.reload to the front of the complex
  boolean expression.

- Updates $state.params and $stateParams when skipping reload

Fixes #1079. Helps with one of the cases broken in #582.

Author: Matt Ginzton

src/state.js

@@ -954,7 +954,7 @@ function $StateProvider(   $urlRouterProvider,   $urlMatcherFactory) {
      *    have not changed, aka a reload of the same state. It differs from reloadOnSearch because you'd
      *    use this when you want to force a reload when *everything* is the same, including search params.
      *    if String, then will reload the state with the name given in reload, and any children.
-     *    if Object, then a stateObj is expected, will reload the state found in stateObj, and any chhildren.
+     *    if Object, then a stateObj is expected, will reload the state found in stateObj, and any children.
      *
      * @returns {promise} A promise representing the state of the new transition. See
      * {@link ui.router.state.$state#methods_go $state.go}.
@@ -1002,7 +1002,6 @@ function $StateProvider(   $urlRouterProvider,   $urlMatcherFactory) {
 
       // Starting from the root of the path, keep all levels that haven't changed
       var keep = 0, state = toPath[keep], locals = root.locals, toLocals = [];
-      var skipTriggerReloadCheck = false;
 
       if (!options.reload) {
         while (state && state === fromPath[keep] && state.ownParams.$$equals(toParams, fromParams)) {
@@ -1020,8 +1019,6 @@ function $StateProvider(   $urlRouterProvider,   $urlMatcherFactory) {
           throw new Error("No such reload state '" + (isString(options.reload) ? options.reload : options.reload.name) + "'");
         }
 
-        skipTriggerReloadCheck = true;
- 
         while (state && state === fromPath[keep] && state !== reloadState) {
           locals = toLocals[keep] = state.locals;
           keep++;
@@ -1034,8 +1031,10 @@ function $StateProvider(   $urlRouterProvider,   $urlMatcherFactory) {
       // TODO: We may not want to bump 'transition' if we're called from a location change
       // that we've initiated ourselves, because we might accidentally abort a legitimate
       // transition initiated from code?
-      if (!skipTriggerReloadCheck && shouldTriggerReload(to, from, locals, options)) {
+      if (shouldSkipReload(to, toParams, from, fromParams, locals, options)) {
         if (to.self.reloadOnSearch !== false) $urlRouter.update();
+        $state.params = toParams;
+        copy($state.params, $stateParams);
         $state.transition = null;
         return $q.when($state.current);
       }
@@ -1429,8 +1428,27 @@ function $StateProvider(   $urlRouterProvider,   $urlMatcherFactory) {
     return $state;
   }
 
-  function shouldTriggerReload(to, from, locals, options) {
-    if (to === from && ((locals === from.locals && !options.reload) || (to.self.reloadOnSearch === false))) {
+  function shouldSkipReload(to, toParams, from, fromParams, locals, options) {
+    // Return true if there are no differences in non-search (path/object) params, false if there are differences
+    function nonSearchParamsEqual(fromAndToState, fromParams, toParams) {
+      // Identify whether all the parameters that differ between `fromParams` and `toParams` were search params.
+      function notSearchParam(key) {
+        return fromAndToState.params[key].location != "search";
+      }
+      var nonQueryParamKeys = fromAndToState.params.$$keys().filter(notSearchParam);
+      var nonQueryParams = pick.apply(this, [fromAndToState.params].concat(nonQueryParamKeys));
+      var nonQueryParamSet = new $$UMFP.ParamSet(nonQueryParams);
+      return nonQueryParamSet.$$equals(fromParams, toParams)
+    }
+
+    // If reload was not explicitly requested
+    // and we're transitioning to the same state we're already in
+    // and    the locals didn't change
+    //     or they changed in a way that doesn't merit reloading
+    //        (reloadOnParams:false, or reloadOnSearch.false and only search params changed)
+    // Then return true.
+    if (!options.reload && to === from &&
+      (locals === from.locals || (to.self.reloadOnSearch === false && nonSearchParamsEqual(from, fromParams, toParams)))) {
       return true;
     }
   }

test/stateSpec.js

@@ -29,6 +29,7 @@ describe('state', function () {
       HH = { parent: H },
       HHH = {parent: HH, data: {propA: 'overriddenA', propC: 'propC'} },
       RS = { url: '^/search?term', reloadOnSearch: false },
+      RSP = { url: '^/:doReload/search?term', reloadOnSearch: false },
       OPT = { url: '/opt/:param', params: { param: "100" } },
       OPT2 = { url: '/opt2/:param2/:param3', params: { param3: "300", param4: "400" } },
       AppInjectable = {};
@@ -55,6 +56,7 @@ describe('state', function () {
       .state('OPT', OPT)
       .state('OPT.OPT2', OPT2)
       .state('RS', RS)
+      .state('RSP', RSP)
 
       .state('home', { url: "/" })
       .state('home.item', { url: "front/:id" })
@@ -212,7 +214,32 @@ describe('state', function () {
       });
       $q.flush();
       expect($location.search()).toEqual({term: 'hello'});
-      expect(called).toBeFalsy();        
+      expect(called).toBeFalsy();
+    }));
+
+    it('updates $stateParams when state.reloadOnSearch=false, and only query params changed', inject(function ($state, $stateParams, $q, $location, $rootScope){
+      initStateTo(RS);
+      $location.search({term: 'hello'});
+      var called;
+      $rootScope.$on('$stateChangeStart', function (ev, to, toParams, from, fromParams) {
+        called = true
+      });
+      $q.flush();
+      expect($stateParams).toEqual({term: 'hello'});
+      expect(called).toBeFalsy();
+    }));
+
+    it('does trigger state change for path params even if reloadOnSearch is false', inject(function ($state, $q, $location, $rootScope){
+      initStateTo(RSP, { doReload: 'foo' });
+      expect($state.params.doReload).toEqual('foo');
+      var called;
+      $rootScope.$on('$stateChangeStart', function (ev, to, toParams, from, fromParams) {
+        called = true
+      });
+      $state.transitionTo(RSP, { doReload: 'bar' });
+      $q.flush();
+      expect($state.params.doReload).toEqual('bar');
+      expect(called).toBeTruthy();
     }));
 
     it('ignores non-applicable state parameters', inject(function ($state, $q) {
@@ -940,6 +967,7 @@ describe('state', function () {
         'OPT',
         'OPT.OPT2',
         'RS',
+        'RSP',
         'about',
         'about.person',
         'about.person.item',