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

[wip] Add completion configurer #328

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Aug 30, 2020

References

Code changes

  • moved the config widget next to the completion feature in jlsp
    • reduces duplication of the category name for command palette
    • also moved the style
  • got the immediate round-trip to work for theme
    • added get_current_theme_id and a current_theme_changed signal to the IconThemeManager
    • the completion plugin listens for this
  • added a command for _Set Completion Icon Theme, starting the CommandIds` pattern
  • add command palette items for themes as registered
  • added the license, pretty name.
    • i wonder about making the ids redmond and mountainview 😈
    • add modifications? to theme definition for licensing compliance
  • add dummy themes for kernel and null
  • thinking harder about the layout...
    • it would actually be simpler if it was a wide table, rather than long.
    • or could be "cards" that wrap, though would lose the comparison advantage of tables, and hover/select is kinda gross
    • this will force thinking about scroll sooner... since we know how big the icons are, it's a little easier
  • started looking at the icon color schemes
    • will also get the theme manager dependency out
    • started a notebook to find/replace the colors based on color scheme... regarding two-tone, we could have "base" and (optional) "accent" icon classes
  • hoist color scheme implementation

User-facing changes

Instead of a dialog, presents a main area widget, and offers appropriately interactive direct manipulations of all of the items (no save button)

Screenshot from 2020-08-30 10-36-47

Backwards-incompatible changes

A number of things changed...

  • making things-we-might-have-n-of asynchronous
  • replacing some ILabIcon with LabIcon (they aren't really compatible)

Chores

  • linted
  • tested
  • documented
  • changelog entry

Discussion

While I actually enjoy this old school kind of work, and tweaking how every pixel looks, doing this per feature is going to be insane, especially once we add per language overrides, etc. It might be fine to iterate on, test, and land this so we have something to look at, and then refactor with RJSF. We would be able to refactor the existing "novel" things (e.g. the icon preview, the token finder) work, and make them stuff that shows up in our uiSchema, so we'd get the best of both worlds.

@krassowski
Copy link
Member

Well if we are adding continuous hinting and documentation here, we indeed may start looking into the other features as well. My rationale of implementing the themes preview at all was only to gather the icons in one place and display the licence and wait for the upstream effort of bringing configuration GUI. I am afraid that if we go too far with the settings configuration we may end up having two GUIs for that...

@krassowski
Copy link
Member

krassowski commented Aug 30, 2020

Though yeah, the layout for icons is looking nice. Note: "suppress in" is not only about continuous hinting (after each keystroke) but also about auto hinting after trigger characters (which are provided by the LSP server) - for example this means that pressing dot in a comment will not bring the completer in a Python file.

@krassowski
Copy link
Member

krassowski commented Aug 30, 2020

So it could be:

  • "Hinting"
    • enable continuous
    • suppress auto-invoke in

packages/completion-theme/src/types.ts Outdated Show resolved Hide resolved
packages/theme-vscode/style/icons/file.svg Show resolved Hide resolved
@@ -1,3 +1,3 @@
<svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M2 5H4V4H1.5L1 4.5V12.5L1.5 13H4V12H2V5ZM14.5 4H12V5H14V12H12V13H14.5L15 12.5V4.5L14.5 4ZM11.76 6.56995L12 7V9.51001L11.7 9.95996L7.19995 11.96H6.73999L4.23999 10.46L4 10.03V7.53003L4.30005 7.06995L8.80005 5.06995H9.26001L11.76 6.56995ZM5 9.70996L6.5 10.61V9.28003L5 8.38V9.70996ZM5.57996 7.56006L7.03003 8.43005L10.42 6.93005L8.96997 6.06006L5.57996 7.56006ZM7.53003 10.73L11.03 9.17004V7.77002L7.53003 9.31995V10.73Z" fill="#75BEFF"/>
<path fill-rule="evenodd" clip-rule="evenodd" d="M2 5H4V4H1.5L1 4.5V12.5L1.5 13H4V12H2V5ZM14.5 4H12V5H14V12H12V13H14.5L15 12.5V4.5L14.5 4ZM11.76 6.56995L12 7V9.51001L11.7 9.95996L7.19995 11.96H6.73999L4.23999 10.46L4 10.03V7.53003L4.30005 7.06995L8.80005 5.06995H9.26001L11.76 6.56995ZM5 9.70996L6.5 10.61V9.28003L5 8.38V9.70996ZM5.57996 7.56006L7.03003 8.43005L10.42 6.93005L8.96997 6.06006L5.57996 7.56006ZM7.53003 10.73L11.03 9.17004V7.77002L7.53003 9.31995V10.73Z" fill="#75BEFF" class="jp-icon-brand1" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid going this route. This is certainly a modification (needs to check license) and there are plans to change these variables, see jupyterlab/jupyterlab#8832 (comment). I would create a set of dedicated variables fully reflecting the exact colors as shipped by MS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the upstream variable names change, we'll update them, and it will work for all themes. If we make new variables, every theme will have to (read: won't) adapt to them. The primary motivating case: if a theme designer decides they need the vscode purple as their background, all the purple icons would disappear. However, as that purple shows up in, for example, the "new markdown file" icon, they'll realize that they have to own the rest of the colors as well, and create an overall harmonious experience: we aren't building vscode.

If we do (I don't) want to build something that beat-matches vscode, it would be a theme, and could define jp-icon-whatever to be exactly that color.

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Aug 30, 2020

we indeed may start looking into the other features as well... may end up having two GUIs for that...

Gotta have one first, and starting with a complicated/rich one is as good as any.

Edit: there's an upstream issue around this.

I don't know where their design stuff is going, as if it doesn't work on top of rjsf/formik/etc. it will take a really long time to implement. Choosing/building something that doesn't take any JSON schema, give consistent form and allow extension author to customize per widget (like our theme preview) is broken, no matter how pretty it is.

The roadmap I see for #196 and related:

  • make completion settings feel really good
  • add jupyterlab-rjsf, figure out how to handle customizations for completion (should set us up well)
  • get basic forms for "free" for all of our features, initially as separate commands/screens
  • add per-language/language-server customization
  • point to these in the discussion on the upstream issue
  • wait for the upstream to provide The One Settings Editor (who knows, might be me what does the work)

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Aug 30, 2020

"Hinting"

  • enable continuous
  • suppress auto-invoke in

Didn't dig too much into these... so does continuous affect suppress in any way?

but also about auto hinting after trigger characters (which are provided by the LSP server) - for example this means that pressing dot in a comment will not bring the completer in a Python file.

Ah. Well, we probably need some more explanation around this... landing it in the schema as text would be good, but we might end up needing an SVG/HTML preview animation or something to show how these work.

Oh, and in preparation for per-language features, all of the features probably need a user-serviceable enabled (default: true) key (which can eventually be overridden per-language)... I'm thinking the general structure will be:

properties:
  global:
    title: Global <Feature> Settings
    $ref: "#/definitions/settings"
  language:  
    title: Per-Language <Feature> Settings
    patternProperties:
      ".*":
        $ref: "#/definitions/settings"

definitions:
  settings:
    title: <Feature> Settings
    enabled:
      type: boolean
      default: true

...then the FeatureSettings can _.mergeWith the composite of global and <language>.

Another thing on the settings: we should maybe make set take a Partial<T>, as right now the key completion works (yay!) but the value is a union of all possible types. We may also want to move schema into src so it is available at runtime...

@krassowski krassowski changed the base branch from features-refactor to master August 31, 2020 12:56
@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Sep 4, 2020

Hopefully unbroke the existing tests... will proceed tomorrow/this weekend with the new ones...

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Sep 4, 2020

Also, newest black went ape on our python... i generally like it, but we should probably resolve that on a separate PR and/or do the yarn.lock refresh...

@bollwyvl bollwyvl closed this Sep 5, 2020
@bollwyvl bollwyvl reopened this Sep 5, 2020
@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Sep 5, 2020

Not sure what's going on in windows... might need to try some stuff like https://www.crummy.com/software/BeautifulSoup/bs4/doc/#inconsistent-encodings

atest/logcheck.py Outdated Show resolved Hide resolved
@krassowski
Copy link
Member

Looks better now! The failure at the moment is:

NotADirectoryError: [WinError 267] The directory name is invalid: 'D:\\a\\1\\s\\atest\\output\\windows_37_4\\user-settings\\@krassowski\\plugin id=@krassowski\\jupyterlab-lsp:completion'

which I guess might because Configure JupyterLab Plugin takes plugin id, while Reset Plugin Settings takes plugin (but is being called with plugin id).

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Sep 8, 2020

Indeed, that would break things! I'll look at normalizing how we work with/clean up the settings ids/files...

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Sep 8, 2020

ok, affb602 and some follow-on should have us back up on robot... we should be better set up to handle more plugins, in the future, too. Re-running a full build locally with fresh envs, but will be in and out for a bit...

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Sep 8, 2020

Just watching the windows build, it has at least gotten past the settings cleanup stuff, as well as some places where the log reading was dying before...

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Sep 8, 2020

So everything's looking good, thus far, but we're running up on the 60 minutes (Duration: 57m 36s) with a single fail on the first trip through win38/win37... and I'm about to actually add a few more tests for the new stuff.

A quick win might be not running the jest tests on windows: https://github.com/krassowski/jupyterlab-lsp/issues/311#issuecomment-688951006

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Sep 8, 2020

Oh, and full local run completed.

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

Successfully merging this pull request may close these issues.

2 participants