Add consumer for DanBrooker/file-icons@2.0.3

[jacobmischka]

With the recent 2.0 release of file-icons, the icons stopped displaying correctly. This fixes that using their new service. The service is currently active in master, and set to be released with 2.0.3.

Related issue in file-icons: DanBrooker/file-icons#474

[Osmose]

Ohhhh, thanks for the PR!

So, I'm the worst. Why am I the worst?

Because master isn't actually the currently-released version of advanced-open-file, it's the remnants of a failed rewrite that I didn't clean up. react-revert is the branch I've been releasing from, which, as implied by it's name, doesn't include React at all.

This is the second time this has been an issue, which is two too many, so I've merged react-revert into master now. But, due to my negligence, half of this PR is moot. But the non-moot parts look fine, nice work on those!

I took your changes and added a commit that makes them work with the currently-released code, and updated this PR with it, but I appear to have messed something up and way more commits than I intended are showing up. I'll see if I can fix that.

[Osmose]

Okay, I believe this is correct. Hooray for cherry-picking!

@jacobmischka How does my extra commit look to you? It doesn't do anything fancy or super-performant but I think that's fine, there's much-worse bottlenecks than this in advanced-open-file. :P

If you're fine with my messing around with your work (I've still got your old commits locally in case pushing to your branch was a problem), then I'll merge and release this and add you the the AUTHORS file.

[Osmose]

Oh, and the Appveyor test failure is unrelated. Not sure why it's failing but it ain't this PR's fault.

[jacobmischka]

Hm I'm afraid there might be a problem, let me check this out and test it real quick, I'll get back to you in a few minutes.

[jacobmischka]

Okay, it seems good. I was afraid that elements created before consumeElementIcons was called wouldn't be styled, that's why I added the emitter initially. I'm not exactly sure why that's not happening now, but it seems to be fine! Thanks!

[Alhadis]

Ah... I couldn't help noticing the default file-icon class wasn't being removed:

The icon being highlighted above should be showing up as a symlink, but it's being overwritten due to the order of CSS rules in the theme's styling (both icons happen to be defined in Atom's CSS, rather than the package's).

Easily fixed though:

diff --git a/lib/view.js b/lib/view.js
index c2abb15..31057c1 100644
--- a/lib/view.js
+++ b/lib/view.js
@@ -250,6 +250,7 @@ export default class AdvancedOpenFileView {
                 let listItem = dom(this.createPathListItem(path));
                 if (addIconToElement) {
                     let filenameElement = listItem.querySelector('.filename.icon');
+                    filenameElement.classList.remove('icon-file-text');
                     addIconToElement(filenameElement, path.absolute);
                 }
                 this.pathList.appendChild(listItem);

[jacobmischka]

Oh, I didn't know they were supposed to be removed. Should icon-file-directory be removed as well?

[Alhadis]

I think so, yes. File-Icons will automatically fall back to that icon if nothing matches for a directory.

The folders are currently showing up fine, but that'll be because of the order of CSS rules in Atom's core stylesheets, most likely (e.g., icon-file-directory ruleset defined first, followed by icon-file-symlink-directory, etc).

[Alhadis]

I should probably include a note in the readme which explains this detail, actually. The service purposefully doesn't remove any icon-classes when used, even if they appear to be defaults. I wanted this service to offer the most unintrusive solution possible.

[Alhadis]

Oh, and the Appveyor test failure is unrelated. Not sure why it's failing but it ain't this PR's fault.

CrappVeyor is failing for another pull-request too, for similarly arcane reasons. So I wouldn't worry about it. =)

Shameless plug for our spiffy test-runner, BTW. Mocha > Jasmine, just sayin'.

[Osmose]

Okay, it seems good. I was afraid that elements created before consumeElementIcons was called wouldn't be styled, that's why I added the emitter initially. I'm not exactly sure why that's not happening now, but it seems to be fine! Thanks!

I expected they wouldn't, but I figured that'd be fine. I suspect that, due to advanced-open-file loading all its stuff when the user first hits the keyboard shortcut instead of on startup (I think that's the case) means that file-icons has already set up its service, and the callback just gets called directly. That's mostly a guess, though.

Anywho, this PR looks good to me, so let's get it merged and released!

[Osmose]

Aaaaaaaaaand it's done! https://github.com/Osmose/advanced-open-file/releases/tag/v0.16.5

Thanks a ton to the both of you, ya'll have been super helpful. :D

[jacobmischka]

Thanks so much for the quick response!