jupyter-lsp / jupyterlab-lsp

Coding assistance for JupyterLab (code navigation + hover suggestions + linters + autocompletion + rename) using Language Server Protocol
https://jupyterlab-lsp.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.8k stars 149 forks source link

Improve the statusbar indicator #85

Open krassowski opened 4 years ago

krassowski commented 4 years ago

Follow up on #78:

bollwyvl commented 4 years ago

Really awesome stuff! I'll take a closer look at the nuts and bolts when I get a chance.

Some things that jump out at me:

bollwyvl commented 4 years ago

Also, here's another icon treatment without any derivatie branding, kinda shouting out the NASA worm and Star Wars: logo_3

(also derived from fira code)

krassowski commented 4 years ago

Also, we need to check if the virtual document/connection is still open, as when opening a notebook initially it has "plain" mimetype and only after kernel is loaded it changes (thus we get two virtual documents - the plain one sticks up there for some time and due to my policy of not destroying websockets/virtual documents immediately to keep overhead low for users having interspersed python and R cells - which would end up in a disaster once they start refactoring). Here is the problem captured:

Screenshot from 2019-10-31 13-39-49.

Re icons and design stuff: all great ideas; i would go for two indicators as you suggest - one for overall LSP condition and second one for the LSP features - those could be two icons + some text; those could even have separate pop-ups/click actions. Stacking languages vertically in the pop-up is certainly a good idea.

Do you think that someone from the core JupyterLab team would be interested to take a look at what we are doing in here and provide some guidance?

bollwyvl commented 4 years ago

Also, we need to check if the virtual document/connection is still open, as when opening a notebook initially it has "plain" mimetype and only after kernel is loaded it changes (thus we get two virtual documents - the plain one sticks up there for some time and due to my policy of not destroying websockets/virtual documents immediately to keep overhead low for users having interspersed python and R cells - which would end up in a disaster once they start refactoring).

To the earlier point, I think we just want zero to one websocket per language, which opens when asked for and stays open until closed explicitly, with the server staying up even after browser closing, similar to how kernels work.

Here is the problem captured:

[image: Screenshot from 2019-10-31 13-39-49] https://user-images.githubusercontent.com/5832902/67953302-c359c180-fbe6-11e9-828f-61e07cfda394.png .

Re icons and design stuff: all great ideas; i would go for two indicators as you suggest - one for overall LSP condition and second one for the LSP features - those could be two icons + some text; those could even have separate pop-ups/click actions.

I think one number, one icon, and the language words are about all the real estate we can take... And that's a lot. It's going to end up like the old notebook toolbar down there, if everyone takes that much, so we have to deliver a lot of value.

We could even cut down on words with language icons: there are some consistent language icons coming down the pipe on some other work, though, so it's probably best to stick with words for now.

Do you think that someone from the core JupyterLab team would be interested to take a look at what we are doing in here and provide some guidance?

For what it's worth, I have commit rights over there, and a fair number of contributions in. The steps in #76 are what I consider the bare minimum before we should even propose the work to the broader developer community (e.g warm up the JEP). At that point, we could likely get it added to the lab org, which would be good marketing.

Luckily, in this community, code wins: if it looks like core, is documented, tested, extensible, and generally useful to the whole range of users, there's no reason we shouldn't be able to get it in. The biggest strike against it is the undesirable externality of requiring more things we can't control (language servers) but there's many developer years locked up in all those servers that the jupyter community won't have the time to reproduce: if you can't beat them, integrate them!

This work would almost certainly have to land at a major release, and it is unlikely we will be able to have this be bullet-proof, production ready by 2.0 (dec-jan). We could start maintaining/testing a branch against the lab 2.0 branch, but again, that won't be in anyone's hands for some time. So continuing to mature the code as an extension is a very good path indeed.

krassowski commented 4 years ago

Current design for the popover

I was thinking about listing connections grouped by LSP servers (this usually makes sense) like this:

---------------------------------------------
| **LSP Servers**                           |
|-------------------------------------------|
|  > Installed/Available (8)                |
|  v Running (2)                            |
|     Python (pyls)               [cog] [o] |
|     - x.ipynb (NB)                    [o] |
|     - %%timeit (cell)                 [o] |
|     R (languageserver)          [cog] [o] |
|     - x.ipynb (NB)                    [o] |
|  v Missing (1)                            |
|      Latex                            [?] |
|-------------------------------------------|
|  a longer status message                  |
--------------------------------------------------------------------------------------
| [LSP icon] 2 connected, 1 missing [o] |                           (stausbar)
--------------------------------------------------------------------------------------

where:

In addition, specific features will be able to:

  1. display temporary messages in a shared area
  2. add custom statusbar indicators

For example:

                              ------------------------------
                              | Diagnostics/linter popover |
--------------------------------------------------------------------------------------
| [LSP icon] 3 connected [o]  |  1 error, 2 warnings  |   Renamed X to Y in two places
--------------------------------------------------------------------------------------

where

having the linters messages summary in the status bar makes sense:

bollwyvl commented 4 years ago

Looking good, thanks for taking the time to make some ASCII art! It's very therapeutic.

My general guidance (with an eye towards getting this into lab) is we should try:

Specifics:

popovers: I don't feel like popovers are used widely in lab, nor are the very accessible. they are also hard to test, because they end up attached to the root. anything more than a single line, or anything with more than one click surface should probably end up in a (dismissable) sidebar.

running: If we can get our servers into lab's Running tab, that would be best, even if it doesn't provide all the bells and whistles... but that feature landed only recently.

status bar: Again, we can't take that much of the status bar real estate, or they have to be broken up and individually disable-able. At most, we could show that there are error/warnings, and delegate the rest to dedicated views. A diagnostic overview would be great, i could totally see having it sidebar'd while doing a tricky refactor in several documents. It should not be in the same view as the language server status, though, as if everything is working, you won't care at all about language servers, just your code.

gutters: i guess i don't understand what a "global" gutter is. I added gutters over on lintotype... I can bring that functionality over, if desired. It's just css and html, so we can do whatever the heck we want with it.

packaging: I think a lot of these things are starting to look like individual packages that all communicate through the ILanguageServerManager interface over phosphor signals/messages. So from what I'm seeing, in addition to the initial ones:

installation: Installing all these piddly little packages sounds annoying. We can have a language-servers-kitchen-sink that can install everything, but I think we'll make enough wrong choices that it will be worth leaving as many pieces of UI as we can open to re-interpretation.

krassowski commented 4 years ago

Subjective (counter)points:

I will add more thoughts as they come, this is what I got in the lunch break. Please continue adding your constructive criticism, this is much needed (even when I disagree)!

krassowski commented 4 years ago

@ellisonbg, @tgeorgeux - I've tried to do some ASCII design for JupyterLab-LSP statusbar. We have some interesting discussion here - if you are interested in chiming in you are more than welcome!

bollwyvl commented 4 years ago

gutters: I know that bringing gutters to code mirror is not so difficult. I meant that we cannot (easily) to this thingy (see the icon in the top right) in notebook:

Here's a self-contained, awful example, which nonetheless could be implemented (much better) in react... or d3... or... Screenshot from 2019-11-05 23-39-22

packages

My point about all the APIs and interfaces and packages is, if someone wanted to make jupyterlab-lsp-notebook-gutter, what API would they need to get all the diagnostic/current token positions in the concrete document, offer a useful hover message, and jump to that position, without having to dig through (presently) around 6k lines of code. Or, if it's important enough to add to notebook (i'd probably agree it is) how would we get it added to NotebookPanel properly, and then become a consumer of it.

I'm not suggesting a full refactor of everything (well, I am, eventually on #76), merely starting new features (like #98) as new packages if they will have worth to a downstream.

popovers: the very language (mode) picker for file editor in statusbar uses popover.

Ha, despite working in lab most of the time (at least on certain projects), because I don't use the status bar, I didn't know that! I'm just saying, it is hard to test them, and they are completely unreachable with the keyboard. Anyhoo... in the language picker case, it is like a traditional menu (you pick a thing from many, it goes away). The ascii mockup has multi-level, collapsible tree control of potentially tens of items, and a number of icons close to the edge of the box.

kernel picker uses the annoying, obstructive dialog window;

I know, right?

I agree that a panel (on the right side, preferably)

That (finally) got promoted to a user-level choice (right click -> switch sidebar side). also in the screenshot above!

this is the maximal complexity we can get.

there's always something we won't have thought of that will make it worse.

krassowski commented 4 years ago

Another use case for quick "disable lsp for this document" button (in addition to slow node indexing): I get to work with some scripts written years ago which are full of yellow/red underlines due to the use of python2, two spaces for indent and other things from before pep8.