Fix duplicate pickers (#735)

* Update .prettierignore

* prettier

* Avoid duplicate gutter markers

To avoid remote, local and merge diff views adding overlapping gutter markers of the same type on the same line. We leave it to CM to handle multiple markers of different types on the same line, but that shouldn't really apply for us.

* Fix GutterMarker.eq for comparison

Use simple identity comparison.

* Update Playwright Snapshots

* Update Playwright Snapshots

* Adjust UI test expected picker numbers

* Update Playwright Snapshots

* Update Playwright Snapshots

* Revert spurious snapshot update

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: krassowski <[email protected]>

.prettierignore

@@ -6,4 +6,5 @@
 **/.ipynb_checkpoints
 **/coverage
 CHANGELOG.md
-package-lock.json
+package-lock.json
+nbdime/tests/files/inline-conflict--decisions.json

packages/nbdime/src/common/mergeview.ts

@@ -295,21 +295,21 @@ const removeGutterMarkerEffect = StateEffect.define<{ type: string }>({
 class MergeMarker extends GutterMarker {
   constructor(options: { symbol: string; className: string; block: boolean }) {
     super();
-    this._symbol = options.symbol;
-    this._className = options.className;
-    this._block = options.block;
-  }
-  get isBlock() {
-    return this._block;
+    this.symbol = options.symbol;
+    this.className = options.className;
+    this.block = options.block;
   }
   toDOM() {
-    let pickerMarker = elt('div', this._symbol);
-    pickerMarker.className = this._className;
+    let pickerMarker = elt('div', this.symbol);
+    pickerMarker.className = this.className;
     return pickerMarker;
   }
-  private _symbol: string;
-  private _className: string;
-  private _block: boolean;
+  eq(other: GutterMarker): boolean {
+    return other === this;
+  }
+  readonly symbol: string;
+  readonly className: string;
+  readonly block: boolean;
 }
 
 /**
@@ -332,7 +332,18 @@ const gutterMarkerField = StateField.define<RangeSet<MergeMarker>>({
               : e.value.block
               ? conflictBlockMarker
               : conflictMarker;
-          gutters = gutters.update({ add: [marker.range(e.value.from)] });
+          // check for overlap (duplicates) with same type
+          let overlap = false;
+          gutters.between(e.value.from, e.value.from, (from, to, value) => {
+            if (from === e.value.from && value.eq(marker)) {
+              overlap = true;
+              return false;
+            }
+            return;
+          });
+          if (!overlap) {
+            gutters = gutters.update({ add: [marker.range(e.value.from)] });
+          }
         }
       }
       if (e.is(removeGutterMarkerEffect)) {
@@ -1417,7 +1428,7 @@ export class MergeView extends Panel {
         class: 'cm-gutter',
         markers: view => {
           return view.state.field(gutterMarkerField).update({
-            filter: (_from, _to, value: MergeMarker) => !value.isBlock,
+            filter: (_from, _to, value: MergeMarker) => !value.block,
           });
         },
         widgetMarker: (
@@ -1430,7 +1441,7 @@ export class MergeView extends Panel {
           }
           const markers = view.state.field(gutterMarkerField).update({
             filter: (from, _to, value: MergeMarker) =>
-              value.isBlock && block.from === from,
+              value.block && block.from === from,
           });
           if (markers.size > 1) {
             throw Error('More than one block gutter widget matched');

packages/nbdime/src/merge/model/cell.ts

@@ -269,7 +269,12 @@ export class CellMergeModel extends ObjectMergeModel<
    */
   get agreedCell(): boolean {
     // TODO: Also check other fields?
-    return this.agreedSource && this.agreedMetadata && this.agreedOutputs && this.agreedIds;
+    return (
+      this.agreedSource &&
+      this.agreedMetadata &&
+      this.agreedOutputs &&
+      this.agreedIds
+    );
   }
 
   /**

packages/nbdime/src/styles/merge.css

@@ -253,13 +253,13 @@
   .cm-merge-m-chunk-end-either-empty::after
   + .cm-linewidget
   .cm-merge-spacer {
-    content: '';
-    width: 100%;
-    height: 1px;
-    background: var(--jp-merge-either-color1);
-    display: block;
-    position: absolute;
-    left: 0px;
+  content: '';
+  width: 100%;
+  height: 1px;
+  background: var(--jp-merge-either-color1);
+  display: block;
+  position: absolute;
+  left: 0px;
 }
 
 .jp-Notebook-merge .cm-central-editor .cm-line .cm-merge-m-deleted,

packages/webapp/src/app/merge.ts

@@ -183,23 +183,26 @@ function onPopState(e: PopStateEvent) {
  */
 function onMergeRequestCompleted(data: any) {
   let layoutWork = showMerge(data);
-  layoutWork.then(() => {
-    toggleSpinner(false);
-    markUnchangedRanges();
-  }, reason => {
-    console.log("Failed to show merge result");
-    let root = document.getElementById('nbdime-root');
-    if (!root) {
-      throw new Error('Missing root element "nbidme-root"');
-    }
-    const pre = document.createElement('pre');
-    pre.innerText = reason || "Error occurred displaying merge!";
-    pre.classList.add("jp-mergeapp-error");
-    // we might have a partial render, so leave that in case it is useful
-    root.prepend(pre);
-    mergeWidget = null;
-    toggleSpinner(false);
-  });
+  layoutWork.then(
+    () => {
+      toggleSpinner(false);
+      markUnchangedRanges();
+    },
+    reason => {
+      console.log('Failed to show merge result');
+      let root = document.getElementById('nbdime-root');
+      if (!root) {
+        throw new Error('Missing root element "nbidme-root"');
+      }
+      const pre = document.createElement('pre');
+      pre.innerText = reason || 'Error occurred displaying merge!';
+      pre.classList.add('jp-mergeapp-error');
+      // we might have a partial render, so leave that in case it is useful
+      root.prepend(pre);
+      mergeWidget = null;
+      toggleSpinner(false);
+    },
+  );
 }
 
 /**

ui-tests/tests/nbdime-merge-test1.spec.ts

@@ -11,7 +11,7 @@ test.describe('merge test1', () => {
   });
 
   test('take a snapshot at opening', async ({ page }) => {
-    await expect.soft(page.getByText('➭')).toHaveCount(12);
+    await expect.soft(page.getByText('➭')).toHaveCount(11);
 
     await expect.soft(page.locator('#nbdime-header-base')).toHaveText('Base');
 
@@ -40,7 +40,7 @@ test.describe('merge test1', () => {
       .locator('.cm-central-editor')
       .nth(1) // This select the cell; 0 being the notebook metadata
       .locator('.jp-Merge-gutter-picker')
-      .nth(2)
+      .nth(1)
       .click();
     await page.getByText('⚠').click();
     expect(await page.locator('#main').screenshot()).toMatchSnapshot();

ui-tests/tests/nbdime-merge-test2.spec.ts

@@ -11,7 +11,7 @@ test.beforeEach(async ({ page }) => {
 /* notebooks of same length with 0 conflict*/
 test.describe('merge test2 ', () => {
   test('take a snapshot at opening', async ({ page }) => {
-    await expect.soft(page.getByText('➭')).toHaveCount(12);
+    await expect.soft(page.getByText('➭')).toHaveCount(11);
     expect(await page.locator('#main').screenshot()).toMatchSnapshot();
   });
 
@@ -30,7 +30,7 @@ test.describe('merge test2 ', () => {
       .locator('.cm-central-editor')
       .nth(1) // This select the cell; 0 being the notebook metadata
       .locator('.jp-Merge-gutter-picker')
-      .nth(2)
+      .nth(1)
       .click();
     expect(await page.locator('#main').screenshot()).toMatchSnapshot();
   });

ui-tests/tests/nbdime-merge-test3.spec.ts

@@ -11,7 +11,7 @@ test.beforeEach(async ({ page }) => {
 /* 2 cells with merge conflict */
 test.describe('merge test3', () => {
   test('should warn for remaining conflicts', async ({ page }) => {
-    await expect.soft(page.getByText('➭')).toHaveCount(26);
+    await expect.soft(page.getByText('➭')).toHaveCount(25);
 
     await page.getByRole('button', { name: 'Download' }).click();