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

Jack-in without prompt #2073

Merged
merged 12 commits into from Feb 14, 2023
Merged

Conversation

SillyCoon
Copy link
Contributor

@SillyCoon SillyCoon commented Feb 9, 2023

What has changed?

Fixes #2049

Now you can set up the default project for Jack-in via config or after the Jack-in run by click on Yes.
I'm not sure that it's great UI/UX but it's pretty simple and the information window doesn't block other input.
Снимок экрана_20230213_203329
Снимок экрана_20230213_203538
Снимок экрана_20230213_203621

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
  • Added to or updated docs in this branch, if appropriate
  • Tests
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
  • Referenced the issue I am fixing/addressing in a commit message for the pull request.
    • If I am fixing the issue, I have used GitHub's fixes/closes syntax
    • If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
  • Created the issue I am fixing/addressing, if it was not present.
  • Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
  • Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik

@SillyCoon SillyCoon marked this pull request as ready for review February 9, 2023 12:04
@SillyCoon
Copy link
Contributor Author

Hi @PEZ! Please review this PR. I also added an integration test for it and I guess I need to edit the documentation. I wanted to record the video, but actually I was not able to do it today, so I can incude it somewhere tomorrow.

@PEZ PEZ requested a review from bpringe February 9, 2023 12:25
@PEZ
Copy link
Collaborator

PEZ commented Feb 9, 2023

Cool! I hadn't thought about this way to do it. I was stuck in how this used to work. I think I like this way better, because it is so straightforward.

We need to be a bit careful with pop-ups. This one is very helpful, but I can see how it could get annoying for someone closing it without selecting an option. Some alternatives:

  1. We could include information about the setting in the pop-up + add a button for ”Don't ask again”, which would mean ”Don't ask again for this workspace”.
  2. Include settings info in the pop-up and always only ask once per workspace.
  3. Don't pop-up anything. We inform about it in the docs.
  4. Don't pop-up anything. We inform about it in the docs, plus in the Output/REPL Window.

I haven't tested this myself yet, so here questions:

  1. Does it work with Custom REPL Connect Sequences?
  2. Does it also suppress the menu when connecting to a running REPL? I think it should, and that the setting probably should be renamed.

I requested review by @bpringe now, so that we can get his opinions on the Ux.

This is great work! 🙏

@SillyCoon
Copy link
Contributor Author

Cool! I hadn't thought about this way to do it. I was stuck in how this used to work. I think I like this way better, because it is so straightforward.

We need to be a bit careful with pop-ups. This one is very helpful, but I can see how it could get annoying for someone closing it without selecting an option. Some alternatives:

  1. We could include information about the setting in the pop-up + add a button for ”Don't ask again”, which would mean ”Don't ask again for this workspace”.
  2. Include settings info in the pop-up and always only ask once per workspace.
  3. Don't pop-up anything. We inform about it in the docs.
  4. Don't pop-up anything. We inform about it in the docs, plus in the Output/REPL Window.

I haven't tested this myself yet, so here questions:

  1. Does it work with Custom REPL Connect Sequences?
  2. Does it also suppress the menu when connecting to a running REPL? I think it should, and that the setting probably should be renamed.

I requested review by @bpringe now, so that we can get his opinions on the Ux.

This is great work! 🙏

Thank you for the great detailed answer!

I agree about pop-ups, it's a kinda slippery road. What do you mean by not pop-up, do you mean to set the selected project as a default for this workspace if we call Jack-in?

About testing. It should work with custom sequences and I guess it doesn't suppress the menu, but I will check it twice in a day or two and will let you know.
Thanks!

@PEZ
Copy link
Collaborator

PEZ commented Feb 10, 2023

Now testing this quickly:

  • Connecting I get this:
    image
    I don't think we should pop it up there. We can write it in the Output Window instead.
  • When pressing escape out of the Project Type menu, jack-in is aborted, but the question to save the default project type still pops up.
  1. Does it work with Custom REPL Connect Sequences?

Yes it does. ✅

  1. Does it also suppress the menu when connecting to a running REPL? I think it should, and that the setting probably should be renamed.

Yes, this works for Connect as well as for Jack-in. As it should. I suggest the setting be named calva.autoSelectReplConnectProjectType and the description could be: When set Calva will not prompt for a project type, at Jack-in or Connect. Instead the project name used here will be used. (Probably mostly for your Workspace Settings.)

What do you mean by not pop-up, do you mean to set the selected project as a default for this workspace if we call Jack-in?

I mean to leave it up to the user to add the setting. If we do this we probably want to inform about the possibility in the Output window when connecting the REPL.

@SillyCoon
Copy link
Contributor Author

SillyCoon commented Feb 11, 2023

Now testing this quickly:

  • Connecting I get this:
    image
    I don't think we should pop it up there. We can write it in the Output Window instead.
  • When pressing escape out of the Project Type menu, jack-in is aborted, but the question to save the default project type still pops up.
  1. Does it work with Custom REPL Connect Sequences?

Yes it does. ✅

  1. Does it also suppress the menu when connecting to a running REPL? I think it should, and that the setting probably should be renamed.

Yes, this works for Connect as well as for Jack-in. As it should. I suggest the setting be named calva.autoSelectReplConnectProjectType and the description could be: When set Calva will not prompt for a project type, at Jack-in or Connect. Instead the project name used here will be used. (Probably mostly for your Workspace Settings.)

What do you mean by not pop-up, do you mean to set the selected project as a default for this workspace if we call Jack-in?

I mean to leave it up to the user to add the setting. If we do this we probably want to inform about the possibility in the Output window when connecting the REPL.

Thank you very much for the testing and quick reply! I will fix the issues soon.

About UX. Personally, I prefer the option with pop-up suppression (Don't show it again for the current workspace), but it would be nice to hear other contributors' opinions as well.

The problem with the manual setting is that we can't suggest only available sequences for the project to choose from. My original thought was to provide the dropdown with the options in the config, but it seems impossible to do since VS Code doesn't support changing the list of available options in settings dropdowns in runtime (or I didn't find a way to do it).

package.json Outdated Show resolved Hide resolved
@bpringe
Copy link
Member

bpringe commented Feb 11, 2023

@SillyCoon Thanks for the PR!

I requested review by @bpringe now, so that we can get his opinions on the Ux.

@PEZ I think we can go with this:

We could include information about the setting in the pop-up + add a button for ”Don't ask again”, which would mean ”Don't ask again for this workspace”.

It seems fairly simple and very little code to maintain overall, plus this PR is already geared toward this.

SillyCoon and others added 4 commits February 11, 2023 14:23
Co-authored-by: Brandon Ringe <12722744+bpringe@users.noreply.github.com>
Co-authored-by: Brandon Ringe <12722744+bpringe@users.noreply.github.com>
Co-authored-by: Brandon Ringe <12722744+bpringe@users.noreply.github.com>
@PEZ
Copy link
Collaborator

PEZ commented Feb 11, 2023

I'm still hesitating about the popup. Reasons:

  • General popup-avoidance, in the past we have had complaints on Calva that it pops open too much.
  • The jack-in and connect often prompts for more things after the project type and the popup then makes us prompt for two things at the same time. It gets confusing.
  • For new users it is sometimes they don't really know which project type they should select.
    • Then this question adds cognitive load.
    • They risk accepting a setting that is not the one they should use. This can put them in a strange state where they don't get the menu again, and might not know how to get it back, since they didn't do it themselves.

I think that for now it is better we print a message in the Output window about adding the setting. We can print the value there, and the user can copy it.

@bpringe
Copy link
Member

bpringe commented Feb 11, 2023

@PEZ I agree with your reasons. I think we should avoid the pop-up as well.

@SillyCoon
Copy link
Contributor Author

Can't argue with that, thanks for your help! I will move the message to Output Window then, should be pretty easy

@SillyCoon
Copy link
Contributor Author

@PEZ @bpringe I've replaced a pop-up with the Output Window. Please check the new screenshots and correct my descriptions of the settings.

@PEZ
Copy link
Collaborator

PEZ commented Feb 13, 2023

Thanks! I like your suggestions for Output window texts.

It is rather few people who read the output in this window, I think. In some other PR we can consider cleaning all that text it up and rather link to calva.io.

We might want to move the instructions for this setting there. Then we can elaborate a bit more on how it works. Not sure where, but I think that for now, just before this section on the Connect the REPL page: https://calva.io/connect/#monorepos-multiple-clojure-projects-in-one-workspace will do. Something like:

## Auto-select Project Type

You can make both Jack-in and Connect stop prompting you for project type in projects where you always want to use the same. The setting `calva.autoSelectReplConnectProjectType` is used for this. Here's how to use it:

1. Connect the REPL and manually select the project type/connect sequence you want to use.
1. In the Output/REPL window Calva will print about auto-selecting project type, including the value you need. **Copy this value**
1. In VS Code Settings, search for ”Calva auto” and you will find the setting
1. Make sure you are editing Workspace settings
1. **Paste the project type value you copied above**

Next time you connect the REPL, Calva will use this value and the project type prompt will not be shown.

To make Calva prompt for the project type, reset this setting (there's a cog icon there giving you an option to do this).

!!! Or edit Workspace Settings JSON
    You can of course also edit this setting manually in `.vscode/settings.json` for the workspace. ”Reset” is then equivalent to removing the setting from this file.

Then in the Output REPL window we can link to https://calva.io/connect/#auto-select-project-type, with short prompts like:

Connecting using "Leiningen” project type. You can make Calva auto-select this:

  • See <link>

Auto-selecting project type "Leiningen". You can change this from settings

  • See <link>

Project type "Bla bla” not found. You need to update the auto-select setting.

  • See <link>

For the last case there I think we should show the menu, which will then make us print the correct settings value.

WDYT?

@SillyCoon
Copy link
Contributor Author

Sounds great! I'll create the PR with the updated documentation later then.
But I don't understand the last point. If the project type is incorrect, you should pick the correct one like there is no setting in the config. You can copy the new value to the config instead of the wrong one, I guess it works as you wrote but maybe I'm wrong.

@PEZ
Copy link
Collaborator

PEZ commented Feb 14, 2023

If the project type is incorrect, you should pick the correct one like there is no setting in the config. You can copy the new value to the config instead of the wrong one, I guess it works as you wrote but maybe I'm wrong.

Sounds exactly like what I had in mind.

@PEZ
Copy link
Collaborator

PEZ commented Feb 14, 2023

I think the docs should be updated in this PR, btw.

@SillyCoon
Copy link
Contributor Author

I think the docs should be updated in this PR, btw.

sorry, I forgot that the code and the docs are in the same repo, will do it in this PR 😌

@SillyCoon
Copy link
Contributor Author

I've updated the docs and output.
As for me, it's useful to leave the setting string in the output directly to simplify copying, so it would be like this. What do you think?

image

@PEZ PEZ merged commit ae656ec into BetterThanTomorrow:dev Feb 14, 2023
@PEZ
Copy link
Collaborator

PEZ commented Feb 14, 2023

Thanks so much for this work, @SillyCoon! It's interesting this with settings. I start with ”no” when someone suggests a setting, because they add more work than I often first calculate with. They add cognitive load for the users too. And in supporting users, a layer of ”which settings for X are you using?” to all other permeations of things. As we can see with this little setting there is a lot to consider right off the bat.

That said, I know we made the right call here making this configurable. When testing this I noticed how nice jack-in got. Let's go for that connect prompt as well, which asks for port with a pre-populated text input.

I noticed a problem with this setting, as well as almost all Calva settings, when testing this PR. I tested in a workspace where I had several root folders. When I have set a default for auto-selecting project type, that was used regardless of which folder I was jacking in to. Because I had set it with the Workspace settings... Trying to set it on Folder settings didn't work because we lack "scope": "resource" on the settings objects in the manifest. What's more. Even when I added that it still didn't work! Because we don't read folder settings... We should fix this. Connect Sequences and a lot of other settings make sense to add here. I think maybe all settings deserve this treatment. CC: @bpringe @julienvincent

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.

Establish REPL connection without being prompted for the kind of project every time
3 participants