Wait for Etch component to render before consuming

[Alhadis]

Second attempt at fixing atom/atom#16133. See #971 for the original.

@nathansobo I was unable to find a more elegant solution which behaved reliably, so I'm resorting to polling instead. I also took care to avoid changing state by not setting the iconDisposable property when the view is rendering. Disposables are now handled using a Map object instead.

Note that this would've been a perfect opportunity to use atom/event-kit#32.

[Alhadis]

@nathansobo Do you know if this will make the v1.23 release? It'd be wonderful to kill off each of the file-icons consumer classes so I can rework the package infrastructure to load asynchronously.

[nathansobo]

Life has been crazy with the teletype launch and knowing I'm going to need to wrap my head on why we need this polling I haven't pulled it down in the midst of travel and hectic stuff. I think I can probably look into it before the next release though.

[Alhadis]

I understand. The polling is simply a workaround for the fact that DOM elements aren't visible in the workspace DOM at the time the element-icon service is consumed. I'm not familiar with Etch and assumed the element was attached synchronously after the render method was called... so I just relied on process.nextTick to connect the service to the element.

With very long lists, however, it seems as though there's some kind of delay between the virtual-node's creation and when the element is actually visible in the DOM tree... hence the use of "polling".

(I hope that makes sense...)

[leroix]

@Alhadis the fact that getIconClasses is side-effecting seems odd to me. Would etch update hooks be the solution you're looking for? It seems like you could "attach" in a readAfterUpdate hook, and that would eliminate the need for polling.

[Alhadis]

@leroix How does one go about calling readAfterUpdate, then...? I'm having trouble finding it...

[leroix]

ResultView would need to have a readAfterUpdate method. etch will call this method when the update is complete https://github.com/atom/etch/blob/master/lib/component-helpers.js#L138.

[Alhadis]

It's still only updating half of the list's icons:

  /* result-view.js */
  readAfterUpdate() {
    getIconServices().updateIcon(this)
  }

  /* get-icon-services.js */
  updateIcon (view) {
    if (this.elementIcons) {
      if (!view.iconDisposable && view.refs && view.refs.icon) {
        view.iconDisposable = this.elementIcons(view.refs.icon, view.filePath)
        this.elementIconDisposables.add(view.iconDisposable)
      }
    } else {
      let iconClass = this.fileIcons.iconClassForPath(view.filePath, 'find-and-replace') || ''
      if (Array.isArray(iconClass)) {
        iconClass = iconClass.join(' ')
      }
      view.refs.icon.className = iconClass + ' icon'
    }
    etch.update(view)
  }

[Alhadis]

Okay, I feel I've tried everything. Nothing's working 100%...

[leroix]

@Alhadis ok, I'm going to checkout your branch and see if I can get a better idea of what's going on.

[Alhadis]

I've not committed my attempts, because the revision history is chaotic enough already. Here's the diff for my unstaged modifications:

diff --git a/lib/get-icon-services.js b/lib/get-icon-services.js
index de9ea92..6e8f7c8 100644
--- a/lib/get-icon-services.js
+++ b/lib/get-icon-services.js
@@ -1,5 +1,6 @@
 const DefaultFileIcons = require('./default-file-icons')
 const {Emitter, Disposable, CompositeDisposable} = require('atom')
+const etch = require('etch')
 
 let iconServices
 module.exports = function () {
@@ -12,7 +13,6 @@ class IconServices {
     this.emitter = new Emitter()
     this.elementIcons = null
     this.elementIconDisposables = new CompositeDisposable()
-    this.elementIconViews = new Map()
     this.fileIcons = DefaultFileIcons
   }
 
@@ -33,7 +33,6 @@ class IconServices {
       if (this.elementIconDisposables != null) {
         this.elementIconDisposables.dispose()
       }
-      this.elementIconViews.clear()
       if (service) { this.elementIconDisposables = new CompositeDisposable() }
       this.elementIcons = service
       return this.emitter.emit('did-change')
@@ -47,28 +46,19 @@ class IconServices {
     }
   }
 
-  getIconClasses (view) {
-    let iconClass = ''
+  updateIcon (view) {
     if (this.elementIcons) {
-      if (!this.elementIconViews.has(view)) {
-        this.waitToAttach(view).then(() => {
-          const iconDisposable = new CompositeDisposable()
-          this.elementIconViews.set(view, iconDisposable)
-          this.elementIconDisposables.add(iconDisposable)
-          iconDisposable.add(this.elementIcons(view.refs.icon, view.filePath))
-          iconDisposable.add(new Disposable(() => {
-            this.elementIconViews.delete(view)
-            this.elementIconDisposables.delete(view)
-          }))
-        })
+      if (!view.iconDisposable && view.refs && view.refs.icon instanceof Element) {
+        view.iconDisposable = this.elementIcons(view.refs.icon, view.filePath)
+        this.elementIconDisposables.add(view.iconDisposable)
       }
     } else {
-      iconClass = this.fileIcons.iconClassForPath(view.filePath, 'find-and-replace') || ''
+      let iconClass = this.fileIcons.iconClassForPath(view.filePath, 'find-and-replace') || ''
       if (Array.isArray(iconClass)) {
         iconClass = iconClass.join(' ')
       }
+      view.refs.icon.className = iconClass + ' icon'
     }
-    return iconClass
   }
 
   async waitToAttach(view, delay = 10){
diff --git a/lib/project/result-view.js b/lib/project/result-view.js
index 4384bc8..705902d 100644
--- a/lib/project/result-view.js
+++ b/lib/project/result-view.js
@@ -43,6 +43,8 @@ class ResultView {
       leadingContextLineCount, trailingContextLineCount
     } = item;
 
+    getIconServices().updateIcon(this)
+
     const changed =
       matches !== this.matches ||
       isExpanded !== this.isExpanded ||
@@ -75,10 +77,14 @@ class ResultView {
       this.contextLineHeight = contextLineHeight;
       this.leadingContextLineCount = leadingContextLineCount;
       this.trailingContextLineCount = trailingContextLineCount;
-      etch.update(this);
+      etch.update(this).then(() => getIconServices().updateIcon(this))
     }
   }
 
+  readAfterUpdate() {
+    getIconServices().updateIcon(this)
+  }
+
   render() {
     let relativePath = this.filePath;
     if (atom.project) {
@@ -93,9 +99,7 @@ class ResultView {
       !this.isExpanded ||
       this.selectedMatchIndex === -1
     );
-    
-    const iconClass = getIconServices().getIconClasses(this)
-    
+
     return (
       $.li(
         {
@@ -113,7 +117,7 @@ class ResultView {
           $.span({className: 'disclosure-arrow'}),
           $.span({
             ref: 'icon',
-            className: iconClass + ' icon',
+            className: 'icon',
             dataset: {name: path.basename(this.filePath)}
           }),
           $.span({className: 'path-name bright'},

[nathansobo]

In the future, I think it's fine to commit a work-in-progress and we could rebase it out if needed. May as well let the tools do the work.

[Alhadis]

I'll remember that for next time, then.

Do you have any idea what might be going on with the flaky updates?

[nathansobo]

I'm going to let @leroix take a stab at it because I'm drowning in teletype issues.

[Alhadis]

I understand implicitly. :) I appreciate the time you've already spent on this.

[nathansobo]

Of course! You've spent more.

[leroix]

@Alhadis what do you think? Happy to clarify anything in this diff. Note: this is a diff from the head of your branch.

diff --git a/lib/get-icon-services.js b/lib/get-icon-services.js
index de9ea92..6364724 100644
--- a/lib/get-icon-services.js
+++ b/lib/get-icon-services.js
@@ -12,7 +12,6 @@ class IconServices {
     this.emitter = new Emitter()
     this.elementIcons = null
     this.elementIconDisposables = new CompositeDisposable()
-    this.elementIconViews = new Map()
     this.fileIcons = DefaultFileIcons
   }
 
@@ -33,7 +32,6 @@ class IconServices {
       if (this.elementIconDisposables != null) {
         this.elementIconDisposables.dispose()
       }
-      this.elementIconViews.clear()
       if (service) { this.elementIconDisposables = new CompositeDisposable() }
       this.elementIcons = service
       return this.emitter.emit('did-change')
@@ -47,36 +45,22 @@ class IconServices {
     }
   }
 
-  getIconClasses (view) {
-    let iconClass = ''
+  updateIcon (view) {
     if (this.elementIcons) {
-      if (!this.elementIconViews.has(view)) {
-        this.waitToAttach(view).then(() => {
-          const iconDisposable = new CompositeDisposable()
-          this.elementIconViews.set(view, iconDisposable)
-          this.elementIconDisposables.add(iconDisposable)
-          iconDisposable.add(this.elementIcons(view.refs.icon, view.filePath))
-          iconDisposable.add(new Disposable(() => {
-            this.elementIconViews.delete(view)
-            this.elementIconDisposables.delete(view)
-          }))
-        })
+      if (view.refs && view.refs.icon instanceof Element) {
+        if (view.iconDisposable) {
+          view.iconDisposable.dispose()
+          this.elementIconDisposables.remove(view.iconDisposable)
+        }
+        view.iconDisposable = this.elementIcons(view.refs.icon, view.filePath)
+        this.elementIconDisposables.add(view.iconDisposable)
       }
     } else {
-      iconClass = this.fileIcons.iconClassForPath(view.filePath, 'find-and-replace') || ''
+      let iconClass = this.fileIcons.iconClassForPath(view.filePath, 'find-and-replace') || ''
       if (Array.isArray(iconClass)) {
         iconClass = iconClass.join(' ')
       }
-    }
-    return iconClass
-  }
-
-  async waitToAttach(view, delay = 10){
-    if (view.refs && view.refs.icon instanceof Element) {
-      return Promise.resolve()
-    } else {
-      await new Promise(done => setTimeout(done, delay))
-      return this.waitToAttach(view, delay)
+      view.refs.icon.className = iconClass + ' icon'
     }
   }
 }
diff --git a/lib/project/result-view.js b/lib/project/result-view.js
index 4384bc8..c244b09 100644
--- a/lib/project/result-view.js
+++ b/lib/project/result-view.js
@@ -30,6 +30,7 @@ class ResultView {
     this.replacePattern = replacePattern;
     this.previewStyle = previewStyle;
     etch.initialize(this);
+    getIconServices().updateIcon(this);
   }
 
   destroy() {
@@ -75,10 +76,14 @@ class ResultView {
       this.contextLineHeight = contextLineHeight;
       this.leadingContextLineCount = leadingContextLineCount;
       this.trailingContextLineCount = trailingContextLineCount;
-      etch.update(this);
+      etch.update(this)
     }
   }
 
+  writeAfterUpdate() {
+    getIconServices().updateIcon(this);
+  }
+
   render() {
     let relativePath = this.filePath;
     if (atom.project) {
@@ -93,9 +98,7 @@ class ResultView {
       !this.isExpanded ||
       this.selectedMatchIndex === -1
     );
-    
-    const iconClass = getIconServices().getIconClasses(this)
-    
+
     return (
       $.li(
         {
@@ -113,7 +116,7 @@ class ResultView {
           $.span({className: 'disclosure-arrow'}),
           $.span({
             ref: 'icon',
-            className: iconClass + ' icon',
+            className: 'icon',
             dataset: {name: path.basename(this.filePath)}
           }),
           $.span({className: 'path-name bright'},

[Alhadis]

PERFECT! 😀 Thank you so much!

Both find-and-replace and file-icons pass their test suites perfectly.

[leroix]

@nathansobo any thoughts before I merge this?

[nathansobo]

I think this is good. The writeAfterUpdate hook is a good place to tack on these kinds of imperative DOM updates. Thanks for jumping in on this @leroix and taking the time to get it right @Alhadis.