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

:arrow_up: snippets & settings-view #18255

Merged
merged 1 commit into from Oct 17, 2018

Conversation

Projects
None yet
5 participants
@Ben3eeE
Member

Ben3eeE commented Oct 17, 2018

This upgrades settings-view and snippets to include the ability to disable a packages snippets.

See atom/snippets#277 atom/settings-view#1084 and atom/settings-view#1076 for details.

It also changes the behavior when pressing Tab after a snippet reaches an explicit end stop atom/snippets#280

/cc: @savetheclocktower @daviwil

@daviwil

This comment has been minimized.

Member

daviwil commented Oct 17, 2018

Looks like you'll need to run script/bootstrap and commit the changes to package-lock.json before this will build successfully. I can give you a hand with this in a little bit if you're not able to get to it first!

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Oct 17, 2018

👍 comitted with the package-lock files now. Let's see if it works better this time 🙂

@daviwil

This comment has been minimized.

Member

daviwil commented Oct 17, 2018

Thanks so much to @savetheclocktower for the great PRs and @Ben3eeE for shepherding them along!

@daviwil daviwil merged commit 09a03a3 into master Oct 17, 2018

3 checks passed

Atom Pull Requests #20181017.3 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil deleted the b3-disable-snippets branch Oct 17, 2018

@vladshcherbin

This comment has been minimized.

vladshcherbin commented Oct 26, 2018

@Ben3eeE @savetheclocktower hey, thank you for this needed feature

Is it possible to enable/disable not all package snippets but only some? For example, package has 20 snippets and I need to disable only 2 of them. In snippets page I can see view/copy buttons, what about enable/disable per snippet?

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Oct 26, 2018

Hi 👋

You can only disable all snippets similarly to how disabling keybindings work. If you want to disable individual snippets you would need to disable all of them and copy back the ones you want to your personal snippets file.

@vladshcherbin

This comment has been minimized.

vladshcherbin commented Oct 26, 2018

@Ben3eeE yes, this is what I thought. However, copy-pasting 18 from 20 snippets is not that great :)

It would be awesome to have a third (enable|disable) button next to copy/view buttons to have a possibility to disable some package snippets. Maybe @savetheclocktower can tell if this is possible to add, this feature would be really nice 🙏

@Aerijo

This comment has been minimized.

Contributor

Aerijo commented Oct 26, 2018

@vladshcherbin

copy-pasting 18 from 20 snippets is not that great

You could disable the other 2 (in snippets.cson)? E.g.,

'.source.js':
  'foo':
    'prefix': 'if'
  'bar':
    'prefix': 'ife'
  'baz':
    'prefix': 'iife'

I'm not sure if it's meant to be used this way, but writing a prefix without a body will effectively disable the original.

@savetheclocktower

This comment has been minimized.

savetheclocktower commented Oct 26, 2018

My gut feeling is that having such an intricate UI as to allow for enabling/disabling of individual snippets would be a granularity mismatch with the rest of Atom. There’s a reason why Atom/VScode/ST lean so heavily on configuration files for customization instead of an elaborate control panel. TextMate has a UI for command/snippet customization, but it’s mostly just a thin veneer over an editor window.

(TextMate also does a lot of work in the background to allow you to customize bundles as though you were editing them directly, but actually storing those customizations as diffs so that the “pristine copies” of bundles can still receive updates. Saying “here’s what this package does; feel free to customize it in your keymap file/snippets file/init script“ punts on that by making it clear to the user how those customizations get applied.)

One possible next step would be to flesh out the relevant sections of Flight Manual so that “how do I disable just a couple of package-provided snippets” has an explicit answer. I do also think that both keymaps and snippets could have a “copy the entire contents of the package’s config file to my clipboard” button so that you can paste the whole thing into your own keymap.cson/snippets.cson file and customize it all at once.

@savetheclocktower

This comment has been minimized.

savetheclocktower commented Oct 26, 2018

@Aerijo’s suggestion is a good one, because then you can leave the “Enable” checkbox checked on the package’s snippets. Explicitly disabling just the ones you dislike means that you’ll still automatically get new snippets if the package author adds some in a future update.

In fact, I think I’ll look into updating the Flight Manual, because this feature should be explained such that the reader understands that disabling a snippet is just one kind of customization. Copying the snippet to your own snippets.cson is step 1; step 2 would be changing the body of the snippet or deleting the body entirely, thus turning it into a no-op.

@vladshcherbin

This comment has been minimized.

vladshcherbin commented Oct 26, 2018

@savetheclocktower I prefer ui toggles as they are more convenient to use for me. Atom packages have enable/disable button which works same way which is very nice.

The issue with snippets file is that when you copy-paste them and a package is updated with new snippets or updates existing ones - you now have your outdated ones.

@Aerijo suggestion is fine, it just feels like a hack when you can disable snippets but overwrite them in config file instead because you can't disable only some using ui.

I guess I'm too used to nice UX and would be happy with any your decision :)

@savetheclocktower

This comment has been minimized.

savetheclocktower commented Oct 26, 2018

The issue with snippets file is that when you copy-paste them and a package is updated with new snippets or updates existing ones - you now have your outdated ones.

Yeah, my second comment supersedes my first one. It’s probably an anti-pattern to dump the entire contents of a package’s snippets.cson into your own for customization purposes. That’s how quickly I can talk myself out of an idea.

it just feels like a hack when you can disable snippets but overwrite them in config file instead because you can't disable only some using ui.

Yeah, I agree that it’s an awkward line to draw. The fact that package-provided keymaps and snippets are disabled via the package settings panel instead of the user’s config file strikes me as a compromise between two valid concerns: (a) user customization should be controlled by config files wherever possible, and (b) a package’s settings page should encapsulate all the configurable aspects of that package.

My view is that snippets should behave similarly to keymaps, so if we were to add individual checkboxes for enabling/disabling snippets, we should do the same for keymaps. Now, whether we’re currently doing the correct thing for both snippets and keymaps is a matter of debate.

I’d suggest creating a new issue for discussion if you think snippets and keymaps should be able to be individually disabled from the settings panel. I haven’t been around long enough to know if the way things are now is the result of a purposeful decision or just because nobody has had a better idea.

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