Notify FileSystem on resource deletion

[Lassi]

The FileSystem module wasn't being notified on resource deletion which
resulted in an inconsistent state of the resource cache. By subscribing
to deletion notifications for files in the project and destroying the
resource if it's cached in the Filesystem module, we solve that issue.

Fixes #490

[Alhadis]

Hey @Lassi, thanks for the help. 👋 It's currently difficult for me to get to a computer with Atom installed, so your pull-request is warmly welcomed.

Do the package specs pass? You'll need to install devDependencies before running them:

cd /path/to/file-icons
apm install .

Then, from Atom's View menu, go to Developer... and then Run Package Specs.

I'll fix the JS style issues myself, I won't nag you to do it for me. :-) I'm in no position to be picky over formatting, haha.

[Lassi]

Hey @Alhadis,

My pleasure! It allowed me to learn a few things about Atom ;)

Thanks a lot for the help, I wasn't sure how to run the specs. There is 3 failures but the broken tests also seem to fail on master so I'm not sure if they are linked to the code I introduced or not. I'll try to look into it when I have a minute.

If you have any suggestion regarding formatting please let me know, I don't mind fixing it at all!

[Alhadis]

Ah... Please don't worry those spec failures. You're right, those definitely aren't related to the PR.

If you have any suggestion regarding formatting please let me know, I don't mind fixing it at all!

Alright, well I personally prefer double-quotes over single quotes, and "Yoda-style" comparisons whenever literals being evaluated:

 // "Yoda-style" comparison example
-if(object.method() ===  “value")
+if(“value" === object.method())

I also prefer no spaces between keywords and brackets. :)

[Lassi]

Hey,

Sorry for the late reply.

Ok, thanks, I'll ignore the broken specs. I just fixed the commit according to your suggestions. Let me know if there is anything else you need me to change!

[Alhadis]

Nope, you've done well. 😀 Thanks for taking the time to help us out. 😀

There's a lot about this package's current infrastructure that needs rehauling. (And it's not going to happen in the near future, unless Apple feel like dropping their price of their overly-expensive MacBooks. 👎 Either that, or one with an unsmashable screen. 💔

[utkarshgupta137]

This needs corresponding handling for a "created" event. Sometimes, especially when using auto-formatters, it can leave the file in "deleted" state i.e. resource is destroyed from here, but never recreated.

[utkarshgupta137]

Suppose this kind of "events" object is received (focus on 1,2). In this case, the resource is destroyed, but not recreated.

[Alhadis]

@utkarshgupta137 You're right. Ive pushed a tentative fix; would you mind checking 27e7cac out locally and seeing if it made a difference…?

[utkarshgupta137]

I've actually been testing my own fix since a couple of days, you can checkout the last commit: https://github.com/utkarshgupta137/file-icons-atom. I don't think your commit will fix the issue as the block inside if is only meant to remove files from FS, not for adding.

That commit has fixed the issue I mentioned for me.

[Alhadis]

I don't think your commit will fix the issue as the block inside if is only meant to remove files from FS, not for adding.

That's actually what the issue is. Releasing a Resource instance by calling its destroy() method instructs the FileSystem to drop it form its cache, forcing a new lstat(2) call the next time an icon for that exact path is requested (which, as it so happens, is immediately after the "created" event gets dispatched.

[utkarshgupta137]

Okay, I'm still able to recreate the issue with your commit.

First destroy() is called when deleted is encountered. Then it is called again when created is encountered. So that is why your commit isn't making a difference.

Also, the issue can also happen after you commit/reset files.

[Alhadis]

Maybe it's my crappy ancient MacBook, but I've never been able to replicate this locally (it's a known issue, BTW).

The real issue is that atom-fs has no reference tracking, and handles lstat(2) calls synchronously (which doesn't play well with asynchronous events). That's largely due to limitations in Atom's APIs that ceased being relevant back in 2017… the whole damn library is worth a rewrite.

[utkarshgupta137]

I'm on 10.15 w/ APFS. Maybe it's due to APFS? It could be handling files differently.

The issue occurs when the files are created/modified "externally" i.e. by any external process that creates/modifies a file. This includes any action taken via the git sidebar. If the same action occurs through atom, then there are no issues.
Try this: Enable Coloured & Only colour when changed. Then touch a file. It won't be coloured.

[Alhadis]

Possibly. I'm running a HFS filesystem, and I'm (stuck) on 10.13.6 (High Sierra, which recently reached EOL). My machine is literally too old to upgrade to Mojave (mid-2010), and everybody's dropping support for it (Homebrew's become little more than a glorified Makefile…).

[utkarshgupta137]

Well, you should get those Intel-Macs before they're out of stock everywhere.

Another thing: Run touch t.py. Then rm t.py && touch t.py. In this case, handleResource is not called. So we need to ensure that we don't destroy the resource, when handleResource is not going to be called - which only seems to happen when created & deleted happen in the same onDidChangeFiles call.

[Alhadis]

I've reverted that last commit. As you said, it solved nothing and it's now causing CI tests to fail on macOS (Linux passed, interestingly enough).

Well, you should get those Intel-Macs before they're out of stock everywhere.

Heh. I'd rather maximise the amount of time between buying a new workstation and when Apple inevitably obsolete it like they do all their hardware. 😉 I doubt they'll bother supporting Intel hardware for long; probably 4 years, tops. I won't have enough money for a new workstation again for a looooooooong time, so I gotta make this count (I'll be broke for a while, but hey…)

Also, I ran touch t.py; rm t.py; touch t.py, and the icon appears perfectly fine. Like I said mate, it's a known issue, and has been for quite some time. 😕

[utkarshgupta137]

No no. The file icon appears fine. But the git status for it is wrong. You've gotta Enable Coloured & Only colour when changed to see the issue.

I've raised a PR with a fix, do have a look.