New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consume File-Icons' element-icon service #281

Merged
merged 11 commits into from Oct 30, 2017

Conversation

Projects
None yet
4 participants
@Alhadis
Contributor

Alhadis commented Mar 9, 2017

This PR applies to the tabs package what's been described in atom/tree-view#1146 for the tree-view package. The PR's updated wording describes this scenario best, so I'll just copy+paste:


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

@50Wliu 50Wliu added the needs-review label Mar 10, 2017

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis May 31, 2017

Contributor

I can imagine how busy the Atom team are right now smoothing over bugs with the recent docked-views feature, as well as concentrating on fixing a pretty severe memory issue in the 1.18 beta.

That being said, I really don't wanna come off sounding naggy or impatient... but could somebody give me an update on this? The rotten monkey-patches I wrote have been allowed to exist for exactly half-a-year now (at least in my timezone, which officially ticked over to June over an hour ago....)

/cc @lee-dohm / @50Wliu / @as-cii

Contributor

Alhadis commented May 31, 2017

I can imagine how busy the Atom team are right now smoothing over bugs with the recent docked-views feature, as well as concentrating on fixing a pretty severe memory issue in the 1.18 beta.

That being said, I really don't wanna come off sounding naggy or impatient... but could somebody give me an update on this? The rotten monkey-patches I wrote have been allowed to exist for exactly half-a-year now (at least in my timezone, which officially ticked over to June over an hour ago....)

/cc @lee-dohm / @50Wliu / @as-cii

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Jun 1, 2017

Member

Thanks for the ping @Alhadis ... I'm going to get some 👀 on this 👍

Member

lee-dohm commented Jun 1, 2017

Thanks for the ping @Alhadis ... I'm going to get some 👀 on this 👍

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Jun 8, 2017

Member

@Alhadis we've added this (and its related PRs) to the list for prioritization. I noticed that file-icons was mentioned as a potential performance issue in #269. Would you mind doing some testing to see if this change helps or hurts performance in that scenario?

Member

lee-dohm commented Jun 8, 2017

@Alhadis we've added this (and its related PRs) to the list for prioritization. I noticed that file-icons was mentioned as a potential performance issue in #269. Would you mind doing some testing to see if this change helps or hurts performance in that scenario?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jun 8, 2017

Contributor

Certainly! :D Which settings do you prefer I use when recording the timeline snapshot? I tend to get them all confused... 😞 Or is there another method you'd prefer I try to compare performance?

Contributor

Alhadis commented Jun 8, 2017

Certainly! :D Which settings do you prefer I use when recording the timeline snapshot? I tend to get them all confused... 😞 Or is there another method you'd prefer I try to compare performance?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jun 8, 2017

Contributor

Atom hanged when it was trying to save the recorded CPU profile, but that's what it usually does when I have my machine's root or home directory opened as a project... :( I managed to screen-cap the first profile, which was recording performance without this PR's changes in effect:

screen shot 2017-06-09 at 3 51 17 am

screen shot 2017-06-09 at 3 52 15 am

screen shot 2017-06-09 at 3 53 49 am

I wish I knew how to read these things better. I'm still getting my head around what each graph means...

Contributor

Alhadis commented Jun 8, 2017

Atom hanged when it was trying to save the recorded CPU profile, but that's what it usually does when I have my machine's root or home directory opened as a project... :( I managed to screen-cap the first profile, which was recording performance without this PR's changes in effect:

screen shot 2017-06-09 at 3 51 17 am

screen shot 2017-06-09 at 3 52 15 am

screen shot 2017-06-09 at 3 53 49 am

I wish I knew how to read these things better. I'm still getting my head around what each graph means...

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Jun 8, 2017

Member

@Alhadis if you can manage to save the profile, zip it up and email it to me at lee-dohm@github.com? I believe saving the profile saves all the data and then I can fiddle with it on my end 👍

Thanks very much for helping out 🙇

Member

lee-dohm commented Jun 8, 2017

@Alhadis if you can manage to save the profile, zip it up and email it to me at lee-dohm@github.com? I believe saving the profile saves all the data and then I can fiddle with it on my end 👍

Thanks very much for helping out 🙇

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jun 8, 2017

Contributor

I already tried, but it saved a blank file. :( As a matter of fact, this is what it always gives me whenever I try saving the CPU profile... :(

Contributor

Alhadis commented Jun 8, 2017

I already tried, but it saved a blank file. :( As a matter of fact, this is what it always gives me whenever I try saving the CPU profile... :(

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jun 8, 2017

Member

You'll need to test on Atom 1.19.0-dev in order to get a non-blank profile.

Member

50Wliu commented Jun 8, 2017

You'll need to test on Atom 1.19.0-dev in order to get a non-blank profile.

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm
Member

lee-dohm commented Jun 8, 2017

What @50Wliu said 😀 atom/atom#13767

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jun 8, 2017

Contributor

Ugh... good grief. 😞

Should I profile the other packages while I'm running a HEAD-build of Atom v1.19, then...?

Contributor

Alhadis commented Jun 8, 2017

Ugh... good grief. 😞

Should I profile the other packages while I'm running a HEAD-build of Atom v1.19, then...?

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Jun 8, 2017

Member

Yes, please 🙏

Member

lee-dohm commented Jun 8, 2017

Yes, please 🙏

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jun 8, 2017

Contributor

Damn. Opening my user directory as an Atom project already shows how bad an issue this is... I'll e-mail you the first CPU profile I captured. It was done using dev-mode and no forked packages.

Please let me know if you have difficulties extracting it: it's a 23.3 MBs file I had to 7-Zip.

Contributor

Alhadis commented Jun 8, 2017

Damn. Opening my user directory as an Atom project already shows how bad an issue this is... I'll e-mail you the first CPU profile I captured. It was done using dev-mode and no forked packages.

Please let me know if you have difficulties extracting it: it's a 23.3 MBs file I had to 7-Zip.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jun 8, 2017

Contributor

Profiling the other packages hasn't really revealed much of a difference. The hit to performance is mostly at startup, when file-icons has to load and hack certain files.

Contributor

Alhadis commented Jun 8, 2017

Profiling the other packages hasn't really revealed much of a difference. The hit to performance is mostly at startup, when file-icons has to load and hack certain files.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 6, 2017

Contributor

Conflicts fixed, and the bitter taste of CoffeeScript washed away.

Contributor

Alhadis commented Sep 6, 2017

Conflicts fixed, and the bitter taste of CoffeeScript washed away.

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Sep 6, 2017

Member

Thanks @Alhadis 😀 I'm going to take a look at these next week when I'm on vacation and decompressing.

Member

lee-dohm commented Sep 6, 2017

Thanks @Alhadis 😀 I'm going to take a look at these next week when I'm on vacation and decompressing.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 26, 2017

Contributor

The 'hell's this...?

standard.cmd : standard: Use JavaScript Standard Style (https://standardjs.com)
At line:158 char:13
+             & "$standardpath" lib/**/*.js
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (standard: Use J...standardjs.com):String) [], RemoteException
    + FullyQualifiedErrorId : NativeCommandError
 
standard: Run `standard --fix` to automatically fix some problems.
  C:\projects\fuzzy-finder\lib\icon-services.js:44:76: Extra semicolon.

Yet there were no lint errors locally:

λ FuzzyFinder (file-icons): ./node_modules/.bin/standard .
λ FuzzyFinder (file-icons):
Contributor

Alhadis commented Oct 26, 2017

The 'hell's this...?

standard.cmd : standard: Use JavaScript Standard Style (https://standardjs.com)
At line:158 char:13
+             & "$standardpath" lib/**/*.js
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (standard: Use J...standardjs.com):String) [], RemoteException
    + FullyQualifiedErrorId : NativeCommandError
 
standard: Run `standard --fix` to automatically fix some problems.
  C:\projects\fuzzy-finder\lib\icon-services.js:44:76: Extra semicolon.

Yet there were no lint errors locally:

λ FuzzyFinder (file-icons): ./node_modules/.bin/standard .
λ FuzzyFinder (file-icons):
@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 26, 2017

Contributor

/cc @nathansobo for review

Contributor

Alhadis commented Oct 26, 2017

/cc @nathansobo for review

@nathansobo nathansobo merged commit b3e0100 into atom:master Oct 30, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 30, 2017

Contributor

I appreciate your patience and dedication to making Atom better!

Contributor

nathansobo commented Oct 30, 2017

I appreciate your patience and dedication to making Atom better!

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 30, 2017

Contributor

Published as 1.7.0 and upgraded in atom/atom@d035e41.

I should have asked you to write some basic tests for this. Would you be willing to do a follow up PR that adds some basic tests so we don't break this in the future? I'm going to hold off on the other 3 PRs until there's some test coverage in place.

Thanks @Alhadis.

Contributor

nathansobo commented Oct 30, 2017

Published as 1.7.0 and upgraded in atom/atom@d035e41.

I should have asked you to write some basic tests for this. Would you be willing to do a follow up PR that adds some basic tests so we don't break this in the future? I'm going to hold off on the other 3 PRs until there's some test coverage in place.

Thanks @Alhadis.

@nathansobo nathansobo self-assigned this Oct 30, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 30, 2017

Contributor

Whoops, I had to revert the upgrade commit because there's an issue in with v8 snapshot generation, which you can see in this Circle build. I'll take a look.

Contributor

nathansobo commented Oct 30, 2017

Whoops, I had to revert the upgrade commit because there's an issue in with v8 snapshot generation, which you can see in this Circle build. I'll take a look.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 30, 2017

Contributor

Okay, I fixed the snapshot issue in 2c1fe65 and re-upgraded to 1.7.1 in atom/atom@058a42f. Basically, you can't interact with the atom require at eval-time in snapshotted code, and constructing the IconServices object at eval time was causing that to happen. I replaced the eval-time construction with a lazy getter function.

Contributor

nathansobo commented Oct 30, 2017

Okay, I fixed the snapshot issue in 2c1fe65 and re-upgraded to 1.7.1 in atom/atom@058a42f. Basically, you can't interact with the atom require at eval-time in snapshotted code, and constructing the IconServices object at eval time was causing that to happen. I replaced the eval-time construction with a lazy getter function.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 31, 2017

Contributor

Oh, you guys are using mksnapshot. That's clever. Should the other PRs be amended too?

I should have asked you to write some basic tests for this. Would you be willing to do a follow up PR that adds some basic tests so we don't break this in the future?

Ah, I never notice that. Sure thing!

Contributor

Alhadis commented Oct 31, 2017

Oh, you guys are using mksnapshot. That's clever. Should the other PRs be amended too?

I should have asked you to write some basic tests for this. Would you be willing to do a follow up PR that adds some basic tests so we don't break this in the future?

Ah, I never notice that. Sure thing!

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 31, 2017

Contributor

Should the other PRs be amended too?

Yeah, maybe you already got to it, but we need to avoid eval-time references to the atom require or any built-in node modules in any package that's going to be bundled.

Contributor

nathansobo commented Oct 31, 2017

Should the other PRs be amended too?

Yeah, maybe you already got to it, but we need to avoid eval-time references to the atom require or any built-in node modules in any package that's going to be bundled.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 31, 2017

Contributor

What about the complementary DefaultFileIcons class? That's being exported as a singleton instance in much the same way. Should that use lazy getters too?

Contributor

Alhadis commented Oct 31, 2017

What about the complementary DefaultFileIcons class? That's being exported as a singleton instance in much the same way. Should that use lazy getters too?

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 31, 2017

Contributor

If it's not accessing any built-in modules it should be technically okay, but might be nice to make things consistent. Even better than a lazy getter would be dependency injection, but that's more work.

Contributor

nathansobo commented Oct 31, 2017

If it's not accessing any built-in modules it should be technically okay, but might be nice to make things consistent. Even better than a lazy getter would be dependency injection, but that's more work.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 31, 2017

Contributor

Think I'll leave it for now. DefaultFileIcons feels like it should be merged into the IconServices singleton more than anything else. I'd prefer not to make any more changes this late, however.

Contributor

Alhadis commented Oct 31, 2017

Think I'll leave it for now. DefaultFileIcons feels like it should be merged into the IconServices singleton more than anything else. I'd prefer not to make any more changes this late, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment