PackageSnippetsView always renders hidden

[savetheclocktower]

Background

I’d like to add a long-requested feature to the snippets package: the ability to disable the snippets provided by packages on a per-package basis. The language-html package, for example, adds a whole bunch of snippets for HTML tags (including really common prefixes like a); users should be able to opt out of these snippets just as easily as they opt out of the keymaps that packages provide in favor of their own shortcuts (or none at all).

Of course, a lot of this work will need to be done in settings-view, and in researching that work I think I stumbled across a couple of bugs. Those bugs have straightforward fixes, but those fixes reveal new problems that I think need a bit of discussion to resolve. Hopefully you’ll see what I mean shortly.

Get the existing snippets view to render

There is a section dedicated to displaying the snippets provided by a package; it’s rendered by PackageSnippetsView. But if I’m doing my code archaeology properly, it disappeared from the settings panel at least a year ago. In its current behavior, it always renders, but also always hides itself via display: none upon initialization, and there’s no code path in which it unhides itself again.

I think this regression was introduced when PackageSnippetsView was rewritten in Etch. The old code said, in effect, “hide the section, but show it again if we have at least one snippet”; the new code says “hide the section, but show the table inside the section if we have at least one snippet.”

If we fix this, we find that only one snippet is shown…

…due to another regression that was introduced in the same commit. I think this section should instead be:

if (snippet != null) {
  if (packageProperties[key] == null) {
    packageProperties[key] = snippet
  }
}

Once we do that, we get a list of snippets.

So far, so good.

Account for scopes

The UI we have at this point shows every prefix for every snippet defined by a package. But a package can declare snippets across multiple scopes. For example, language-html defines lots of snippets in .text.html but redefines them to be no-ops in .text.html .meta.tag, .text.html .embedded. A JavaScript package could define f to create one kind of function snippet in the general case but a different kind of function snippet within a class body.

Say a package defines ten snippets and redefines five in a more specific scope. There are fifteen snippets total, but only ten unique prefixes. So do we show ten rows (one for each prefix)? Or fifteen (one for each combination of prefix and scope)?

(Getting the scope information to show up in the table requires us to do a little more work. To give us the list of snippets, the snippets package simply deep-clones its ScopedPropertyStore instance. But the selector object we get back is the selector after it’s been parsed by atom-slick, and the deep clone strips that object of its special toString behavior which returns a string representation of the selector. So the serialization logic gets more complex on the other side. But I digress!)

The status quo of the snippets panel (once the two bugs above are fixed) lists a snippet without its scope, shows the body of the snippet in the table, and ensures that only one snippet for a given prefix is shown, even if that prefix is used several times in separate scopes. That means that a particular snippet can appear via the snippets:available command, but wouldn’t necessarily get shown in the snippets panel for its package. Feels weird.

Show the snippet — or don’t

Displaying the actual contents of each provided snippet is probably a good idea. It’d help with discoverability and allow the user to make a more informed decision about whether they should disable the package’s snippets.

But a snippet body can contain arbitrary text of arbitrary length, including line breaks and tabs. This makes it tricky to render inside of a table view. We can ellipsize the contents of the cell so that we don’t ever show more than we have room for, but (a) this diminishes the utility of showing the snippet contents in the first place, and (b) we can only ellipsize a table cell if we can constrain its width to a fixed, absolute measurement with max-width or use table-layout: fixed, both of which require us to micromanage the layout of the table. (All the screenshots I’ve posted so far employ table-layout: fixed; notice how all cells are the same width by default.)

Perhaps we show a bit of the snippet and then show the whole thing on hover/click inside a rich tooltip? Or in some sort of lightweight modal?

Conclusion

What should the snippets panel look like? Should it show the minimum amount of information and rely on snippets:available to do most of the work of snippet discoverability, or should it try to act as a comprehensive source of truth?

Hell, if nothing else I think that it’d improve upon the status quo if the panel omitted the list of snippets altogether but had a “disable this package’s snippets” checkbox. But hopefully there’s enough consensus to deliver more than that.

[savetheclocktower]

Also: could #973 be related to this?

[lee-dohm]

@as-cii Do you have a moment to take a look here and give us some background?

[as-cii]

Apologies for this regression, it seems like it was a mistake on my part while porting the old jQuery code to Etch. I don't remember much about it since I ported a dozen other packages to Etch, but I would say we should at least go back to the pre-Etch behavior, as my changes were supposed to be a mere drop-in replacement.

Perhaps we show a bit of the snippet and then show the whole thing on hover/click inside a rich tooltip? Or in some sort of lightweight modal?

This seems smart as well. @simurai: could you weigh in on this? What do you think it would be the best approach UX-wise?

[simurai]

Perhaps we show a bit of the snippet and then show the whole thing on hover/click inside a rich tooltip? Or in some sort of lightweight modal?

Tooltip on hover sounds like a good idea. 👍 Another idea might be to add a "copy snippet" button, similar to the one for keybindings. Then you can paste it in your own snippets.cson and customize it.

[savetheclocktower]

@simurai, how’s this?

The “view” button came about because I couldn’t decide which element should “own” the tooltip otherwise, and once it was a button it made sense to me to make click the trigger rather than hover. Happy to try another idea if you’ve got one.

[simurai]

how’s this?

That looks great. 👍

An alternative might be to show the tooltip on hover or even when hovering the whole row? But could also be annoying since it covers the scope.

[savetheclocktower]

Yeah, I played around with that, but the tooltip suffers from not having an obvious element to “anchor” to.

[savetheclocktower]

So there’s the issue described here (the Snippets view needs a couple fixes to be visible and show accurate information) and the issue that prompted this investigation (atom/snippets#55). Both fixes will require code changes across both repositories, so I’m planning to combine them into one pair of PRs.

The PR I opened the other day, atom/snippets#277, contains the small code change needed for this issue (serializing the scope selector into a string). At some point soon I’ll open a PR in settings-view that will contain the enable/disable checkbox needed to address atom/snippets#55.

The upcoming settings-view PR should not get landed until atom/snippets#277 lands.

[Ben3eeE]

Closing this since #1084 is merged. @savetheclocktower let me know if there is something I missed.