Consume File-Icons' element-icon service

[Alhadis]

Redux of #1009 after the original PR, copied below, was derailed by a (now blocked) user.

Just to clarify, this is integrating Atom's core with a community package, rather than the other way around. Though it seems strange, it is being handled smoothly through the service-hub. So if this package magically disappears all of a sudden, no Atom packages will break - it'll just mean there's no service-provider offering the element-icons service.

---

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

[Alhadis]

Should this be merged, please squash-merge, for the love of everything that's good and sacred:

On the plus side, this PR is nearly a year old. Happy birthday, tree-view. 🍾

[as-cii]

Apologies for the delay on this, @Alhadis.

@iolsen @ungb: can we prioritize this?

[nathansobo]

This is going to be cool. I agree 100% that monkey patching should be replaced by services, and you're doing the exact right thing here from ecosystem standpoint. Bravo.

I have some questions about the specific implementation. One is inline about the IconServices object. I'm also curious why you structured the service to imperatively assign an icon on a DOM element rather than returning it. Is it because you want to adjust other properties of the containing element? Could these be returned as an object or something?

My main concern is someday we want to move this old tree view code to more of a declarative virtual-DOM based structure. Accessing the icon as a value rather than an imperative function would probably make that transition easier. Forgive me if you've already explained your reasoning for the imperative API elsewhere and I've missed it.

[nathansobo]

Do you use the flexibility of the dynamic service naming in this class? If not, I think I'd prefer explicit methods for setting the file icons and element icons services just to keep things super clear.

[Alhadis]

I'm pretty sure this class can be axed, actually. I kept it because our current implementation uses it: lib/file-icons.coffee. I renamed it IconServices to disambiguate. This class isn't necessary to integrate with our package, and we can easily do without it.

[Alhadis]

Oh! Wait, no, scratch that. I forgot the IconServices class is being used to "select" the most appropriate icon service that's available. Where file-icons is, confusingly enough, the old, pre-2017 service that was too limited for the file-icons package, and element-icons is the service which points to, well, file-icons's service.

Confusing, eh? 😀

[Alhadis]

I'm also curious why you structured the service to imperatively assign an icon on a DOM element rather than returning it. Is it because you want to adjust other properties of the containing element? Could these be returned as an object or something?

We use hard DOM references in order to update them in future when changes are detected (such as when a hashbang or modeline is picked up in a file's header, or when a user changes the package settings). We don't return references to an Icon instance because, quite simply, that would be doing things inside-out. The file-icons package handles all the icon updating and filesystem change detection, and stuff like that...

I've kept our implementation as minimalist as possible, and it's working perfectly fine. I'm not sure what it could be lacking...?

[nathansobo]

Okay, cool. I can see the logic in inverting control and letting the service reassign the icon as needed. If you want to eliminate the unnecessary flexibility around managing icons of different types with that class, I'm down to merge the imperative implementation and see how it goes.

[Alhadis]

Sorry, scratch that. I remember now what the IconServices class was used for: if the file-icons package is deactivated, the disposable instructs tree-view to use the current/built-in/limited icon-service for whatever other packages happen to be providing it.

want to eliminate the unnecessary flexibility around managing icons of different types with that class

I'm not sure what you mean. The class is essentially a fractured switch statement. All it's doing is holding a pointer to whatever service the tree-view package was able to grab to provide icons. The real complexity of selecting icons (and I do mean complex) is handled internally by file-icons.

[nathansobo]

I'm not sure what you mean. The class is essentially a fractured switch statement. All it's doing is holding a pointer to whatever service the tree-view package was able to grab to provide icons.

I just think the interface of the IconServices class could be simplified... we don't need to eliminate it entirely. There are these get and set methods that take strings but I only see them being called with two different hard-coded strings in the rest of the code. If that class is all about selecting between two services for backward compatibility, why not fully encapsulate that concern in the class?

It could have a method like updateIcon that took the element in question and then decided which service to use to update it. If it needed to use the older service it could manipulate the element's class itself. Then the set method could be replaced with 2 service-specific methods, setElementIconsService and setFileIconsService.

In general I'd really like to avoid methods that take strings because you can't tell by looking at the method how many possible strings might be passed in.

[Alhadis]

Fair enough. I'll inline it then. I guess a circular dependency between DirectoryView and the TreeViewPackage class won't hurt...

Also, just to help you understand what's going on under the hood on our side, here's a grossly simplified diagram illustrating what the package is doing when it's consuming the service.

Blue stuff is async.

[nathansobo]

I'm not sure what you mean by inline it... I just think you could simplify the interface and make it more specific and less general, as in not take strings. And maybe push more logic about deciding between the two services into it while you're at it.

That's a really nice diagram! What did you make it in?

[Alhadis]

@nathansobo Concerning your recommendation with the updateIcon method... how do you propose I handle this logic? It sounded like you wanted the bulk of the class-assignment logic handled in the TreeViewPackage controller, but it feels counter-intuitive for a DirectoryView instance to pass itself as an argument to TreeViewPackage.updateIcon and have TreeViewPackage inspect its properties before adding an icon

EDIT: Wait, never mind. I misunderstood. My head is all over the place tonight...

[nathansobo]

Yeah, give it a try and see if we can simplify this interface to just be about assigning services and updating the icon with the best service. I think we're pretty close here.

[Alhadis]

Ah right, I misunderstood. Sorry.

That's a really nice diagram! What did you make it in?

Heh. I just hacked it together in Adobe Illustrator. :) My background is graphic design/multimedia. Somehow I ended up in web programming instead.

[Alhadis]

Alrighty, how's this look? =)

[nathansobo]

Looks really good. It's easier for me to understand what is going on with less flexibility. Thanks for the quick turnaround! I'll wait on a green build and merge.

[Alhadis]

Thank you so much!!

(Don't forget to squash merge, haha)

[nathansobo]

Normally don't squash-merge, but for you, I did it. 😉

[nathansobo]

Published as 0.221.0 and upgraded to that version in atom/atom@32e9547. Hold on to your butts.

[Alhadis]

We're 20% of the way there. :D Thank you!

I'm currently updating the other PRs to include your earlier requests:

• Consume File-Icons' element-icon service tabs#412
• Consume File-Icons' element-icon service find-and-replace#870
• Consume File-Icons' element-icon service fuzzy-finder#281
• Consume File-Icons' element-icon service archive-view#47

[nathansobo]

Cool. I will add them all to my queue. I need to deal with this file watcher emergency on Linux but then they're next.

[nathansobo]

Hi. I should have asked you to add some tests before merging this. Would you be cool following up with a test that exercises the basic functionality of the icon service case? I'm worried that this could break in the future without any coverage.

[Alhadis]

I'm following up on the others. Sorry about the slowish response, Internet's been crap the last couple days...

[Alhadis]

@nathansobo To test this reliably, we should merge default-file-icons-spec.coffee and file-icons-spec.coffee into one, and do so where the tree-view package has activated so we can test what's happening in the DOM.

At a minimum, I'd like to add what atom/fuzzy-finder#330 adds, but I don't know which spec to add to. tree-view-package-spec.coffee is a mess, and tree-view-spec.js implies that new specs should be added there (what with the long-term move away from CoffeeScript).

What do you recommend?

[nathansobo]

@Alhadis Thanks for asking. I think we should expand on tree-view-spec.js. Our testing approach in a lot of the original packages is way too integrated. There's really no need to activate the entire package in almost 100% of cases. It adds a lot of complexity and makes the tests miserable to maintain. Instead, we should be testing individual components like we are in tree-view-spec.js.

Ideally, the IconServices instance wouldn't be a global singleton, but would instead be injected into the TreeView and then threaded down to any code that needs to access it. That would make it easier to construct a new IconServices instance for each test rather than needing to clear the state of some global in the tests. That said, if you don't have time to change to dependency injection any test that ensures this functionality doesn't regress would be fine.

Thanks again.

[Alhadis]

I'm sorry this took so long. 😞 Submitted in #1198.