Merge pull request #1093 from atom/ns/fix-flakys

Overhaul height invalidation
atom · Jun 26, 2019 · 2327c9c · 2327c9c
2 parents 75dcfbe + 694bcb0
commit 2327c9c
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 89 deletions.
diff --git a/lib/project/list-view.js b/lib/project/list-view.js
@@ -1,6 +1,5 @@
 const etch = require('etch');
 const $ = etch.dom;
-const resizeDetector = require('element-resize-detector');
 
 module.exports = class ListView {
   constructor({items, heightForItem, itemComponent, className}) {
@@ -12,7 +11,8 @@ module.exports = class ListView {
     this.previousClientHeight = 0
     etch.initialize(this);
 
-    resizeDetector({strategy: 'scroll'}).listenTo(this.element, () => etch.update(this));
+    const resizeObserver = new ResizeObserver(() => etch.update(this));
+    resizeObserver.observe(this.element);
     this.element.addEventListener('scroll', () => etch.update(this));
   }
 

diff --git a/lib/project/results-view.js b/lib/project/results-view.js
@@ -10,7 +10,6 @@ const {
 
 const ListView = require('./list-view');
 const etch = require('etch');
-const resizeDetector = require('element-resize-detector');
 const binarySearch = require('binary-search')
 
 const path = require('path');
@@ -31,9 +30,6 @@ class ResultsView {
     this.model = model;
     this.pixelOverdraw = 100;
 
-    this.resolveHeightInvalidationPromise = null
-    this.heightInvalidationPromise = new Promise((resolve) => { this.resolveHeightInvalidationPromise = resolve })
-
     this.resultRowGroups = Object.values(model.results).map(result =>
       new ResultRowGroup(result, this.model.getFindOptions())
     )
@@ -63,10 +59,8 @@ class ResultsView {
 
     etch.initialize(this);
 
-    resizeDetector({strategy: 'scroll'}).listenTo(
-      this.element,
-      this.invalidateItemHeights.bind(this)
-    );
+    const resizeObserver = new ResizeObserver(this.invalidateItemHeights.bind(this));
+    resizeObserver.observe(this.element);
     this.element.addEventListener('mousedown', this.handleClick.bind(this));
 
     this.subscriptions = new CompositeDisposable(
@@ -190,7 +184,6 @@ class ResultsView {
       this.contextRowHeight = contextRowHeight;
       this.clientHeight = clientHeight;
       await etch.update(this);
-      this.resolveHeightInvalidationPromise();
     }
 
     etch.update(this);

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -32,7 +32,6 @@
   },
   "dependencies": {
     "binary-search": "^1.3.3",
-    "element-resize-detector": "^1.1.10",
     "etch": "0.9.3",
     "fs-plus": "^3.0.0",
     "temp": "^0.8.3",

diff --git a/spec/project-find-view-spec.js b/spec/project-find-view-spec.js
@@ -22,10 +22,6 @@ describe(`ProjectFindView (ripgrep=${ripgrep})`, () => {
   function getExistingResultsPane() {
     const pane = atom.workspace.paneForURI(ResultsPaneView.URI);
     if (pane) {
-
-      // Allow element-resize-detector to perform batched measurements
-      advanceClock(1);
-
       return pane.itemForURI(ResultsPaneView.URI);
     }
   }
@@ -303,7 +299,6 @@ describe(`ProjectFindView (ripgrep=${ripgrep})`, () => {
           await searchPromise;
 
           const resultsView = getResultsView();
-          await resultsView.heightInvalidationPromise
           expect(resultsView.element).toBeVisible();
           expect(resultsView.refs.listView.element.querySelectorAll(".match-row")).toHaveLength(2);
         })
@@ -317,7 +312,6 @@ describe(`ProjectFindView (ripgrep=${ripgrep})`, () => {
           await searchPromise;
 
           const resultsView = getResultsView();
-          await resultsView.heightInvalidationPromise
           expect(resultsView.element).toBeVisible();
           expect(resultsView.refs.listView.element.querySelectorAll(".match-row")).toHaveLength(1);
         });
@@ -329,7 +323,6 @@ describe(`ProjectFindView (ripgrep=${ripgrep})`, () => {
           await searchPromise;
 
           const resultsView = getResultsView();
-          await resultsView.heightInvalidationPromise
           expect(resultsView.element).toBeVisible();
           expect(resultsView.refs.listView.element.querySelectorAll(".match-row")).toHaveLength(2);
           expect(resultsView.refs.listView.element.querySelectorAll(".match.highlight-info")).toHaveLength(3);
@@ -342,7 +335,6 @@ describe(`ProjectFindView (ripgrep=${ripgrep})`, () => {
           await searchPromise;
 
           const resultsView = getResultsView();
-          await resultsView.heightInvalidationPromise
           expect(resultsView.element).toBeVisible();
           expect(resultsView.refs.listView.element.querySelectorAll(".match-row")).toHaveLength(1);
         });
@@ -714,7 +706,6 @@ describe(`ProjectFindView (ripgrep=${ripgrep})`, () => {
         await waitForSearchResults();
 
         const resultsView = getResultsView();
-        await resultsView.heightInvalidationPromise
         expect(resultsView.element).toBeVisible();
         expect(resultsView.refs.listView.element.querySelectorAll(".match-row")).toHaveLength(11);
         expect(resultsView.refs.listView.element.querySelectorAll(".match.highlight-info")).toHaveLength(13);
@@ -776,7 +767,6 @@ describe(`ProjectFindView (ripgrep=${ripgrep})`, () => {
           const resultsView = getResultsView();
           const resultsPaneView = getExistingResultsPane();
 
-          await resultsView.heightInvalidationPromise
           expect(resultsView.element).toBeVisible();
           expect(resultsView.refs.listView.element.querySelectorAll(".match-row")).toHaveLength(11);
           expect(resultsView.refs.listView.element.querySelectorAll(".match.highlight-info")).toHaveLength(13);
@@ -805,7 +795,6 @@ describe(`ProjectFindView (ripgrep=${ripgrep})`, () => {
           const listView = resultsView.refs.listView;
           const resultsPaneView = getExistingResultsPane();
 
-          await resultsView.heightInvalidationPromise
           expect(listView.element.querySelectorAll(".match-row")).toHaveLength(11);
           expect(listView.element.querySelectorAll(".match.highlight-info")).toHaveLength(13);
           expect(resultsPaneView.refs.previewCount.textContent).toBe("13 results found in 2 files for items");
@@ -851,7 +840,6 @@ describe(`ProjectFindView (ripgrep=${ripgrep})`, () => {
           const resultsView = getResultsView();
           const resultsPaneView = getExistingResultsPane();
 
-          await resultsView.heightInvalidationPromise
           expect(resultsView.refs.listView.element.querySelectorAll(".list-item")).toHaveLength(13);
           expect(resultsPaneView.refs.previewCount.textContent).toBe("13 results found in 2 files for items");
 

diff --git a/spec/results-view-spec.js b/spec/results-view-spec.js
@@ -24,48 +24,48 @@ describe('ResultsView', () => {
 
   function getResultsPane() {
     let pane = atom.workspace.paneForURI(ResultsPaneView.URI);
-
-    // Allow element-resize-detector to perform batched measurements
-    advanceClock(1);
-
     if (pane) return pane.itemForURI(ResultsPaneView.URI);
   }
 
   function getResultsView() {
     return getResultsPane().refs.resultsView;
   }
 
-  function buildResultsView() {
+  function buildResultsView(options = {}) {
     const FindOptions = require("../lib/find-options")
     const ResultsModel = require("../lib/project/results-model")
     const { Result } = ResultsModel
     const ResultsView = require("../lib/project/results-view")
     const model = new ResultsModel(new FindOptions({}), null)
     const resultsView = new ResultsView({ model })
-    model.addResult("/a/b.txt", Result.create({
-      filePath: "/a/b.txt",
-      matches: [
-        {
-          lineText: "hello world",
-          matchText: "world",
-          range: {start: {row: 0, column: 6}, end: {row: 0, column: 11}},
-          leadingContextLines: [],
-          trailingContextLines: []
-        }
-      ]
-    }))
-    model.addResult("/c/d.txt", Result.create({
-      filePath: "/c/d.txt",
-      matches: [
-        {
-          lineText: "goodnight moon",
-          matchText: "night",
-          range: {start: {row: 0, column: 4}, end: {row: 0, column: 8}},
-          leadingContextLines: [],
-          trailingContextLines: []
-        }
-      ]
-    }))
+
+    if (!options.empty) {
+      model.addResult("/a/b.txt", Result.create({
+        filePath: "/a/b.txt",
+        matches: [
+          {
+            lineText: "hello world",
+            matchText: "world",
+            range: {start: {row: 0, column: 6}, end: {row: 0, column: 11}},
+            leadingContextLines: [],
+            trailingContextLines: []
+          }
+        ]
+      }))
+      model.addResult("/c/d.txt", Result.create({
+        filePath: "/c/d.txt",
+        matches: [
+          {
+            lineText: "goodnight moon",
+            matchText: "night",
+            range: {start: {row: 0, column: 4}, end: {row: 0, column: 8}},
+            leadingContextLines: [],
+            trailingContextLines: []
+          }
+        ]
+      }))
+    }
+
     return resultsView
   }
 
@@ -97,7 +97,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
       expect(resultsView.refs.listView.element.querySelector('.path-name').textContent).toBe("one-long-line.coffee");
       expect(resultsView.refs.listView.element.querySelectorAll('.preview').length).toBe(1);
       expect(resultsView.refs.listView.element.querySelector('.preview').textContent).toBe('test test test test test test test test test test test a b c d e f g h i j k l abcdefghijklmnopqrstuvwxyz');
@@ -116,7 +115,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
       expect(resultsView.refs.listView.element.querySelector('.path-name').textContent).toBe(path.join("project", "one-long-line.coffee"));
     });
   });
@@ -135,7 +133,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
       expect(resultsView.refs.listView.element.querySelector('.path-name').textContent).toBe("one-long-line.coffee");
       expect(resultsView.refs.listView.element.querySelectorAll('.preview').length).toBe(1);
       expect(resultsView.refs.listView.element.querySelector('.match').textContent).toBe('ghijkl');
@@ -147,7 +144,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
       expect(resultsView.refs.listView.element.querySelector('.match').textContent).toBe('ghijkl');
       expect(resultsView.refs.listView.element.querySelector('.match')).toHaveClass('highlight-info');
       expect(resultsView.refs.listView.element.querySelector('.replacement').textContent).toBe('');
@@ -179,7 +175,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
       const listElement = resultsView.refs.listView.element;
       expect(listElement.querySelectorAll('.match')[0].textContent).toBe('function ()');
       expect(listElement.querySelectorAll('.replacement')[0].textContent).toBe('() =>');
@@ -198,7 +193,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
       const {listView} = resultsView.refs;
       expect(listView.element.scrollTop).toBe(0);
       expect(listView.element.scrollHeight).toBeGreaterThan(listView.element.offsetHeight);
@@ -303,7 +297,6 @@ describe('ResultsView', () => {
       projectFindView.confirm();
       await searchPromise;
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
     });
 
     it("selects the first/last item when core:move-to-top/move-to-bottom is triggered", async () => {
@@ -343,7 +336,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
 
       resultsView.moveDown();
       resultsView.moveDown();
@@ -382,7 +374,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
 
       await resultsView.collapseResult();
       expect(resultsView.element.querySelector('.collapsed')).not.toBe(null);
@@ -450,7 +441,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
       resultsView.selectFirstResult();
     });
 
@@ -546,7 +536,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
 
       let {length: resultCount} = resultsView.refs.listView.element.querySelectorAll(".match-row");
       expect(resultCount).toBe(11);
@@ -594,7 +583,6 @@ describe('ResultsView', () => {
         atom.commands.dispatch(projectFindView.element, 'core:confirm');
         await searchPromise;
         resultsView = getResultsView();
-        await resultsView.heightInvalidationPromise;
       });
 
       it("shows the preview-controls", () => {
@@ -660,11 +648,7 @@ describe('ResultsView', () => {
 
   describe("when the results view is empty", () => {
     it("ignores core:confirm and other commands for selecting results", async () => {
-      projectFindView.findEditor.setText('thiswillnotmatchanythingintheproject');
-      atom.commands.dispatch(projectFindView.element, 'core:confirm');
-      await searchPromise;
-      resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
+      const resultsView = buildResultsView({ empty: true });
       atom.commands.dispatch(resultsView.element, 'core:confirm');
       atom.commands.dispatch(resultsView.element, 'core:move-down');
       atom.commands.dispatch(resultsView.element, 'core:move-up');
@@ -675,10 +659,8 @@ describe('ResultsView', () => {
     });
 
     it("won't show the preview-controls", async () => {
-      projectFindView.findEditor.setText('thiswillnotmatchanythingintheproject');
-      atom.commands.dispatch(projectFindView.element, 'core:confirm');
-      await searchPromise;
-      expect(getResultsPane().refs.previewControls.style.display).toBe('none');
+      const resultsPane = new ResultsPaneView();
+      expect(resultsPane.refs.previewControls.style.display).toBe('none');
     });
   });
 
@@ -759,7 +741,6 @@ describe('ResultsView', () => {
         await searchPromise;
 
         resultsView = getResultsView();
-        await resultsView.heightInvalidationPromise;
         let fileIconClasses = Array.from(resultsView.refs.listView.element.querySelectorAll('.path-row .icon')).map(el => el.className);
         expect(fileIconClasses).toContain('first-icon-class second-icon-class icon');
         expect(fileIconClasses).toContain('third-icon-class fourth-icon-class icon');
@@ -939,7 +920,6 @@ describe('ResultsView', () => {
       await searchPromise;
 
       resultsView = getResultsView();
-      await resultsView.heightInvalidationPromise;
     });
 
     it('maintains selected result when adding and removing results', async () => {