Consume File-Icons' element-icon service

[Alhadis]

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#1009
• atom/tabs#412
• atom/find-and-replace#870
• atom/fuzzy-finder#281
• atom/archive-view#47

[lee-dohm]

I've been watching this develop, the snide comments toward the Atom maintainers on various issues. I actually had no idea that anything was wrong until I saw that you had closed a bunch of PRs with various obviously frustrated comments. Did you not understand that you were the biggest driver of Atom's file icon service? If the current file icon service isn't useful, you were as much a part of that as anyone else, possibly more so.

But after all the sarcastic comments and the ill feelings, now you want us to work with you again? What was it you said when we made a decision you didn't like? Ah yes, "But I can't shake the feeling [this is a] temporary [solution] until something else changes for no logical reason."

Regardless, I'm curious to know what the benefit is here. So if you wouldn't mind explaining exactly what your thought processes were on making this change, I'll take a look and see about bringing it to the maintainer team.

This is an official warning though that your various actions, snide and backhanded comments toward us I consider violations of the Atom Code of Conduct. We have enough going on without having to deal with you slinging mud at us when we do something you don't agree with.

[Alhadis]

@lee-dohm As a follow up to my e-mail to you, I've rewritten the PR with a much clearer/politer description. :)

/cc @as-cii in case he has input

[Alhadis]

@lee-dohm You may wish to edit the body of your reply, since it now looks... really strange following my revised OP.

[mehcode]

@Alhadis From reading your post and looking at the existing file icons service, a passerby like me can't tell why you need this. All I know from reading things that are linked and this PR is that its a dynamic icon service.

Why does it need to be dynamic? What use case do we achieve that a class name based on a file path does not? I don't really want my icons blinking at me. And icons don't tend to change from anecdotal use.

I'm coming into this wanting Atom to be awesome and if this needs to get merged I'll fight for it too, but I just don't understand why its needed.

[Alhadis]

What use case do we achieve that a class name based on a file path does not?

Because not every icon is assigned by path. The package has six default matching strategies, four of which involve asynchronous I/O. Having no ability to affect the DOM outside the current event loop makes it impossible to implement these strategies through sane means.

[mehcode]

Ok. I think I completely understand the situation now.

---

Here is my proposal:

// atom.file-icons
// 2.0.0
class FileIconService {
  iconClassNamesForPath(filePath: string): Promise<Array<string>>
}

That's it. If a file icon service isn't being async Promise.resolve( ... ) should work well.

I went through the file icons package carefully and that should be enough to do everything you want. We really want async not the DOM node. The service in the PR satisfies the use case but I think exposes way more than it should. This is core and services should only do and expose what is absolutely necessary.

Thoughts @Alhadis @lee-dohm ?

---

This would be very easy to integrate into existing packages. It would also get rid of the flicker that file-icons currently has. Just need .then and to set classes after the promise resolves. I'd also make sure to reserve the display space.

tree-view/lib/file-view.coffee

Lines 23 to 27 in 411f199

	iconClass = FileIcons.getService().iconClassForPath(@file.path, "tree-view")
	if iconClass
	unless Array.isArray iconClass
	iconClass = iconClass.toString().split(/\s+/g)
	@fileName.classList.add(iconClass...)

Keep the context parameter if you think it's really needed but nothing uses it on github (apart from one place in your package that is a hack comment so it doesn't look like its really needed). Looking at the original motivation for this feature (tree view icon colors if modified) that's easily done by some custom css and matching status-*. Oh I saw something that used it. The tabs package uses it to apparently not show icons in the MRU switcher. That's a bit odd.

[Alhadis]

First of all, if there's a display issue concerning icon flickering, please report it to the package repository. Don't just take things like these for granted when they clearly need fixing. FYI, you can also use the package's settings to disable async matching strategies, so everything is matched linearly.

Now, about this:

We really want async not the DOM node.

If this could be solved using a single async op, I obviously would've done that a year ago.

This is core and services should only do and expose what is absolutely necessary.

We're not asking them to alter their service. We're requesting they use ours. Community code != core code. Two completely different things.

I went through the file icons package carefully

It sounds like you've skimmed the consumers and strategy directories, and perhaps neglected the other half of the code I wrote. Which is where most of the heavy lifting is being done.

[mehcode]

If this could be solved using a single async op, I obviously would've done that a year ago.

I'm not trying to start an argument here. I'm seriously trying to resolve this issue. I'm sorry if I come off slow here. I haven't looked into your package before.

I'm not sure what you mean by "single async op". Could you elaborate? A Promise instance can be thought of as a stored success and error callback (promise.resolve and promise.reject). You could have a dozen async operations before you call resolve.

Now I didn't think about file modifications (either typing on-the-fly to change a mode line or a file is saved outside of atom). It doesn't look like file-icons handles this at all (which is totally fine, I think this is overkill).

---

First of all, if there's a display issue concerning icon flickering [...]

I was mainly talking about this sad hack: file-icons/atom@890de29

---

We're not asking them to alter their service. We're requesting they use ours. Community code != core code. Two completely different things.

You're asking them to commit to providing you with a physical DOM node. Atom has etch now. New packages are being written with that (from what I can tell). The physical DOM is abstracted away and it'd be a non-insignificant burden to support this API as it stands.

I also (again not trying to be argumentative, just trying to understand the situation) have not yet heard why you need a DOM node. I believe I looked through all the relevant code. You're just applying classes asynchronously.

I also disagree with this not being "core" code. Atom being able to consume this element icon service is something that needs to be documented and taught (and used in several core packages, which makes that code, "core" code). One of the first questions an enterprising atom package dev will have is why does Atom support 2 radically different APIs for file icons?

[Alhadis]

I'm not sure what you mean by "single async op". Could you elaborate? A Promise instance can be thought of as a stored success and error callback (promise.resolve and promise.reject). You could have a dozen async operations before you call resolve.

Can you not lecture me on JavaScript, please? I know this language better than any other. I know damn well what a Promise object is, and how its lifecycle operates. You're coming off condescending much more than helpful.

I was mainly talking about this sad hack: file-icons/atom@890de29

That "sad hack" is one of many sad hacks I've forced myself to do in order to workaround the current limitations of Atom's API.

You're asking them to commit to providing you with a physical DOM node. Atom has etch now. New packages are being written with that (from what I can tell). The physical DOM is abstracted away and it'd a non-insignificant burden to support this API as it stands.

Package authors are under no obligation to use etch whatsoever; it's purely an implementation choice. Same as how authors never had an obligation to use atom-space-pens or whatever other convoluted libraries they were using.

I also (again not trying to be argumentative, just trying to understand the situation) have not yet heard why you need a DOM node. I believe I looked through all the relevant code. You're just applying classes asynchronously.

You said 80% of my code. Was this the 20% you missed? Because if you claim to understand what's going on, you'd know that each instance of an IconDelegate is responsible for maintaining the class-lists of several separate DOM elements, each of which may be removed at any time. Think of the same icon being shown in the TreeView, an opened tab pane, and an opened fuzzy-finder window. A background file-scan picks up on a modeline, and it updates the class-lists of all three currently visible DOM elements.

Getting the picture now?

[mehcode]

Can you not lecture me on JavaScript, please? I know this language better than any other. I know damn well what a Promise object is, and its lifecycle operates. You're coming off condescending much more than helpful.

I'm not trying to be at all. I apologize for coming off that way. I also apologize as I still don't understand why a promise doesn't work for you.

Package authors are under no obligation to use etch whatsoever [...]

Totally agree. I was talking about core packages though as that's what this is all about. Another example is dozens of packages use React which has the same problem of being annoying to expose physical DOM nodes. I'm just getting at the requirement of a needing to send a DOM node is a non starter for me at least.

You said 80% of my code. Was this the 20% you missed? [...]

Yes, I understand that your code is working via a background task that changes several nodes at once as its tracking that in several places. That doesn't mean it has to work that way.

I'm still not getting why we can't:

• Start-up a background worker
• Maintain a promise map keyed by URI
• When a request comes for a file path, start a new job, and return a promise
• When a request comes in for that same file path (tabs package after the tree-view got to you first), return the same promise from the promise map
• Task is done, resolve the promise and both packages now have the icon

[Alhadis]

@mehcode I'm not going to justify my own code's implementation details in a PR you've now irreparably hijacked. Gonna close and reopen a fresh PR, because I've already had enough problems getting the Atom team's attention to this, and you're making this harder for them. They already have more than enough relevant crap to sift through on a daily basis.

/EOT

[mehcode]

@Alhadis would you like to chat on Slack?

I'm not trying to attack you or your code. Just trying to understand.