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

Recommended version #130

Closed
jonashaag opened this issue Nov 20, 2021 · 27 comments · Fixed by #144
Closed

Recommended version #130

jonashaag opened this issue Nov 20, 2021 · 27 comments · Fixed by #144
Assignees
Labels
enhancement New feature or request question Further information is requested
Milestone

Comments

@jonashaag
Copy link

jonashaag commented Nov 20, 2021

Hello there, I would like to use cpp-terminal in Mamba. Specifically we'd like to use it for a yes/no prompt with Ctrl+C support. What's the recommended version to use right now? Would you recommend using latest master, or v0.1, or would you consider publishing a more recent stable release?

@certik
Copy link
Collaborator

certik commented Nov 20, 2021

I would recommend the latest master. If you run into any issues, please let us know and we'll fix it. Also please give us any feedback that you have. We want to make sure it works for you.

@jonashaag
Copy link
Author

Thanks!

Master seems to be very actively worked on, for example in the README it says that this a header-only library but I see you've moved to .cpp files. I also see open issues and PRs about restructuring of the codebase. I wonder if it's better to wait until most of that is done and use the v0.1 release until then?

@certik
Copy link
Collaborator

certik commented Nov 22, 2021

Yes, @MCWertGaming has been working on restructuring into cpp and header files.

@jonashaag I think #129 is the main one to merge, and I think it is ready to be merged as is. Beyond that, @MCWertGaming do you expect any other big changes? It seems the other changes will be rather iterative. In particular, for the y/n prompt, I would imagine we would add some high level API for that into cpp-terminal itself, so from mamba's side, I think things should be rather stable.

@jonashaag why don't we start working on an example how to do this and put it into cpp-terminal? In the meantime the refactor will be done and we can then use it in mamba.

@jonashaag
Copy link
Author

That would be great. You can see the code we currently use in the PR that's linked above.

@jonashaag
Copy link
Author

@certik
Copy link
Collaborator

certik commented Nov 22, 2021

Perfect. I think that is straightforward, except:

#ifdef _WIN32
            response = strip(response);
#endif

Do you now the reason for this? Does the prompt command somehow return whitespace or is the issue that it returns \r\n instead of just \n at the end of the string?

@jonashaag
Copy link
Author

@wolfv was there trailing whitespace in the Windows prompt response? Ref mamba-org/mamba#543 and mamba-org/mamba#544 Will we be facing this with cpp-terminal as well?

@wolfv
Copy link
Member

wolfv commented Nov 22, 2021

I think this might be related to Windows \n\r endlines but I am not 100% sure.

@MCWertGaming
Copy link
Collaborator

MCWertGaming commented Nov 22, 2021

The current stage on the terminal is a bit complicated. We have split the original headers into smaller headers with a clear structure. Also moved the source into actual source files to fix the definition clashes between the windows library and those of other peoples projects.

Currently the splitting is almost done. I have merged #129, wich basically does the splitting for everything regarding the base.hpp and some internally used functions. Splitting the rest will take me a bit as the input requires the class that initializes the terminal - which we want to avoid so the input function can be called without requirering objects to be passed into functions. This process will take me a week or two, i think.

The next will be the Window class which needs a generall overhaul for the color management. I have created a proof of concept with @certik already for that. It might take me another week. But thats only important for you, if you plan on using the window class for managing the terminal.

So the usage of the master branch is a bit tricky for the next one or two weeks, but I'll get back to this issue as soon as the bare minimum is done.

If you want to use the old "header only" library, you can check it out here:
https://github.com/jupyter-xeus/cpp-terminal/tree/b8eaa1f555746b9cbe76e60b34afca19b862f48e
But i'd highly recommend using the master branch.

@certik
Copy link
Collaborator

certik commented Nov 22, 2021

Thanks @MCWertGaming, I appreciate your work on this.

Regarding the prompt API, let's simply add an example how to do this, and implement it in cpp-terminal. I bet this example will have some simple function or a class such as prompt, and the high level user API won't change.

@MCWertGaming
Copy link
Collaborator

I could create an example for that! But first I have to finish splitting the input and terminal preparation appart.

Doing a prompt is really easy though. Therre is the read() function which blocks until a key was pressed and the you have to check the char with an if() and you are done basically.

so:

#include <cpp-terminal/terminal.h>
#include <iostream>

int main() {
try {
       // enters raw mode to capture input without requirering the user to press enter
       // (not sure if it's desired for a prompt, but currently required. This will not be necessary with my next PR anymore)
       Terminal term(true, false);

       bool on = true;
       int key;
       while(on) {
           std::cout << "Proceed? [y/N]"
           key = term.read_key();
           if (key == "y")
                // proceed
         else if (key == "n")
               // stop
          else
              std::cout << "wrong input! Please try again\n";
      }
} catch (const std::runtime_error& re) {
      std::cerr << "Runtime error: " << re.what() << std::endl;
      return 2;
} catch (...) {
      std::cerr << "Unknown error." << std::endl;
      return 1;
}
return 0;
}

Something like that should work. Just make it a bit more fancy, maybe with colors or something :)

@jonashaag
Copy link
Author

Thank you! Actually I guess we want the prompt to require <Return> for confirmation.

@certik
Copy link
Collaborator

certik commented Nov 22, 2021

@MCWertGaming Mamba is already using cpp-terminal. All we need is to take the code and ensure it works with the latest master, and wrap it into some nice API.

@MCWertGaming
Copy link
Collaborator

MCWertGaming commented Nov 22, 2021

Oh okay, now I understand. Like I said, I'm working on finishing the spliting and restructuring. Tell me if you need additional features or something is not working. Updating from the header only cpp-terminal library the the current master is possible by changing the includes to the new headers. But if you have already cpp-terminal running. you can wait a few days until we pushed the next PR. Will look into finishing it this week with additional testing on windows due to the input class changes.

For the API, what are you thinking of? Something like a simple function for creating a yes/no prompt that returns if CTRL+C was pressed? That should be quickly implemented. @certik
That would mean that we have to create something like a "temporary raw mode". Basically turning it on for the input and off afterwards. What do you think?

@certik
Copy link
Collaborator

certik commented Nov 22, 2021

For the API, what are you thinking of? Something like a simple function for creating a yes/no prompt that returns if CTRL+C was pressed? That should be quickly implemented. @certik That would mean that we have to create something like a "temporary raw mode". Basically turning it on for the input and off afterwards. What do you think?

Yes, something like that. Once the example works, we can refine the API to make it as simple to use as possible.

@MCWertGaming
Copy link
Collaborator

Once #132 is merged, we will work on the prompt.

@MCWertGaming MCWertGaming added enhancement New feature or request question Further information is requested labels Nov 23, 2021
@MCWertGaming
Copy link
Collaborator

MCWertGaming commented Nov 24, 2021

#132 is merged since yesterday. I'm looking foreward into creating a first prototype for the prompt. @certik the multiline update of the prompt kind of broke what they are doing in mamba. Would you prefer to patch the prompt to make the multiline editing and the prmpt message optional / changable? Or should we make the second prompt independant from the first one?

Also I'm thinking that it would be nice to extend the y/n prompt a bit with like colors and a spelling indicator that is green/red wether if the given input is valid or not.

So my idea would be:

Proceed? [Y/n] > input
^         ^    ^ indicates if the input is right or wrong
|         | default value
| custom message

The prompt would only allow return, if no input, "yes", "y", "no" or "n" is entered and returns a value for "yes", "no" or abort. I'll try make other options than "yes" and "no" possible so it's really universally usable.

Is this in your interest @jonashaag and @wolfv? Or would you prefer to have something more simple?

@jonashaag
Copy link
Author

@wolfv Are we trying to keep the prompt identical to conda?

@certik
Copy link
Collaborator

certik commented Nov 24, 2021

I think the y/n prompt should probably be a special kind of API, say prompt_yn or something like that. And just do whatever Mamba needs, which likely is whatever everybody else would need also.

@wolfv
Copy link
Member

wolfv commented Nov 24, 2021

hey, yeah, I don't think we need anything crazy fancy. The main problem with our current implementation is that it doesn't react without pressing enter, and doesn't abort on CTRL+C. Those two things (I think it requires the "raw" input mode?) would already be a major win!

@certik
Copy link
Collaborator

certik commented Nov 24, 2021

Yes, that's all I am imagining for the prompt_yn API.

@jonashaag
Copy link
Author

@wolfv so you actually prefer not having to press return? That'd be different from what Conda does, just FYI. I don't have any preference.

@wolfv
Copy link
Member

wolfv commented Nov 24, 2021

Let's do some research when we're implementing this. I don't recall how other command line programs do this -- for example apt or dnf etc. could be good to look at a bunch. I think y and n are also far enoguh apart on the keyboard that not requiring enter migth be fine :)

@MCWertGaming
Copy link
Collaborator

I started working on the prompt. Might get it done this week!

image

@MCWertGaming
Copy link
Collaborator

Am done with the first prototype now. Is this API desirable? It's not fully done yet, I'll add a "raw" prompt as well that does not require pressing enter to pass the key stroke and a "simplified" version that gives you a prompt with the minimal possible interface. @certik @jonashaag @wolfv
image

@MCWertGaming MCWertGaming linked a pull request Nov 30, 2021 that will close this issue
4 tasks
@MCWertGaming
Copy link
Collaborator

I have made a PR with some variations of y/n prompts in #144.

@certik
Copy link
Collaborator

certik commented Nov 30, 2021

Awesome, thanks @MCWertGaming for working on it!

@MCWertGaming MCWertGaming added this to To do in CPP-Terminal Dec 6, 2021
@MCWertGaming MCWertGaming moved this from To do to In progress in CPP-Terminal Dec 6, 2021
@MCWertGaming MCWertGaming added this to the V1.X.X milestone Dec 7, 2021
@MCWertGaming MCWertGaming self-assigned this Dec 7, 2021
CPP-Terminal automation moved this from In progress to Done Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants