Skip to content
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

phantoms cannot be dismissed / this feedback style too obtrusive #23

Closed
braver opened this issue Aug 19, 2017 · 8 comments
Closed

phantoms cannot be dismissed / this feedback style too obtrusive #23

braver opened this issue Aug 19, 2017 · 8 comments
Milestone

Comments

@braver
Copy link
Contributor

braver commented Aug 19, 2017

I tried this on a python project that has a lot of pycodestyle errors (indentation mostly). While perhaps I should do something about those in the first place, this makes it pretty hard to work with (and pretty hard to do something about the errors 😉) :

screen shot 2017-08-19 at 12 20 19

Sublime's standard build result phantoms can be dismissed, and then cycled through with next_result and prev_result.

Showing unrequested feedback using phantoms is really obtrusive. All code has to shift to make place for the phantoms, which makes for a really bumpy ride if there is more than one phantom. Pretty much unusable.

bumpy-ride

I would suggest using linter-style (squiggly underline and/or gutter icons) feedback during editing, and then display the phantoms using a command. Perhaps the current behaviour should be optional, but it's easy enough to add a trigger for a "show phantoms" command. I definitely don't think it should be the default behaviour.

@rwols
Copy link
Member

rwols commented Aug 19, 2017

It should be configurable in the settings: https://github.com/tomv564/LSP/blob/master/LSP.sublime-settings#L43

@tomv564
Copy link
Contributor

tomv564 commented Aug 20, 2017

I feel your pain with the default linters from python-language-server, there is an outstanding issue to make them configurable here: palantir/python-language-server#57.
The pycodestyle linter does already try to read any configuration / comments in code to ignore certain rules.

As for LSP itself:
As @rwols pointed out, adding show_diagnostics_phantoms: false to your LSP package settings should result the linter-style feedback being shown all the time.

Dismissing the phantoms is tricky to implement as the diagnostics they are based on will be recreated every time you edit the document.

The linter-style feedback may not be visible unless you have SublimeLinter installed, I will create an issue to find a good scope to use that works with many color schemes.

@braver
Copy link
Contributor Author

braver commented Aug 20, 2017

I feel your pain with the default linters from python-language-server

Well, it's just an example where there is more than one error. Silencing the linter is not the solution, because the linter is not the problem, the UX of diagnostics_phantoms is just not very good.

adding show_diagnostics_phantoms: false to your LSP package settings should result the linter-style feedback being shown all the time

Wouldn't that make them never show at all?

Dismissing the phantoms is tricky to implement as the diagnostics they are based on will be recreated every time you edit the document.

You're describing the problem here: diagnostics are run all the time and that will make phantoms appear and disappear all the time.

What I'm saying is, you shouldn't use phantoms this way, not because of this particular linter, but because they're super disruptive and they get in the way of editing code. If I wanted an annoying tool that gets in the way I'd be using an IDE 😉

Phantoms are super useful when dealing with build errors, because they stay visible while editing and you can go through them one by one. However, the phantoms here disappear as soon as you start editing.

I don't think a setting to switch them off is going to solve anything, the approach to UX needs to change completely. You already have the same info in a popup on hover, which is excellent. You can have a marker in the gutter to tell you something is wrong in a line. I understand Sublime doesn't let you do the squiggly underline via a nice api, but you could do worse than lean on SublimeLinter here. It's an excellent package that solves the unobtrusive feedback issue completely. And then, when I tell LSP to "show me everything that's wrong with this file", that's a good moment to show the phantoms. Have the phantoms stick around while I'm editing, and let me dismiss them when I'm done with them.

Perhaps I'm so used to working with SublimeLinter and the built-in build system that I expect it to work that way. But this package takes over much of what those features already offer, so why shouldn't it mirror those features? They've been polished for years and mostly get it right. LSP should at least be just as focussed on supporting your workflow instead of interrupting it.

@tomv564
Copy link
Contributor

tomv564 commented Aug 20, 2017

Agree 100% that SublimeLinter gets it right, and I think LSP covers the gutter + squiggly marks functionality? I see squiggly lines and gutter marks in your GIF, and this is also what LSP produces in this screenshot:

screen shot 2017-08-20 at 18 48 41

And then, when I tell LSP to "show me everything that's wrong with this file", that's a good moment to show the phantoms. Have the phantoms stick around while I'm editing, and let me dismiss them when I'm done with them.

That does sound like a nice workflow when working with a build system. I have to think about how this fits with an always-updating language service. You don't want the phantoms to update while you're in "fixing mode", so how do you want to be notified of new errors you are creating?

@braver
Copy link
Contributor Author

braver commented Aug 20, 2017

I see squiggly lines and gutter marks in your GIF, and this is also what LSP produces in this screenshot:

I wasn't sure if that was because of SublimeLinter running in the background, but if LSP already provides this that's excellent.

You don't want the phantoms to update while you're in "fixing mode",

You're probably right about that...

so how do you want to be notified of new errors you are creating?

Isn't that what the squigglies are for? That's what the linter does: squigglies update live and if your caret is in a problematic region the message is show in the statusbar. I expect LSP might be able to get more detailed messages and auto-fix actions, so would want to be able to open a popup (as in your screenshot) to deal with the error at my caret.

Perhaps "fixing mode" could be a mode you enter explicitly, and a slightly different behaviour that gives more visibility to the errors could work here while it doesn't during normal editing. Exiting "fixing mode" could be as simple as dismissing the phantoms.

Phantoms that change position are super annoying and you definitely don't want to remove and restore them "on change". Ideally, but I'm not sure that's even possible, you'd want to just remove the error that has just been solved.

The language service isn't fully continuous right? If I'm right it waits for a certain action (or perhaps a timeout) before updating the diagnostics. Now it removes all phantoms as soon as you start editing, and when you're done they return. So while you're editing you lose the visual info of exactly what you're fixing, and possibly the next problem you might want to go to and fix. It would be nice if it removes the phantom for problem A as soon as I fixed that one, while keeping the phantom for problem B visible at all times so I can go there and continue working through the errors. Now this might be stretching what the phantoms system was designed for, I'm just thinking aloud about what the interaction could be here.

@tomv564
Copy link
Contributor

tomv564 commented Aug 20, 2017

How about the following:

  • A "Create Phantoms" command that creates phantoms from a diagnostics snapshot
  • We restore the close button on the phantoms so you can dismiss them individually.
  • A "Dismiss Phantoms" command that gets rid of all of them.

The current behaviour could then be made configurable with:
"create_diagnostics_phantoms_on_save": true
"dismiss_diagnostics_phantoms_on_dirty": true

And in your case you'd probably want both of those set to false.

I think this a good attempt to deliver the best of both worlds. I hope for a little more feedback from you and other users before starting on this feature.

In the meantime, there are configuration options for pycodestyle in this issue: palantir/python-language-server#93
If you're determined to short all those lines, then the Format Document command might be worth a try.

@braver
Copy link
Contributor Author

braver commented Aug 20, 2017

A "Create Phantoms" command that creates phantoms from a diagnostics snapshot

👍

We restore the close button on the phantoms so you can dismiss them individually.

I'm not even sure the api allows that, I think you have to dismiss (and then recreate) them all. But I haven't dived too deeply into what you can do here.

Another thought: after creating the phantoms, update/reload them on save events until they're dismissed. I'm not sure how jumpy that would be, but it's a way to be more interactive/live than traditional build output.

I hope for a little more feedback from you and other users before starting on this feature.

Right now disabling the phantoms doesn't remove anything too valuable, so that's good enough option for me. Perhaps that's also telling (why is the feature) even there?), but it might also be just my opinion.

I've never been a heavy user of LSP-like features, mostly because most implementations in editors are annoying and get in the way of writing code, and IDE's are seemingly all about getting in the way. I think LSP is super promising, so I'll definitely share opinions and ideas.

If you're determined to short all those lines, then the Format Document command might be worth a try.

Yeah, this particular file isn't something I maintain, but a good test case. It has a flake8 config, pycodestyle takes a similar config file and that also solves it. Realistically, if there is real time linting and perhaps format on save, you don't have to solve too many errors at once unless you're introducing the tests/linter to an existing project.

@tomv564
Copy link
Contributor

tomv564 commented Aug 28, 2017

Phantoms have been disabled by default by #37, a new issue was created to explore suggestions in this issue.

@tomv564 tomv564 closed this as completed Aug 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants