file-icons / atom

Atom file-specific icons for improved visual grepping.
MIT License
1.32k stars 251 forks source link

Not working in Nuclide #471

Closed jmrborges05 closed 7 years ago

jmrborges05 commented 7 years ago

Hello, i update to version 2.0.1 but stop work, if i downgrade to v 1.7.25 it works. lasted version of atom with nuclide on mac ox

Alhadis commented 7 years ago

Nuclide? I've not tried that.

Ugh, I wasn't even aware this existed until now. Good grief.

I'll look into it mate.

ghost commented 7 years ago

same issue, it's not working in Nuclide's file-tree, and some kind of "can not read root..." error came out once, sry, i 4got take a pic.....

Alhadis commented 7 years ago

@401KG Was the Cannot read… error the same as the one described here?

If so, it was fixed in v2.0.1. If not, please let me know if you encounter it again.

Strutsagget commented 7 years ago

Still not working in nuclide file tree. Tabbar works fine with 2.0.1 I really miss those beautiful icons :(, thx for making this awesome module in the first place :)

Alhadis commented 7 years ago

The nuclide-file-tree package that Nuclide installs isn't consuming Atom's file-icon service like it should be doing. They really need to resolve that... anybody care to send them a PR to make our future lives easier? =)

FYI, I can likely fix this with a tasteless jackhammer solution, but I'd be more comfortable knowing they're integrating with Atom properly.

Alhadis commented 7 years ago

I really miss those beautiful icons :(

Can we meet halfway? Will you accept icons that look like shit? =)

…

It's okay, I wouldn't either.

Think this is gonna involve a PR to Nuclide sooner rather than later...

Alhadis commented 7 years ago

Stop, React... please stop... they're already dead...

…
ghost commented 7 years ago

@Alhadis i never catch a sight of that error again, and my file-tree still " look like shit"( :p) after the updating 4 v2.0.2.......

cb5c4b02-0532-4a25-954a-2d2f9dd98df4
Alhadis commented 7 years ago

Glad to hear that. =) The first half of it, that is.

I released v2.0.2 without attempting to fix Nuclide because I had no way of knowing how long it would take (and I'm trying to put as many fires out as possible, as quickly as possible).

My knowledge of React/Flow is precisely zero, but I can tell at a glance it's doing some funky weird shit with adding/removing elements as the user scrolls or navigates. So, a MutationObserver clearly isn't gonna work... gonna have to think laterally here.

ghost commented 7 years ago

@Alhadis thx 4 ur done and doing, man. i'll try 2 contact Nuclide's guys and find out if there any way 2 clear the Long term problem.

Alhadis commented 7 years ago

All too happy to help, mate.

Alright, this is what we'll do... we define our own icon API, and ask the Nuclide guys to use that.

Atom's file-icon API is useless. Seriously. If anybody wants to know just how much heavy-lifting this package is really doing, delete the line from package.json that connects it to Atom's icon-service:

diff --git a/package.json b/package.json
index 0ea3b1d..3180651 100644
--- a/package.json
+++ b/package.json
@@ -29,11 +29,6 @@
        "tmp": "*"
    },
    "providedServices": {
-       "atom.file-icons": {
-           "versions": {
-               "1.0.0": "provideService"
-           }
-       }
    },
    "configSchema": {
        "coloured": {

Then restart. Aside from tab-icons being misaligned, everything else displays fine.

Since we've pretty much already written our own damn API, we may as well formalise it.

thierryc commented 7 years ago

file-type-icons works with nuclide on mac ox. This file may help you to find the solution for file-icons.

https://github.com/lee-dohm/file-type-icons/blob/master/styles/nuclide-file-type-icons.less

Alhadis commented 7 years ago

Thanks, but that file's just CSS (or Less, if you will). It's simply doing what this package used to do.

Don't worry, this will be fixed. I'm just making sure the solution isn't gonna introduce more problems for anybody. :p

thierryc commented 7 years ago

Thank you Alhadis

Alhadis commented 7 years ago

It's done. Integration instructions are written in the readme. Just gotta get it into Nuclide's codebase.

There's nothing more we can do on our end that won't cause more problems. So hopefully they'll integrate our service and everybody will be happy in the end. :)

jacobmischka commented 7 years ago

Small issue, looks like nuclide doesn't like .icon-nuclicon-* elements's font-family being overwritten. It looks like directories are the only elements with a class name like that. So essentially that means that any directory icon doesn't work in nuclide right now.

Please see the .atom directory in the screenshot below.

virtualbox_elementary os_01_01_2017_01_47_45

Alhadis commented 7 years ago

Hrm. Could you post your current modifications to Nuclide? Need to check this locally.

Alhadis commented 7 years ago

It might take manual removal of the icon-nuclicon-file-directory class beforehand, BTW:

iconSpan.classList.remove("icon-nuclicon-file-directory");
addIconToElement(iconSpan, "/home/mischka/.atom");
jacobmischka commented 7 years ago

I've still not gotten it working fully yet, but sure I'll push: https://github.com/jacobmischka/nuclide

That might work but I feel like that'll cause larger problems, I'll try it.

Alhadis commented 7 years ago

Weird... it's like it's only generating a maximum of 10 icons or so in the file-tree, and reusing those whenever other entries are listed. If that's the case, then literally nothing - icon-service or otherwise, is going to get this working reliably.

Sigh... stuff like this is why I avoid frameworks...

jacobmischka commented 7 years ago

Yeah, it really doesn't make much sense lol, I don't really understand why they do it this way. Hm.

Alhadis commented 7 years ago

Sounds like this is more an issue with their implementation, to be honest. If they're not accommodating the possibility of dynamic icons, well... there's the first major problem to fix...

It might explain the spastic icon-morphing I witnessed the other day while trying the most brutally direct solution.

jacobmischka commented 7 years ago

Well actually I kind of see now. I haven't given up yet, I'll let you know when I have.

jacobmischka commented 7 years ago

Okay I actually got it working. https://github.com/jacobmischka/nuclide

For some reason the directory thing doesn't seem to be an issue anymore, nuclide's rules are taking priority now, as they should. Not sure why they weren't before though.

jacobmischka commented 7 years ago

This still needs some cleanup before a PR can be submitted, but I believe it works. So if anyone wants to apm install jacobmischka/nuclide in the meantime it should make icons work again.

I'll have to wait until tomorrow to submit the PR, I need to go to sleep now.

Alhadis commented 7 years ago

It's still doing that... thing for me. :<

Figure 1

I hear ya on the rest factor, I'm not sure if React's still screwing with me, or I'm hallucinating...

jacobmischka commented 7 years ago

Hm, okay I'll take another look in the morning, thanks for letting me know.

Alhadis commented 7 years ago

@jacobmischka Any progress, mate? :)

jacobmischka commented 7 years ago

Ha, sorry. See facebook/nuclide#488 for my excuses.

xxsnakerxx commented 7 years ago

With file-icons@1.7.25 everything works fine

Alhadis commented 7 years ago

Okay, I've worked out a stopgap. It's not perfect, but having mostly-complete support is better than nothing:

Right?

Notable limitations

jacobmischka commented 7 years ago

Maybe it might be worthwhile to use both the old CSS method and the new JS one simultaneously. That way everyone at least gets file extension based icons, and if the new method works it takes priority.

What do you think about something like that?

Alhadis commented 7 years ago

I'm afraid that would require some pretty heavy rewriting of the package's core-code, which would only be temporary anyway (I hope... trying to stay optimistic here).

Alhadis commented 7 years ago

@jacobmischka How's it looking for you, mate? Thanks again for having a crack at fixing this. :) Hopefully FB get this sorted.

If they don't, they're probably exacting their revenge on me after I deleted my Facebook in Feb 2015 and never looked back, HAH.

jacobmischka commented 7 years ago

I guess maybe we should see if atom decides to make their file icon service better, but if not I feel like there's always going to be somewhere where this js solution won't work, and css might just be a nice fallback.

Still haven't had a chance to do anything else with my nuclide fork, sorry lol. Been a little busy with work, maybe later this week. They said they probably won't merge it anyway until it's discussed further, so I don't really think it's a priority anymore.

Alhadis commented 7 years ago

To be honest, this has nothing to do with Atom's existing file-icon service. The base issue is that Nuclide isn't using any service. Of course, Atom's own file-icon service isn't enough; they need the whole shebang.

GET IT?

Alhadis commented 7 years ago

I am way too good at cracking myself up.

jacobmischka commented 7 years ago

Ha, I mean I know that. But they're much more willing to implement an official atom service than this one, if the atom one would fix the issue I think they'd be happy to include it.

I just meant, if atom changes theirs to be useful, we can require packages to support it to get the icons, but if not there's always going to be some packages that won't start using this one I feel.

Alhadis commented 7 years ago

SHUT UP AND PAY ATTENTION TO HOW HILARIOUS I AM.

Nah, I hear what you're saying, mate. But truth be told, I'd say Atom would prefer keeping their services generic and simple, because the spirit of the services API is to help package authors achieve things without deciding how they should be implemented.

So what we've done is really too specific and, dare I say, should be kept separate from the core APIs.

I admit I'm also a control freak who'd rather retain his grip on how this package functions. But yeah.

jacobmischka commented 7 years ago

That's why I think it might be worthwhile to try to maintain both the CSS and the JS solutions. It would be a lot of work though, so I can understand not wanting to take something like that on.

Version 2.0 is definitely much more powerful and very nice, but there's also something to be said for people who want a lightweight CSS solution that just works like ^1.0. I know there's another CSS-based file icons package but frankly the icons just aren't as good as this package's.

Maybe it'll just have to come down to someone releasing a fork of 1.7.25 and backporting new icons from this package.

Alhadis commented 7 years ago

Lightweight? Uhm.

  1. Each of the dynamic strategies can be selectively disabled, and turning off modeline and hashbang strategies will disable IO output altogether, resulting in no wasted system calls

  2. Startup time is improved, and there's a readable and maintainable config for updating icons

  3. Nothing about v1's CSS was lightweight, period. Not just talking about the startup time, mind you; matching overly-qualified attribute selectors really, really, really isn't performant. Especially given how many there were... I can't imagine how badly it slowed repaint times.

Alhadis commented 7 years ago

Anyway, now that we have a service of our own (rather than Atom's), the key is getting Facebook to integrate it.

... we're on the same page here, right?

jacobmischka commented 7 years ago

Oh wow nice, I didn't realize 2.0 was more performant in addition to being more powerful. Great work then.

I just don't think they will frankly, but if we can do that that would be ideal yes.

Alhadis commented 7 years ago

Yeah, well, I'm a grouch when it comes to performance, probably far more so than most people. Probably for the same reason I use no transpilers or frameworks, opting instead to hand-core the barest minimum of what's needed.

Con: Finding a job is a nightmare Pro: But hey, at least my code runs like a cut cat Con: Still broke

(It's 3:16 am and I'm gibbering shit, sorry)