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

Reimplement the depext UI with a menu #5053

Merged
merged 9 commits into from
Apr 19, 2022
Merged

Reimplement the depext UI with a menu #5053

merged 9 commits into from
Apr 19, 2022

Conversation

AltGr
Copy link
Member

@AltGr AltGr commented Feb 9, 2022

Closes #5026
Additionally, also shows the hint on C-c :)

@AltGr
Copy link
Member Author

AltGr commented Feb 9, 2022

Here is what it looks like:
image

@rjbou rjbou added the AREA: UI label Feb 9, 2022
@rjbou rjbou added this to the 2.2.0~alpha milestone Feb 9, 2022
@AltGr
Copy link
Member Author

AltGr commented Feb 10, 2022

Pushing a small style update. Here are a few screenshots to show the interaction:

Trying to do it manually, then ignoring:

image

With depext-run-installs=false (different menu)

image

The code is a little spaghetti, but there is probably no better way. I have the before/after flow charts (on paper!) if you are interested ^^

@rjbou rjbou requested a review from dra27 February 11, 2022 10:04
@dra27
Copy link
Member

dra27 commented Feb 11, 2022

I think is marvellous 🙂 It's not crystal clear (to me) for the sentences - I read the partial sentences w.r.t. the opening menu sentence (e.g. "Let opam run apt-get to install the required system packages? ... and abort" rather than "[quit]................. and abort"

I wonder if numbered options and a full sentence for each works more clearly:


opam believes some required external dependencies are missing. opam can:

  1. (*) Run apt-get to install them (may need root/sudo access)
  2. Display the recommended apt-get command and wait while you run it manually (e.g. in another terminal)
  3. Attempt installation anyway, and permanently register that this external dependency is present, but not detectable
  4. Abort the installation

@dra27
Copy link
Member

dra27 commented Feb 11, 2022

For the manual menu, I'd suggest:


This command should get the requirements installed:

apt-get install libzmq3-dev

Would you like opam to:

  1. Check again, as the package is now installed
  2. Attempt installation anyway, and permanently register that this external dependency is present, but not detectable
  3. Abort the installation

@AltGr
Copy link
Member Author

AltGr commented Feb 11, 2022

I wonder if numbered options and a full sentence for each works more clearly:

Probably so, although in my display the brackets + table alignment didn't help with that, so we could have fiddled with that as well.
One of the ideas behind the y/n/i/q was to have y/n be consistent with existing questions, but I don't know if there is a point in that (keeping the same behaviour for "default" on being "y"; although n of most y/n questions would be closer to "abort" in this case.
The big plus of numbers is that it's rather obvious, and easy to read with much less clutter.

@AltGr
Copy link
Member Author

AltGr commented Feb 11, 2022

image

@dbuenzli
Copy link
Contributor

I find the > prompt slightly strange (maybe because that's my shell prompt) and a bit inconsistent with the usual way opam asks for stuff.

Just saying, I can live with that.

@AltGr
Copy link
Member Author

AltGr commented Feb 11, 2022

Just saying, I can live with that.

I already had doubts about it, your input is welcome.

@AltGr
Copy link
Member Author

AltGr commented Feb 11, 2022

menu
😁

@AltGr
Copy link
Member Author

AltGr commented Feb 14, 2022

I'd be happy to see how it fares on MacOS shells; but I am actually doing the minimum in terms of escape codes magic so it shouldn't be too much trouble. And at worst, there will be a little garbage on top of the menu when moving around, but it will still be readable; or the arrow keys just won't work, which is an ok fallback.

@AltGr
Copy link
Member Author

AltGr commented Feb 18, 2022

image
(this is when depext_run_installs is disabled)

@kit-ty-kate
Copy link
Member

On macOS the arrows work fine but some thing that throws me a bit off is that it seems that the prompt is waiting for only one character.

For example is you go and input 2 (only one character, no clicking enter) it directly does the action instead of waiting for the user to tap the enter key.

In the same kind of behaviour, if you type anything but one of the character that the prompt is waiting for, nothing happens and your character does not appear. This feels kind of stressful, even claustrophobic a little, it feels like opam just broke and froze your computer (^C still works but still)

@kit-ty-kate
Copy link
Member

In the same kind of behaviour, if you type anything but one of the character that the prompt is waiting for, nothing happens and your character does not appear. This feels kind of stressful, even claustrophobic a little, it feels like opam just broke and froze your computer (^C still works but still)

the same freezing behaviour happens on a simple installation prompt. The y (default) character is already there but no character appears if you type anything.

I think we should keep the old [Y/n] <no character> type prompt as well

@AltGr
Copy link
Member Author

AltGr commented Apr 14, 2022

Rebased; windows test broken due to last cygwin/git update (unrelated to this PR)

AltGr added 7 commits April 15, 2022 16:31
plus some goodies like:
- intuitive display of the default choice (it's already printed left of the
cursor so it's clear you just have to `<enter>`)
- fixed and generalised handling of `echap`

Handling arrows in menus would be cool and not so difficult (just need to
rollback n lines and clear lines), but I am afraid of portability...
Alright I had to do it; "it works for me", and actually the menus don't appear
on Windows (yet?) so... :D
AltGr added 2 commits April 15, 2022 16:31
- split to a separate function for easier porting (Windows support would also
  require correctly detecting arrow key presses though)
- clear all lines to avoid glitches if the terminal is resized while in the menu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

depext: opam tries to be intelligent but fails; permit to choose the system package manager
5 participants