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

Ensure snippets are displayed in a package’s settings view. #1084

Merged
merged 4 commits into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@savetheclocktower
Contributor

savetheclocktower commented Oct 10, 2018

Add an “Enable” checkbox inside the snippet view to add/remove the current package from the snippets blacklist.

Description of the Change

PackageSnippetsView is rehabilitated and made to show a package’s snippets (should a package have any snippets to display). The issues described in #1076 have been fixed.

An “Enable” checkbox has been added at the top of the section — identical to the one at the top of a Keybindings panel. Just as unchecking the Keybindings panel’s Enable checkbox immediately disables that package’s key bindings, unchecking the Snippet panel’s Enable checkbox immediately disables all snippets provided by that package. No reloading of the window is necessary.

The snippets themselves are listed in the table with columns for prefix, name, scope, and body. Since the body of a snippet is arbitrary text of arbitrary length, we don’t show it in the table; we show it in a tooltip that is shown when the View button is clicked. The Copy button writes a string to the clipboard that can be pasted into a user’s snippets.cson (or snippets.json) for further customization.

Alternate Designs

I mooted this change in #1076 before contributing this PR because I wasn’t yet certain how snippet bodies ought to be displayed in the UI.

Benefits

First of all, this fixes the bug (described in #1076) that prevented a package's settings page from listing snippets altogether.

This is also the UI side of atom/snippets#277, which allows package-provided snippets to be disabled on a per-package basis. This is one of the oldest and most-requested enhancements for the snippets package.

Possible Drawbacks

Can't think of any. This fixes a regression. We were already paying the cost of retrieving the snippets for display in PackageSnippetsView; we were merely failing to display them.

Applicable Issues

  • #1076 has discussion about the bugfix and the UI for the snippets table.
  • atom/snippets#55 is the original feature request, and atom/snippets#277 is a prerequisite change for this PR. Ideally these two PRs should be landed simultaneously, but under no circumstances should this one land before the other, or else this UI will be hooked up to nothing at all.
Ensure snippets are displayed in a package’s settings view.
Add an “Enable” checkbox inside the snippet view to add/remove the current package from the snippets blacklist.
@savetheclocktower

This comment has been minimized.

Contributor

savetheclocktower commented Oct 10, 2018

Pinging @Ben3eeE as he requested.

@savetheclocktower

This comment has been minimized.

Contributor

savetheclocktower commented Oct 10, 2018

The CI failures are happening because a couple of the specs rely on behavior added in atom/snippets#277.

@50Wliu 50Wliu requested a review from Ben3eeE Oct 10, 2018

@Ben3eeE

Thanks for contributing 🙇

This is looking great. I left two comments below.

Regarding the tests we will need to version guard them, see https://github.com/atom/language-javascript/blob/ff23c6041579b61ad695b898bce206dc17570145/spec/javascript-spec.coffee#L701-L709 for how we have done this in the past.

/cc: @daviwil

Show resolved Hide resolved lib/package-snippets-view.js
addSnippets () {
getSnippetsModule () {
const snippetsPackage = atom.packages.getLoadedPackage('snippets')
return snippetsPackage ? snippetsPackage.mainModule : null

This comment has been minimized.

@Ben3eeE

Ben3eeE Oct 16, 2018

Member

We want to avoid accessing the mainModule directly here.

Can you change this to provide the function we need through the snippets-service that settings-view consumes?

See https://flight-manual.atom.io/behind-atom/sections/interacting-with-other-packages-via-services/

This comment has been minimized.

@daviwil

daviwil Oct 16, 2018

Member

You can find the service object definition here, just exposing the getUserSnippetsPath should be good enough!

This comment has been minimized.

@savetheclocktower

savetheclocktower Oct 16, 2018

Contributor

Changing the reference to mainModule in this added section is easy enough. There is preexisting code in getSnippets that deals with the module directly instead of the exposed service object — will that need to get addressed before we can land this, or can I disregard it for now?

This comment has been minimized.

@Ben3eeE

Ben3eeE Oct 16, 2018

Member

@daviwil Do you think we should open an issue for this?

const snippetsModule = snippetsPackage ? snippetsPackage.mainModule : null

This comment has been minimized.

@daviwil

daviwil Oct 16, 2018

Member

Interesting, I didn't realize there's pre-existing code for this. I just took a look at how we consume the snippet service and it appears we don't even pass along the object directly but instead pass along its getSnippets method with a different object:

https://github.com/atom/settings-view/blob/master/lib/main.js#L86-L90

It looks like this code definitely needs to be overhauled at some point in the future so I'd suggest leaving the current usage of snippetsPackage.mainModule the way it is for now.

If it's not too much extra work to get the user settings service method piped through, it'd be ideal if we could at least do that part well without the rest of it being fixed. If it ends up adding more headache than it's worth, I'd be OK with just calling directly into the mainModule to get the config path.

This comment has been minimized.

@savetheclocktower

savetheclocktower Oct 16, 2018

Contributor

If it's not too much extra work to get the user settings service method piped through, it'd be ideal if we could at least do that part well without the rest of it being fixed. If it ends up adding more headache than it's worth, I'd be OK with just calling directly into the mainModule to get the config path.

Nah, that part was easy enough. The rest I’d be happy to tackle in a separate issue.

This comment has been minimized.

@daviwil

daviwil Oct 16, 2018

Member

Excellent, thanks a lot!

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Oct 16, 2018

@simurai Do you have any feedback on the design here? I saw you commented in #1076 and said it looked good.

@savetheclocktower

This comment has been minimized.

Contributor

savetheclocktower commented Oct 16, 2018

@Ben3eeE To version-guard the tests it looks like I’ll have to make an educated guess about which version this feature will be included in. Version 1.32 is in beta; should I assume 1.33? Or is there a better approach?

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Oct 16, 2018

@savetheclocktower I hope to get this merged so it goes out in beta in Atom 1.33 🙂 but I can't make any promises about that. Please version guard it for Atom 1.33 🙇

@daviwil

This comment has been minimized.

Member

daviwil commented Oct 16, 2018

We'll be creating the release branches for 1.32.0 and 1.33.0-beta0 on Friday. If this PR gets in before then, checking for a minor version >= 33 should be sufficient, otherwise you'll have to check for 1.34.0.

@savetheclocktower

This comment has been minimized.

Contributor

savetheclocktower commented Oct 16, 2018

OK, the last commit addresses all feedback. @Ben3eeE, I fixed the issue with newlines when copying to the clipboard, and I changed the specs to cover that scenario. But since those tests are now version-guarded they won't run unless you temporarily change the spec file just to verify it.

(Also updated atom/snippets#277 to expose getUserSnippetsPath on the provider.)

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Oct 16, 2018

Huh I'm getting this error when trying to test out the changes. I don't know why I definitely pulled and linked both the settings-view and snippets pr. I'll check more tomorrow.

Uncaught TypeError: this.snippetsProvider.getUserSnippetsPath is not a function
    at PackageSnippetsView.writeSnippetToClipboard (/I:/atom_master/settings-view/lib/package-snippets-view.js:212)

I noticed another thing. The bundled snippets in the snippets package do not show up in the settings for the snippets package so those can't be disabled or copied. I am not sure if they have ever shown up in the settings. This is not something that we have to address now and can open an issue for.

@@ -40,6 +40,10 @@ describe "InstalledPackageView", ->
snippetsTable = null
snippetsModule = null
# Relies on behavior not present in the snippets package before 1.33.
# TODO: These tests should always run once 1.33 is released.
shouldRunScopeTest = parseFloat(atom.getVersion()) >= 1.33

This comment has been minimized.

@daviwil

daviwil Oct 16, 2018

Member

This is slightly horrifying but I'm surprised to find that it works pretty well for this purpose even with versions like 1.33.0-beta3 :) Since we're using the pattern in other repos I think it's fine to use it here too.

This comment has been minimized.

@Ben3eeE

Ben3eeE Oct 16, 2018

Member

Haha, yeah. We get many commenting about this. And also the fact that 1.5 > 1.33. But we still use it :)

@savetheclocktower

This comment has been minimized.

Contributor

savetheclocktower commented Oct 16, 2018

Huh I'm getting this error when trying to test out the changes. I don't know why I definitely pulled and linked both the settings-view and snippets pr. I'll check more tomorrow.

I bet that's a legit error. It occurred to me that I didn’t actually test out the functionality after that change — I only ran the specs. But the specs use a mock snippets provider, so I haven't tested the integration at all. I'll look at that shortly.

@savetheclocktower

This comment has been minimized.

Contributor

savetheclocktower commented Oct 17, 2018

I noticed another thing. The bundled snippets in the snippets package do not show up in the settings for the snippets package so those can't be disabled or copied. I am not sure if they have ever shown up in the settings. This is not something that we have to address now and can open an issue for.

I think this happens for any package that is currently apm link'd in dev mode. The package's path is the symlinked version (~/.atom/dev/packages/foo), but the snippets it should be recognizing as belonging to package foo are all reporting the actual, resolved path of foo's snippet's file.

@savetheclocktower

This comment has been minimized.

Contributor

savetheclocktower commented Oct 17, 2018

Anyway: fixed the error @Ben3eeE reported (settings-view wraps the snippetsProvider for some reason) but CI is still being ornery. I’m not sure why it fails on AppVeyor. The previous iteration passed on stable but failed on beta, and now it's failing both.

@simurai

Ben3eeE Do you have any feedback on the design here? I saw you commented in #1076 and said it looked good.

Yes, still looks good. 👍

@savetheclocktower

This comment has been minimized.

Contributor

savetheclocktower commented Oct 17, 2018

Oh, the test in question is flaky (see #973). I feel slightly better about this.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Oct 17, 2018

It's green after rebuilding twice 👍 Seems like this test is very flaky. I'll pull and test all the changes later today to see that everything works before merging.

'body': '${body}'
`
} else {
body = body.replace(/"/g, `\\"`).replace(/\n/g, '\\n')

This comment has been minimized.

@Ben3eeE

Ben3eeE Oct 17, 2018

Member

Oops. I completely forgot that tabs should be replaced with \t when writing my review comment about newlines. The article snippet copies the newlines correctly but the tab character is still there.

This comment has been minimized.

@savetheclocktower

savetheclocktower Oct 17, 2018

Contributor

I almost asked about \t also. 😁️ Will fix.

@daviwil

This comment has been minimized.

Member

daviwil commented Oct 17, 2018

Kicked off the AppVeyor build again to see if it goes green. Is there anything else that needs to be done for this PR?

@Ben3eeE Ben3eeE merged commit 5915845 into atom:master Oct 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Oct 17, 2018

@savetheclocktower Thank you for contributing 🙇 This is going to be a great addition 💯

@savetheclocktower savetheclocktower deleted the savetheclocktower:apd-fix-snippets-view branch Oct 17, 2018

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