AlecAivazis / survey

A golang library for building interactive and accessible prompts with full support for windows and posix terminals.
MIT License
4.07k stars 351 forks source link

first pass on cursor tracking select focus #358

Closed vilmibm closed 2 years ago

vilmibm commented 3 years ago

Fixes #305

This solution is inelegant and brittle but it does work :sweat_smile: please let me know if I should add more test coverage.

AlecAivazis commented 3 years ago

Hey @vilmibm - thanks for submitting this! I'll try to find some time this weekend to pull it down and give it a shot.

It's been awhile since I've looked at the code for paginate but I think you should be able to use the second returned value to figure out how many lines to move up (assuming that the cursor is always starting at the bottom of a list pageSize long). If that doesn't work for some reason, then you would need to have some other way of identifying the correct line and your approach would make sense. Here are the tests for paginate so you can get a feeling for how it behaves.

EDIT: it might make sense to bake this directly into paginate so we get accessibility "for free" in the multi-line inputs 🤔

vilmibm commented 3 years ago

Using the paginate output is a great idea; I don't know why my eyes just glossed right past the output of paginate.

I'll add support for wide option lines and get this cleaned up.

EDIT: regarding adding to paginate; I believe that would require it having access to a Renderer, right? paginate is nicely independent of the prompts that make use of it right now.

AlecAivazis commented 3 years ago

Ah yea, good point. Maybe a reasonable solution is to add a method to Renderer that invokes and returns the values from paginate along with moving the cursor? That will keep the actual page logic pure and easily testable but still give us the behavior without having to implement it in multiple places. Thoughts?

vilmibm commented 3 years ago

Let me know what you think of the current approach; it's not the most elegant (the width() method for the prompts is a brittle thing) but I think it keeps things testable / comprehensible.

Unfortunately I broke multi-select rendering so I'll fix that tomorrow; if you are ok with this approach though in general I'll go ahead and add tests as needed.

vilmibm commented 3 years ago

I was waylaid by work stuff but finally marking this as non-draft. This solution is not the best but it does work; it will be a little annoying to support it in new prompt types, but it will weather icons of variable length when computing the width/word wrapping of long options.

AlecAivazis commented 2 years ago

Sorry i haven't gotten to this yet @vilmibm. work was really busy this week, will try to find some time to look at it over the weekend

vilmibm commented 2 years ago

@AlecAivazis buying a house seems so stressful -- no rush on my part and good luck!

I followed up on those two comments, but I think I have some test fixing to do 😅 I guess because I'm injecting more control sequences the virtual terminal tests need to be reconciled? I'll start work on that; let me know if you have any protips.

EDIT: actually it looks like the failing tests from a previous commit of my may have been flakes? They pass fine locally on Ubuntu and looking at them in Actions it feels like an actions problem (availability blip or something). I guess we'll find out once the tests are approved to run again.

AlecAivazis commented 2 years ago

Oh damn sorry I didn't realize they were still pending. GitHub didn't notify me about your edit so I totally missed that. I just kicked them off

vilmibm commented 2 years ago

hm, can you help me with some more context?

I'm running this locally:

go test -v github.com/AlecAivazis/survey/v2 github.com/AlecAivazis/survey/v2/core github.com/AlecAivazis/survey/v2/terminal -mod=vendor which is failing for me locally against both master and my PR for reasons seemingly unrelated to my changes. It seems like they are passing in CI, though?

How are you triggering the failure?

AlecAivazis commented 2 years ago

Ah yea sure. So there is this directory which contains scripts, not actual tests, that are meant to be run manually in order to verify that things behave in a real scenario. If you run the select and multi select tests everything behaves correctly but if I run the confirm test (just typing go run tests/confirm.go from the root of the repo) i see the issue

vilmibm commented 2 years ago

@AlecAivazis ah oops, I screwed up the tiny refactor and was restoring the cursor when I shouldn't have been by accident. Fixed now; running through the stuff in tests/ seems to work fine.

AlecAivazis commented 2 years ago

Awesome! Thanks for hunting that down! I finally found the time to pull down your branch and test it out. Sorry that took so long - was on vacation for 10 days.

I'm pretty sure this is good to go but I want to give it one more go around. Pretty excited we should be getting this in soon

vilmibm commented 2 years ago

awesome news! thank you!!

billygriffin commented 2 years ago

Great news @AlecAivazis, thanks for working with us to get it across the finish line. And great work @vilmibm! 🎉 ❤️