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 #47

Merged
merged 5 commits into from Oct 31, 2017

Conversation

Projects
None yet
3 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 Oct 26, 2017

Contributor

/cc @nathansobo It was tempting to decaffeinate lib/archive-editor.coffee, being the last remaining CoffeeScript file in the package... but for the sake of a cleaner diff-review, I left it alone. ;)

Once this is merged, I'll quickly follow up with another PR to convert it to JavaScript. That'll be another package freed of the CoffeeScript-era, specs notwithstanding. EDIT: Nah, did those too.

Contributor

Alhadis commented Oct 26, 2017

/cc @nathansobo It was tempting to decaffeinate lib/archive-editor.coffee, being the last remaining CoffeeScript file in the package... but for the sake of a cleaner diff-review, I left it alone. ;)

Once this is merged, I'll quickly follow up with another PR to convert it to JavaScript. That'll be another package freed of the CoffeeScript-era, specs notwithstanding. EDIT: Nah, did those too.

@nathansobo nathansobo self-assigned this Oct 30, 2017

@nathansobo nathansobo merged commit a55e10e into atom:master Oct 31, 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 31, 2017

Contributor

Published as 0.64.0 and upgraded in atom/atom@c3adde5. 🙇

Contributor

nathansobo commented Oct 31, 2017

Published as 0.64.0 and upgraded in atom/atom@c3adde5. 🙇

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 31, 2017

Contributor

Oops, looks like another issue with v8 snapshots. I had to revert. Stand by.

Contributor

nathansobo commented Oct 31, 2017

Oops, looks like another issue with v8 snapshots. I had to revert. Stand by.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 31, 2017

Contributor

Shouldn't this have affected the tree-view as well?

Contributor

Alhadis commented Oct 31, 2017

Shouldn't this have affected the tree-view as well?

@Alhadis Alhadis deleted the Cutlery-Drawer:file-icons branch Oct 31, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 31, 2017

Contributor

Fixed the snapshot issues in 38f9671 and upgraded Atom to 0.64.1 in atom/atom@25cb5c2.

Shouldn't this have affected the tree-view as well?

It looks like we got away with tree-view because its IconServices class doesn't reference any built-in modules in the constructor. I guess tree-view bundles its own event-kit for some reason? 🤷‍♂️

Contributor

nathansobo commented Oct 31, 2017

Fixed the snapshot issues in 38f9671 and upgraded Atom to 0.64.1 in atom/atom@25cb5c2.

Shouldn't this have affected the tree-view as well?

It looks like we got away with tree-view because its IconServices class doesn't reference any built-in modules in the constructor. I guess tree-view bundles its own event-kit for some reason? 🤷‍♂️

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 31, 2017

Contributor

I guess tree-view bundles its own [event-kit] for some reason? 🤷‍♂️

Wouldn't it be easier if it shared Atom's event-kitexports so there's a lighter load at start-time? So to speak

Contributor

Alhadis commented Oct 31, 2017

I guess tree-view bundles its own [event-kit] for some reason? 🤷‍♂️

Wouldn't it be easier if it shared Atom's event-kitexports so there's a lighter load at start-time? So to speak

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 31, 2017

Contributor

Yeah it would definitely be better. It might be getting de-duped but I'm not sure. If you want to try dropping the dependency, I'd welcome the PR.

Contributor

nathansobo commented Oct 31, 2017

Yeah it would definitely be better. It might be getting de-duped but I'm not sure. If you want to try dropping the dependency, I'd welcome the PR.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 31, 2017

Contributor

Alright, on it.

Contributor

Alhadis commented Oct 31, 2017

Alright, on it.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

@nathansobo Just a heads up to let you know I'm collating the existing icon specs in a format that's, well, sane. No idea why the icon-related specs are split between three files, but I'm rewriting them in a proper BDD style and moving them to tree-view.js.

I've dumped the event-kit dependency and added a lazy getter. Working file. Shout out if you'd like me to submit that as a separate PR.

Contributor

Alhadis commented Nov 1, 2017

@nathansobo Just a heads up to let you know I'm collating the existing icon specs in a format that's, well, sane. No idea why the icon-related specs are split between three files, but I'm rewriting them in a proper BDD style and moving them to tree-view.js.

I've dumped the event-kit dependency and added a lazy getter. Working file. Shout out if you'd like me to submit that as a separate PR.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 1, 2017

Contributor

@Alhadis Yeah, since this one is already merged, opening a separate PR would be good. I could do it for you but I'd rather that you get the credit.

Contributor

nathansobo commented Nov 1, 2017

@Alhadis Yeah, since this one is already merged, opening a separate PR would be good. I could do it for you but I'd rather that you get the credit.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 1, 2017

Contributor

Haha, thank you! :D It's ready atom/tree-view#1195

Contributor

Alhadis commented Nov 1, 2017

Haha, thank you! :D It's ready atom/tree-view#1195

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