Remove two-way databinding from patch range select

Two-way databinding was causing some cyclical UI updating, resulting in
an incorrect redirect URL. Instead of binding to the patchNum and
basePatchNum in both directions, the patch-range-select fires an event
when it notices a change in the selected patchsets.

Bug: Issue 7336, Issue 7315, Issue 7316
Change-Id: Ifdf476dcf4fa1353d0b280d7e6dd25e4ef40865a

polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js

@@ -506,12 +506,13 @@
         patchNum: value.patchNum,
         basePatchNum: value.basePatchNum || 'PARENT',
       };
+
       this.$.fileList.collapseAllDiffs();
+      this._patchRange = patchRange;
 
       if (this._initialLoadComplete && patchChanged) {
         if (patchRange.patchNum == null) {
           patchRange.patchNum = this.computeLatestPatchNum(this._allPatchSets);
-          this._patchRange = patchRange;
         }
         this._reloadPatchNumDependentResources().then(() => {
           this.$.jsAPI.handleEvent(this.$.jsAPI.EventType.SHOW_CHANGE, {
@@ -523,7 +524,6 @@
       }
 
       this._changeNum = value.changeNum;
-      this._patchRange = patchRange;
       this.$.relatedChanges.clear();
 
       this._reload().then(() => {

polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html

@@ -778,25 +778,6 @@ limitations under the License.
       assert.isNull(element._getUrlParameter('test'));
     });
 
-    test('navigateToChange called when range select changes', () => {
-      element._change = {
-        change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
-        revisions: {
-          rev2: {_number: 2},
-          rev1: {_number: 1},
-          rev13: {_number: 13},
-          rev3: {_number: 3},
-        },
-        status: 'NEW',
-        labels: {},
-      };
-      element._patchRange = {basePatchNum: 1, patchNum: 2};
-      element.$.fileListHeader.patchNum = 3;
-      assert.equal(navigateToChangeStub.callCount, 1);
-      assert.isTrue(navigateToChangeStub.lastCall
-          .calledWithExactly(element._change, 3, 1));
-    });
-
     test('revert dialog opened with revert param', done => {
       sandbox.stub(element.$.restAPI, 'getLoggedIn', () => {
         return Promise.resolve(true);

polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html

@@ -145,8 +145,8 @@ limitations under the License.
               id="rangeSelect"
               comments="[[comments]]"
               change-num="[[changeNum]]"
-              patch-num="{{patchNum}}"
-              base-patch-num="{{basePatchNum}}"
+              patch-num="[[patchNum]]"
+              base-patch-num="[[basePatchNum]]"
               available-patches="[[allPatchSets]]"
               revisions="[[change.revisions]]"
               on-patch-range-change="_handlePatchChange">

polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js

@@ -38,16 +38,8 @@
         type: String,
         notify: true,
       },
-      patchNum: {
-        type: String,
-        notify: true,
-        observer: '_patchOrBaseChanged',
-      },
-      basePatchNum: {
-        type: String,
-        notify: true,
-        observer: '_patchOrBaseChanged',
-      },
+      patchNum: String,
+      basePatchNum: String,
       revisions: Array,
       // Caps the number of files that can be shown and have the 'show diffs' /
       // 'hide diffs' buttons still be functional.
@@ -144,18 +136,11 @@
           this.findSortedIndex(basePatchNum, this.revisions);
     },
 
-    /*
-     * Triggered by _patchNum and _basePatchNum observer, in order to detect if
-     * the patch has been previously set or not. The new patch number is not
-     * explicitly used, because this could be called by either _patchNum or
-     * _basePatchNum's observer. Since the behavior is the same, they are
-     * combined.
-     */
-    _patchOrBaseChanged(patchNew, patchOld) {
-      if (!patchOld) { return; }
-
-      Gerrit.Nav.navigateToChange(this.change, this.patchNum,
-          this.basePatchNum);
+    _handlePatchChange(e) {
+      const {basePatchNum, patchNum} = e.detail;
+      if (this.patchNumEquals(basePatchNum, this.basePatchNum) &&
+          this.patchNumEquals(patchNum, this.patchNum)) { return; }
+      Gerrit.Nav.navigateToChange(this.change, patchNum, basePatchNum);
     },
 
     _handlePrefsTap(e) {

polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_test.html

@@ -216,7 +216,8 @@ limitations under the License.
       };
       element.basePatchNum = 1;
       element.patchNum = 2;
-      element.patchNum = 3;
+
+      element._handlePatchChange({detail: {basePatchNum: 1, patchNum: 3}});
       assert.equal(navigateToChangeStub.callCount, 1);
       assert.isTrue(navigateToChangeStub.lastCall
           .calledWithExactly(element.change, 3, 1));

polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.html

@@ -43,7 +43,8 @@ limitations under the License.
     <span class="patchRange">
       <gr-dropdown-list
           id="basePatchDropdown"
-          value="{{basePatchNum}}"
+          value="[[basePatchNum]]"
+          on-value-change="_handlePatchChange"
           items="[[_baseDropdownContent]]">
       </gr-dropdown-list>
     </span>
@@ -57,7 +58,8 @@ limitations under the License.
     <span class="patchRange">
       <gr-dropdown-list
           id="patchNumDropdown"
-          value="{{patchNum}}"
+          value="[[patchNum]]"
+          on-value-change="_handlePatchChange"
           items="[[_patchDropdownContent]]">
       </gr-dropdown-list>
       <span is="dom-if" if="[[filesWeblinks.meta_b]]" class="filesWeblinks">

polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js

@@ -22,8 +22,8 @@
    *
    * @event patch-range-change
    *
-   * @property {string} leftPatch
-   * @property {string} rightPatch
+   * @property {string} patchNum
+   * @property {string} basePatchNum
    */
 
   Polymer({
@@ -45,14 +45,8 @@
       comments: Array,
       /** @type {{ meta_a: !Array, meta_b: !Array}} */
       filesWeblinks: Object,
-      patchNum: {
-        type: String,
-        notify: true,
-      },
-      basePatchNum: {
-        type: String,
-        notify: true,
-      },
+      patchNum: String,
+      basePatchNum: String,
       revisions: Object,
       _sortedRevisions: Array,
     },
@@ -133,19 +127,6 @@
           this.findSortedIndex(basePatchNum, sortedRevisions);
     },
 
-    // On page load, the dom-if for options getting added occurs after
-    // the value was set in the select. This ensures that after they
-    // are loaded, the correct value will get selected.  I attempted to
-    // debounce these, but because they are detecting two different
-    // events, sometimes the timing was off and one ended up missing.
-    _synchronizeSelectionRight() {
-      this.$.rightPatchSelect.value = this.patchNum;
-    },
-
-    _synchronizeSelectionLeft() {
-      this.$.leftPatchSelect.value = this.basePatchNum;
-    },
-
     // Copied from gr-file-list
     // @todo(beckysiegel) clean up.
     _getCommentsForPath(comments, patchNum, path) {
@@ -219,5 +200,23 @@
           (opt_addFrontSpace ? ' ' : '') +
           rev.description.substring(0, PATCH_DESC_MAX_LENGTH) : '';
     },
+
+    /**
+     * Catches value-change events from the patchset dropdowns and determines
+     * whether or not a patch change event should be fired.
+     */
+    _handlePatchChange(e) {
+      const detail = {patchNum: this.patchNum, basePatchNum: this.basePatchNum};
+      const target = Polymer.dom(e).localTarget;
+
+      if (target === this.$.patchNumDropdown) {
+        detail.patchNum = e.detail.value;
+      } else {
+        detail.basePatchNum = e.detail.value;
+      }
+
+      this.dispatchEvent(
+          new CustomEvent('patch-range-change', {detail, bubbles: false}));
+    },
   });
 })();

polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html

@@ -315,5 +315,22 @@ limitations under the License.
       delete comments['bar'];
       assert.equal(element._computePatchSetCommentsString(comments, 1), '');
     });
+
+    test('patch-range-change fires', () => {
+      const handler = sandbox.stub();
+      element.basePatchNum = 1;
+      element.patchNum = 3;
+      element.addEventListener('patch-range-change', handler);
+
+      element.$.basePatchDropdown._handleValueChange(2, [{value: 2}]);
+      assert.isTrue(handler.calledOnce);
+      assert.deepEqual(handler.lastCall.args[0].detail,
+          {basePatchNum: 2, patchNum: 3});
+
+      // BasePatchNum should not have changed, due to one-way data binding.
+      element.$.patchNumDropdown._handleValueChange('edit', [{value: 'edit'}]);
+      assert.deepEqual(handler.lastCall.args[0].detail,
+          {basePatchNum: 1, patchNum: 'edit'});
+    });
   });
 </script>

polygerrit-ui/app/elements/shared/gr-dropdown-list/gr-dropdown-list.js

@@ -45,6 +45,14 @@
   Polymer({
     is: 'gr-dropdown-list',
 
+    /**
+     * Fired when the selected value changes
+     *
+     * @event value-change
+     *
+     * @property {string|number} value
+     */
+
     properties: {
       /** @type {!Array<!Defs.item>} */
       items: Object,
@@ -98,6 +106,10 @@
       if (!selectedObj) { return; }
       this.text = selectedObj.triggerText? selectedObj.triggerText :
           selectedObj.text;
+      this.dispatchEvent(new CustomEvent('value-change', {
+        detail: {value},
+        bubbles: false,
+      }));
     },
   });
 })();