Consume File-Icons' element-icon service

[Alhadis]

This PR applies to the tabs package what's been described in atom/tree-view#1146 for the tree-view package. The PR's updated wording describes this scenario best, so I'll just copy+paste:

---

I realised too late that your icon-service fulfils a different purpose from what our package needed. We wrote our own instead, which is built on a dubious but (currently) stable foundation of monkey-patches.

Opinions differ on the subject, but mine is that monkey-patching code you didn't write is bad, and should only be used as a last resort (DOM polyfills notwithstanding). Which is precisely what I've been forced to do to get dynamic icon-assignment working.

This needs to be fixed at a formal level, because:

1. Patches don't persist if File-Icons is deactivated and reactivated.
   I've actually chosen not to fix this, because it would step outside the expected lifecycle of the package. E.g., when a user deactivates a package, they expect it to leave no traces in the workspace. I also can't imagine this happening too often, but still...

2. Any innocent change to your packages could break stuff.
   The obvious danger of monkey-patching what wasn't expected or supposed to be changed. We have specs to alert us of breakage, but there's no telling what would be involved in the repair. We both know this is the wrong approach.

Now, I don't know how the Atom team would feel about adding support for third-party package services. Ideally, this would be addressed on the level of Atom's core icon-API. However, the differences of our needs (as well as the specific use-case of our needs) make me hesitant to propose a change to your existing service, especially because @as-cii has stressed he prefers keeping its functionality simple for the time being. In light of that, it would be more appropriate and ergonomic to support a third-party service in the interim, should a mutually-compatible solution be realised in future.

What this service does

The file-icons.element-icons service, when consumed, provides a function to add dynamic icons to DOM elements. The function is to be called by the package on any element that's supposed to represent a filesystem resource (either files, or directories):

let fileIcon = document.querySelector("li.file-entry > span.icon");
addIconToElement(fileIcon, "/path/to/file.txt");

Calling the function returns a Disposable that clears the icon-node instance from memory, which should be done once the view is destroyed.

Note there's no requirement to specify whether a path is a file or directory. Our heavy-duty filesystem API takes care of the heavy lifting... I've even plans to separate it from File-Icons in the form of a standalone Node module, so other package authors can benefit from my hard work too. :)

Related pull-requests

• atom/tree-view#1146
• atom/tabs#412
• atom/find-and-replace#870
• atom/fuzzy-finder#281
• atom/archive-view#47

[gnestor]

+1 for this PR as it fixes an issue with file-icons and project-plus compatibility

[gnestor]

@Alhadis More services!!!

[Alhadis]

/cc @nathansobo PR updated. I doubt it'll take long to review; it follows the same approach as tree-view.

Again, you'll want to squash-merge, unless you want merge-commits clogging the repository's revision history...

[nathansobo]

One thing I just realized... could you add some tests for the service code paths where you sub in a different icon?

[Alhadis]

@nathansobo Will that do?

[nathansobo]

The structure of the code looks good here, but the tests are failing for me locally and on AppVeyor as well.

[Alhadis]

They're all good now. 👍

[nathansobo]

Published as 0.109.0 and upgraded in atom/atom@cffa433. Thanks for incorporating feedback so quickly.