Allow file-icons service to set multiple class names #825

Alhadis · 2016-04-29T18:53:52Z

Packages that provide the atom.file-icons service are currently limited to setting only one class name per file. This is because any attempt to add a DOMTokenList item with a whitespace character will trigger an exception, as per the spec.

However, this is particularly limiting for packages that need flexibility. For example, the File Icons package will be adding two different classes: one for the icon, and another for its colour.

I've amended the service consumption routine to allow a list of classes to be specified as either an array or a whitespace-delimited string (analogous to jQuery's approach).

So this:

service.iconClassForPath("gear-icon medium-orange");

will now lead to this:

<span class="name icon gear-icon medium-orange">

... instead of, well, this:

If the package providing the service needs additional information on the file, they can't do so without querying the filesystem. For instance, an author may need a link-icon shown for symlinked files. Without providing a direct file reference, this becomes much more cumbersome.

This provides developers with much more information beyond a simple file reference, which can always be accessed through the DOM node anyway.

Alhadis · 2016-05-31T20:44:07Z

@lee-dohm Anything I can do to move this along?

Alhadis · 2016-06-21T10:10:54Z

@50Wliu, any progress?

I don't mean to come across as impatient, but two months of solid silence does tend to leave one feeling uncomfortable.

winstliu · 2016-06-21T15:14:26Z

I'm not the person to ask about progress generally. All I do is add the appropriate labels and move on.

Alhadis · 2016-06-21T15:17:28Z

Ah, right. Sorry to bother.

I guess I'll just keep waiting then..

as-cii · 2016-07-22T13:59:30Z

lib/file-view.coffee

@@ -19,7 +19,11 @@ class FileView extends HTMLElement
    @fileName.dataset.name = @file.name
    @fileName.dataset.path = @file.path

-    @fileName.classList.add(FileIcons.getService().iconClassForPath(@file.path))
+    iconClass = FileIcons.getService().iconClassForPath(@file.path, this)


Could you explain why we need to pass the current FileView instance to the file icons service? Maybe it's not a huge deal, but seems like the service shouldn't need it in order to provide an icon. What am I missing? 💭

It allows the provider to understand the context of where the icon is being requested. To give a concrete example, File-Icons has a "Show in tabs" setting that should determine whether an icon gets displayed in a tab or not.

Interesting, I wonder if we should put that setting on the individual packages instead of customizing every package UI via File-Icons. Both tree-view and tabs could expose a disableCustomIcons setting which, if enabled, wouldn't call the file icons service at all, keeping its default icons.

What do you think? Seems like this would be more customizable and would allow File-Icons to not implement a setting for every package that uses it. 💭

Not a bad idea, actually. =) Want me to tackle that?

That would be 🆒! How about avoiding to pass this in the current pull-request and open a new one with the new config parameter?

After a quick glance, it doesn't seem to have any reference to a File object,

Here:

TabView.item?.buffer.file

What I am worried about is inconsistency, though

To be honest, inconsistency is a trivial sacrifice to make until Atom has a formal API for interacting with the filesystem. And yes, you're 100% right. Furthermore, I believe that API should be the one consuming the icon-class service... that way, there's only one point of info, and services can deal with that without having to care about where the class is being used.

it seems like the only alternative to return a correct result that is also consistent across calls is to bake fs queries into the file icons service package

And no, I think it's a harsh burden that's being placed on package authors, particularly those who have to consider scalability.

As a stopgap measure, I guess we could hand some metadata about the file to the service, like:

service.iconClassForPath(path, {symlink: false})

Thoughts?

I'm happy with that. I'll even see if I can tie it in with what #859 uses.

FYI, that IS an addition I took delicate measures to shield against complications.

@as-cii Okay, it's done! :D You'll notice I've passed a @stats property from the newly-added File method, which will be smoothly taken care of by #859.

The metadata includes atomic values with no dangling references. I only included stuff that was strictly filesystem-related, and not in any way related to the tree-view itself (which I believe was your point in abstracting the consumer's logic from the provider's).

as-cii · 2016-07-25T14:26:22Z

lib/file.coffee

@@ -67,3 +67,6 @@ class File

  isPathEqual: (pathToCompare) ->
    @path is pathToCompare or @realPath is pathToCompare
+
+  # Return a snapshot of the instance's filesystem properties for API consumption
+  getMetadata: -> {@symlink, @realPath, @status, @stats}


Again, I am a little worried about consistency. In this package we have access to stats and status, but what about the tabs package?

*cough* UH, well... at the moment, it's returning a reference to the calling TabView element, which I'll obviously tend to once we're done with this PR

Access to the tab's File instance is available through @item.buffer.file, but that's an instance of one of these, which doesn't have the cool extras of filesystem stats the Tree-View's File class has.

I'm aware it's not 100% consistent, but how about storing a reference to the buffer's File instance on the second argument? stats won't be available, but the source file can be keyed to bufferFile or just file.

tree-view: { symlink, realPath, status, stats } tabs: { symlink, realPath, sourceFile }

To be honest, I think we can probably justify having an extra filesystem call to return the missing info to stay consistent with tree-view's handling. Let's face it: there won't be nearly as many icon-displaying tabs as there will be icon-displaying file listings.

@as-cii What's happening here? The sudden silence has me rather worried.

To be honest, I think we can probably justify having an extra filesystem call to return the missing info to stay consistent with tree-view's handling. Let's face it: there won't be nearly as many icon-displaying tabs as there will be icon-displaying file listings.

That's fair, although this feels like a clear indicator that this interface is inconsistent and more complex than it should need to be. I think we should try to design services in an elegant manner, so I am 👎 on passing different arguments based on the package, and I think we should issue fs.stat calls directly in the FileIcons service, delegating to it all the concerns related to the filesystem.

The only reasonable alternative I could think of (that doesn't involve a major redesign in Atom core) is to provide an alternative iconClassForPath method that returns a Promise, so that the UI thread never blocks and we can do whatever I/O is needed on that function. It would make the calling code a little more complex, but it's probably worth a try if fs.statSync makes file-icons a lot slower.

So what's wrong with passing different arguments as a temporary solution until a formal API is developed? When it is, you can just use Grim to deprecate usage of the second argument so package authors know to use the more elegant approach.

Actually never mind. I've lost patience during all this bikeshedding. I'll kill the second argument and write whatever craploads more code I need to write in order to get things off the ground. Seriously.

Alhadis · 2016-07-25T15:35:10Z

@as-cii Actually, I'm beginning to think having a disableCustomIcons setting probably isn't a good idea. Might be convenient for package authors, but it threatens to make a mess of user configuration. What if a user prefers one package show icons in tabs, and their theme's icons in the tree-view? (Or vice versa...).

There'd be no logical in-between, and package authors would (again) be reduced to using ugly hackery. I think in the long-term, the setting would introduce more complications than it'd solve. My gut feeling says "no, don't…" and my guts are usually pretty good about this kinda stuff.

Real-world example. The ever-popular Seti UI theme comes with icons included. It has a setting to disable them for users who have a dedicated package installed (like File Icons, obviously). It doesn't use the icons API, but this is a classic case of what I'm talking about.

as-cii · 2016-07-29T08:22:06Z

Actually, I'm beginning to think having a disableCustomIcons setting probably isn't a good idea. Might be convenient for package authors, but it threatens to make a mess of user configuration. What if a user prefers one package show icons in tabs, and their theme's icons in the tree-view? (Or vice versa...).

Could you explain why having a setting on each package wouldn't solve this? Like, users could set tree-view.enableCustomIcons = false and have tabs.enableCustomIcons = true, which seems reasonable to me. What am I missing?

Alhadis · 2016-07-29T09:02:56Z

Well, a package or theme (like Seti) might offer users a way to disable custom icons specifically to allow other packages to display their icons instead. Seti and File-Icons have had problems in the past with overlapping icon rules, and this is precisely one of the issues the icon-service exists to solve.

See: #825 (comment)

Alhadis · 2016-07-30T06:59:08Z

I've removed the second parameter, but if I were to suggest a solution to the "disabling icons per package" issue, it'd be to simply pass the package's name as an argument:

iconClassForPath(path, "tree-view");

That way, packages can decide for themselves whether they want to provide an icon class or not.

Alhadis · 2016-08-01T17:37:51Z

@as-cii I've implemented my last suggestion... hope that's okay with you, mate.

I'll make the same change to the tabs package too.

EDIT: Done.

See also: * atom/tree-view#825 * atom/tabs#359

as-cii · 2016-08-05T08:33:32Z

Thanks for the changes, @Alhadis! 🙇

Well, a package or theme (like Seti) might offer users a way to disable custom icons specifically to allow other packages to display their icons instead. Seti and File-Icons have had problems in the past with overlapping icon rules, and this is precisely one of the issues the icon-service exists to solve.

I see, so if for instance we have a enableCustomIcons setting for tree-view but users have the Seti-UI theme installed they would need to:

Set tree-view.enableCustomIcons = true.
Set seti-ui.fileIcons = false

Or, if they want only standard icons for the tree-view, they would need to:

Set tree-view.enableCustomIcons = false.
Set seti-ui.fileIcons = false

Is that accurate? If so, it still seems like having a per-package setting provides a high degree of customization while still being pretty easy to change, also allowing new consumers of the file-icons service to pass just a path. Then from the tree-view all we have to do is (pseudocode):

if (atom.config.get('tree-view.enableCustomIcons')) {
  someElement.classList.add(fileIconsService.iconForPath(path))
}

I guess you're more concerned about not having the enableCustomIcons settings all in one place? I also wonder: do we currently have any usage of disabling custom icons only for only a part of the UI? Seems like it could make the UX between tabs and tree-view inconsistent, right?

That said, I am not totally opposed to pass the context, but seems worth exploring other solutions that make the file-icons service highly reusable as well as keeping the API consistent and clean.

Thanks for your patience on this! If in the meantime you'd like to change this pull-request to introduce only the change you mentioned in the title (i.e. allowing multiple class names to be passed), I think we can merge that right away! 👍

Alhadis · 2016-08-05T08:57:57Z

…but seems worth exploring other solutions that make the file-icons service highly reusable as well as keeping the API consistent and clean.

The issue here is that you're trying to predict and solve issues on behalf of package authors, instead of entrusting them to tailor their package's logic. There is no logical reason to obfuscate the context of the API's call for the sake of keeping things "clean".

Alhadis · 2016-08-05T09:07:25Z

I'll remove the context from this PR temporarily so this bullshit bug can finally be fixed. But I'll be adding it in a follow-up PR.

See: #825 (comment)

Allow packages to provide multiple file-icon classes

06f7030

winstliu added the needs-review label Apr 29, 2016

Alhadis added 4 commits May 10, 2016 21:41

Add specs for enhanced class-list handling

457b26e

Pass the file's DOM element as a secondary argument

4ec7de3

This provides developers with much more information beyond a simple file reference, which can always be accessed through the DOM node anyway.

Silence Travis

37b791e

as-cii reviewed Jul 22, 2016
View reviewed changes

Alhadis added 5 commits July 24, 2016 06:49

Merge branch "master" into class-list-fix

8770863

Merge changes from upstream's master branch

0e3193e

Move icon-class specs to tree-view-spec

a09b884

Define a File method for supplying basic metadata

04b55c4

Modify spec to expect metadata to be passed

38be0cb

as-cii reviewed Jul 25, 2016
View reviewed changes

Alhadis added 2 commits July 30, 2016 15:17

Merge branch 'master' into class-list-fix

32efdfb

Delete second argument passed to iconClassForPath

6655c26

See: #825 (comment)

Use package's name to identify service context

03c7455

Alhadis added a commit to file-icons/atom that referenced this pull request Aug 1, 2016

Update service handler to use proposed additions

5969c80

See also: * atom/tree-view#825 * atom/tabs#359

Temporarily remove context from API call

6127c9e

See: #825 (comment)

as-cii merged commit 3e7e1d9 into atom:master Aug 5, 2016

Alhadis deleted the class-list-fix branch August 5, 2016 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow file-icons service to set multiple class names #825

Allow file-icons service to set multiple class names #825

Alhadis commented Apr 29, 2016 •

edited

Loading

Alhadis commented May 31, 2016

Alhadis commented Jun 21, 2016

winstliu commented Jun 21, 2016

Alhadis commented Jun 21, 2016 •

edited

Loading

as-cii Jul 22, 2016

Alhadis Jul 22, 2016

as-cii Jul 22, 2016

Alhadis Jul 22, 2016

as-cii Jul 22, 2016

Alhadis Jul 25, 2016

Alhadis Jul 25, 2016 •

edited

Loading

as-cii Jul 25, 2016 •

edited

Loading

Alhadis Jul 25, 2016

Alhadis Jul 25, 2016

as-cii Jul 25, 2016

Alhadis Jul 25, 2016

Alhadis Jul 25, 2016

Alhadis Jul 28, 2016

as-cii Jul 29, 2016 •

edited

Loading

Alhadis Jul 30, 2016

Alhadis Jul 30, 2016

Alhadis commented Jul 25, 2016 •

edited

Loading

as-cii commented Jul 29, 2016

Alhadis commented Jul 29, 2016

Alhadis commented Jul 30, 2016

Alhadis commented Aug 1, 2016 •

edited

Loading

as-cii commented Aug 5, 2016

Alhadis commented Aug 5, 2016

Alhadis commented Aug 5, 2016 •

edited

Loading

Allow file-icons service to set multiple class names #825

Allow file-icons service to set multiple class names #825

Conversation

Alhadis commented Apr 29, 2016 • edited Loading

Alhadis commented May 31, 2016

Alhadis commented Jun 21, 2016

winstliu commented Jun 21, 2016

Alhadis commented Jun 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alhadis Jul 25, 2016 • edited Loading

Choose a reason for hiding this comment

as-cii Jul 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

as-cii Jul 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alhadis commented Jul 25, 2016 • edited Loading

as-cii commented Jul 29, 2016

Alhadis commented Jul 29, 2016

Alhadis commented Jul 30, 2016

Alhadis commented Aug 1, 2016 • edited Loading

as-cii commented Aug 5, 2016

Alhadis commented Aug 5, 2016

Alhadis commented Aug 5, 2016 • edited Loading

Alhadis commented Apr 29, 2016 •

edited

Loading

Alhadis commented Jun 21, 2016 •

edited

Loading

Alhadis Jul 25, 2016 •

edited

Loading

as-cii Jul 25, 2016 •

edited

Loading

as-cii Jul 29, 2016 •

edited

Loading

Alhadis commented Jul 25, 2016 •

edited

Loading

Alhadis commented Aug 1, 2016 •

edited

Loading

Alhadis commented Aug 5, 2016 •

edited

Loading