Prompt options support #42

redbaty · 2018-01-18T11:54:00Z

Added checkbox support with the following features:

Can be treated as radio button.
Can be created from a class.

Missing features (yet):

Nesting
Select what properties to show from model using attributes.

* Moved options to its own file * Added object support * Added radio button support * Added generic static member to prompt

* Added help text * Added question text * Added is selection required

redbaty · 2018-01-18T11:56:38Z

@natemcmaster Could I use the Options attribute to also mark properties that should be displayed as options? Or should I create a separate one?

natemcmaster · 2018-01-19T01:14:32Z

A separate attribute would be better.

Btw, I'm headed out on vacation so I won't be able to take a closer look until next week.

redbaty · 2018-01-19T10:26:57Z

@natemcmaster Alright, have fun!

natemcmaster

@redbaty looks good so far. First, a trivial thing. My code style preference is to always use brackets for if, foreach, else etc. I called out a few specific places where I think they're vital.

I tried using this on macOS and the rendering doesn't appear correct. After pressing up/down a few times, this is what I see:

Final question, what do you think about this API surface? seems like most users would end up filtering Dictionary<string, bool> anyways.

// implies "radio". Only one response allowed
string GetOption(string prompt, string[] options)

// implies "checkbox". Zero or many responses allowed
string[] GetOptions(string prompt, string[] options)

natemcmaster · 2018-01-27T02:02:54Z

src/CommandLineUtils/Utilities/CheckboxManagerOptions.cs

+        /// <value>
+        /// The checked character.
+        /// </value>
+        public string CheckedChar { get; set; } = "🔘";


UTF8 output doesn't work well for all shells. Ascii may be a better default. I haven't tested on how well Console.OutputEncoding works, but my experience in the past is that UTF8 usually appears � instead of the actual character.

We could easily add an option to enable Unicode output, and in that case, use the prettier output characaters by default.

natemcmaster · 2018-01-27T02:05:40Z

src/CommandLineUtils/Utilities/Prompt.CheckboxManager.cs

+                if (!Equals(Console.OutputEncoding, Encoding.UTF8))
+                    Console.OutputEncoding = Encoding.UTF8;
+
+                if (boxes != null)


Would rather we throw an ArgumentNullException. I prefer we use uses Enumerable.Empty instead of null for empty collections.

natemcmaster · 2018-01-27T02:14:06Z

src/CommandLineUtils/Utilities/Prompt.CheckboxManager.cs

+            /// <param name="boxes">The boxes.</param>
+            public CheckboxManager(CheckboxManagerOptions options, IEnumerable<string> boxes)
+            {
+                if (!Equals(Console.OutputEncoding, Encoding.UTF8))


This check doesn't really seem necessary. Re-setting output encoding is fine.

natemcmaster · 2018-01-27T02:25:34Z

src/CommandLineUtils/Utilities/Prompt.cs

+        /// </summary>
+        /// <param name="possibleSelections">The possible selections.</param>
+        /// <returns></returns>
+        public static Dictionary<string, bool> GetOption(params string[] possibleSelections) =>


For consistency with the other Prompt APIs, can you make this GetOption(string prompt, params string[] options)?

natemcmaster · 2018-01-27T02:27:44Z

src/CommandLineUtils/Utilities/Prompt.cs

+        /// <param name="options">The options.</param>
+        /// <param name="possibleSelections">The possible selections.</param>
+        /// <returns></returns>
+        public static Dictionary<string, bool> GetOption(CheckboxManagerOptions options = null,


Let's make options a non-optional parameter. It helps disambiguate which overload is used.

natemcmaster · 2018-01-27T02:33:54Z

src/CommandLineUtils/Utilities/Prompt.CheckboxManager.cs

+                        if (End()) break;
+                    }
+
+                    switch (key.Key)


Any plans to handle modifiers like shift/alt/ctrl?

We could, whats your idea tho?

natemcmaster · 2018-01-27T02:34:21Z

src/CommandLineUtils/Utilities/Prompt.CheckboxManager.cs

+                        case ConsoleKey.UpArrow:
+                            GoUp();
+                            break;
+                        case ConsoleKey.Spacebar:


Could we also treat X as a selection key?

natemcmaster · 2018-01-27T02:35:52Z

src/CommandLineUtils/Utilities/Prompt.CheckboxManager.cs

+            private void BoxesOnCollectionChanged(object sender,
+                NotifyCollectionChangedEventArgs notifyCollectionChangedEventArgs)
+            {
+                foreach (var item in notifyCollectionChangedEventArgs.NewItems)


Code-style: nit. Can you use brackets in this method for the foreach and if?

natemcmaster · 2018-01-27T02:39:09Z

src/CommandLineUtils/Utilities/Prompt.CheckboxManager.cs

+            {
+                get
+                {
+                    _boxes.CollectionChanged += BoxesOnCollectionChanged;


Move this to the constructor so its only added once. In current form, each get on .Boxes adds a duplicate event subscription, for instance, on each call to Boxes.Add and each redraw.

natemcmaster · 2018-01-27T02:46:37Z

src/CommandLineUtils/Utilities/CheckboxManagerOptions.cs

+        /// <value>
+        ///   <c>true</c> if this instance is radio; otherwise, <c>false</c>.
+        /// </value>
+        public bool IsRadio { get; set; } = false;


Could we vary the default characters based on whether output is radio or not?

IsRadio Unicode Ascii

true 🔘 / 〇 (o) / ( )

false ☑ / ▢ [x] / [ ]

redbaty · 2018-01-27T16:18:55Z

Alright, just got home will make those review happen

* Made keys customizable * Made help text customizable * Added non-unicode support * Added overloads

redbaty · 2018-01-27T17:56:54Z

As for the mac bug I can't test it since I don't own one, I'll search arround tho, probably something with the way unix treats the console.clear thing?

natemcmaster · 2018-01-27T19:15:36Z

Do you have Bash for Windows? it might reproduce there.

Thanks for making the some of the changes so far.

natemcmaster · 2018-01-27T19:16:31Z

src/CommandLineUtils/Utilities/Options/OptionsKeys.cs

+        /// <value>
+        /// The select keys.
+        /// </value>
+        public ConsoleKey[] Select { get; set; } = { ConsoleKey.Spacebar, ConsoleKey.X };


natemcmaster · 2018-01-27T19:19:20Z

src/CommandLineUtils/Utilities/Prompt.OptionsManager.cs

+                while (true)
+                {
+                    var key = Console.ReadKey();
+                    if (Options.Keys.Finalize.Contains(key.Key))


I don't think we need this right now, but it might be nice to support using ctrl or alt to jump to the bottom or top of a list. For now, ignoring input when modifiers are used seems simpler.

redbaty · 2018-01-27T21:00:51Z

@natemcmaster I've tried it on my Deepin ISO, and couldn't reproduce the MacOS bug, can you try it again? Cause maybe it could be fixed after the redraw() thingy I forced some commits ago

natemcmaster · 2018-01-27T21:23:07Z

src/CommandLineUtils/Utilities/Prompt.OptionsManager.cs

+
+                while (true)
+                {
+                    var key = Console.ReadKey();


This should be Console.ReadKey(intercept: true). This will prevent other characters from being displayed when the key is pressed.

Totally forgot about that one 👍

natemcmaster · 2018-01-27T21:23:30Z

Latest commit looks a lot better now. Still seeing issues if I press other keys

natemcmaster · 2018-01-27T21:29:58Z

Weird. It almost working before setting intercept to true, but now it's broken again. Something must be weird in the way Console.SetCursorPosition works on macOS when intercept true is enabled.

redbaty · 2018-01-27T21:31:09Z

Hmm let me try to reproduce it

redbaty · 2018-01-27T21:38:23Z

Can you check if for some reason this line is being hit on mac?

https://github.com/redbaty/CommandLineUtils/blob/a5a0b0e0595e1872269aaa6cc2c3db5beb4dd8fa/src/CommandLineUtils/Utilities/Prompt.OptionsManager.cs#L373

or try to comment this

https://github.com/redbaty/CommandLineUtils/blob/a5a0b0e0595e1872269aaa6cc2c3db5beb4dd8fa/src/CommandLineUtils/Utilities/Prompt.OptionsManager.cs#L285

If any of those dont work it'll probably be something here:

https://github.com/redbaty/CommandLineUtils/blob/a5a0b0e0595e1872269aaa6cc2c3db5beb4dd8fa/src/CommandLineUtils/Utilities/Prompt.OptionsManager.cs#L356

Ps: I couldnt repro this on linux, so probably something mac specific

natemcmaster · 2018-01-27T22:10:20Z

So, I played a little with the code, and here is what I found. Console.SetCursorPosition isn't reliable.

Here's an experiment. It first draws two lines, then tries to move the cursor to the end of the first line. It works until the buffer in Terminal.app reaches the bottom.

    static void Main(string[] args)
    {
        var (x, y) = (Console.CursorLeft, Console.CursorTop);
        const string line = "x <-- start here. continue --";
        Console.WriteLine(line);

        Console.BackgroundColor  = ConsoleColor.Red;
        Console.WriteLine("       Bottom bar       ");
        Console.ResetColor();

        Console.SetCursorPosition(x + line.Length, y);
        Console.Write(">");
        Console.CursorVisible = true;
        Console.ReadLine();
    }

natemcmaster · 2018-01-27T22:13:55Z

I think what we need to make this work is that we need to think of the output area as a rectangle of pixels, each pixel is a character. SetCursorPosition is working as expected, but the value we should be passing to it aren't absolute x/y positions. The positions need to be relative to the top-left point of the box, which moves up when you hit the bottom of the screen buffer.

So, maybe a better approach is to first compute the width/height of output text needed, and use that to compute the cursor positions that should be used.

redbaty · 2018-01-28T22:43:08Z

Hmm this is really tricky, since I can't really redraw cause we dont have a fixed point. This only occurs on unix, linux included. The only solution I've thought of is to clear the console area, and show only the options then when that is done we re-write the things that were already drawn.

natemcmaster · 2018-02-01T01:36:21Z

I took a peek at what some other libraries do. It appears they use special ANSI escape codes for moving the cursor up/down/left/right. They use these moves to redraw the output. As a far as I can tell, this will be the only way to ensure this feature works cross-platform....which I'm going to require before I'd be okay shipping this feature.

I realized you've done quite a bit of work already. If you want to take care of this in a separate PR, let me know. I can merge this to a feature branch instead of master to give this work more time to mature before it gets released. Feature branches are still available in nightly builds on my myget feed.

redbaty · 2018-02-01T19:19:06Z

@natemcmaster I'll take a look at what other libraries do then, I had not thought about that 😜
After that we'll decide what to do with this PR

natemcmaster · 2018-02-16T07:15:41Z

Any progress? I'm inclined to take this PR as-is into a feature branch so we can work on it more. I want to keep the foundation you're laid out. I think it's a great start

redbaty · 2018-02-16T10:06:41Z

@natemcmaster Yeah sorry, haven't given any updates 😢

I haven't found a solution yet, the only one I think would work is to clear the console do the prompt and then rewrite everything that was inputted. Anyway if you think a feature branch would work for you then all right I'm in aswell

natemcmaster · 2018-02-20T05:13:12Z

Ok, let's but this into the feature/checkbox branch. We can use #41 to track finishing up this feature.

redbaty added 7 commits January 17, 2018 21:40

Added Prompt demo

e6630e2

Fixed build

3ebadc6

Added options, and changed default characters

942764b

Major changes

00bfb50

* Moved options to its own file * Added object support * Added radio button support * Added generic static member to prompt

Moved and improve example

7da8f34

Fixed arguments order mistake

0cee63a

Added more options

89d36b1

* Added help text * Added question text * Added is selection required

redbaty mentioned this pull request Jan 18, 2018

Prompt checkbox support #41

Closed

redbaty changed the title ~~Checkbox support~~ Prompt options support Jan 18, 2018

natemcmaster reviewed Jan 27, 2018

View reviewed changes

Lot of fixes and added more customization options

e3e2965

* Made keys customizable * Made help text customizable * Added non-unicode support * Added overloads

redbaty added 3 commits January 27, 2018 16:06

Force redrawing on each keypress to prevent some bugs

60a2e1e

Minor documentation added

1a725f2

Added UseOnOptionsAttribute to mark properties

6ef814f

natemcmaster reviewed Jan 27, 2018

View reviewed changes

Ignore if any modifiers are pressed and added useinoptions attribute doc

285ecb8

natemcmaster reviewed Jan 27, 2018

View reviewed changes

Intercept Console.ReadKey on Show()

a5a0b0e

natemcmaster changed the base branch from master to feature/checkbox February 20, 2018 05:12

natemcmaster merged commit d958cdd into natemcmaster:feature/checkbox Feb 20, 2018

This was referenced Jul 21, 2020

[Question] How to setup an options menu #366

Closed

Options Menu #385

Closed

Prompt options support #42

Prompt options support #42

Conversation

redbaty commented Jan 18, 2018 • edited

redbaty commented Jan 18, 2018 • edited

natemcmaster commented Jan 19, 2018

redbaty commented Jan 19, 2018

natemcmaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redbaty commented Jan 27, 2018

redbaty commented Jan 27, 2018

natemcmaster commented Jan 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redbaty commented Jan 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Jan 27, 2018

natemcmaster commented Jan 27, 2018

redbaty commented Jan 27, 2018

redbaty commented Jan 27, 2018 • edited

natemcmaster commented Jan 27, 2018 • edited

natemcmaster commented Jan 27, 2018

redbaty commented Jan 28, 2018

natemcmaster commented Feb 1, 2018

redbaty commented Feb 1, 2018

natemcmaster commented Feb 16, 2018

redbaty commented Feb 16, 2018 • edited

natemcmaster commented Feb 20, 2018

redbaty commented Jan 18, 2018 •

edited

redbaty commented Jan 18, 2018 •

edited

redbaty commented Jan 27, 2018 •

edited

redbaty commented Jan 27, 2018 •

edited

natemcmaster commented Jan 27, 2018 •

edited

redbaty commented Feb 16, 2018 •

edited