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

readline: Update fq fork #269

Merged
merged 1 commit into from
May 19, 2022
Merged

readline: Update fq fork #269

merged 1 commit into from
May 19, 2022

Conversation

wader
Copy link
Owner

@wader wader commented May 19, 2022

Rebase on master
Cherry-pick chzyer/readline#202

Rebase on master
Cherry-pick chzyer/readline#202
@wader wader merged commit cc33bb1 into master May 19, 2022
@wader wader deleted the readline-update branch May 19, 2022 15:06
@wader
Copy link
Owner Author

wader commented May 19, 2022

@thushan Would be great if you could do some Windows testing of this change. Make sure colors work and mess around in the REPL.

@tpodowd
Copy link

tpodowd commented May 20, 2022

Thanks for trying this out guys. My use case is not Windows but hopefully I didn't miss too much :-)

@thushan
Copy link
Contributor

thushan commented May 20, 2022

will get our QA folks to use this new build and test it out over the weekend and let you know Monday (AEST)

@wader
Copy link
Owner Author

wader commented May 20, 2022

@thushan Great!

@tpodowd When i was doing development on Windows i ended up with this dev setup https://github.com/wader/fq/blob/master/doc/dev.md#setup-docker-desktop-with-golang-windows-container which is very nice. If your on macOS you can have one shared source tree and then have three terminals using docker to build/run on macOS, linux and windows, made iterating a lot faster.
Sadly every time i've done it was initially quite a mess to find a windows version that matches a golang docker image build.

@wader
Copy link
Owner Author

wader commented May 20, 2022

If i remember correctly the terminal stuff on windows works quite well, but i guess there must be some terminal control codes translation going on? so in the end i guess it's best to try directly on a windows desktop.

@tpodowd
Copy link

tpodowd commented May 26, 2022

@wader @thushan I presume there is no trouble with this so far? If you guys are happy, maybe you can stick another comment on my mail pull request to get it back on the radar again.

@wader
Copy link
Owner Author

wader commented May 26, 2022

@tpodowd i've used the changes on macOS and some Linux for some days and haven't noticed anything bad, which is probably a good sign as before i think there were some weirdness happening at times.

@thushan
Copy link
Contributor

thushan commented May 26, 2022

Apologies I thought that one of the guys will have updated this thread - but he's got COVID it seems, so he'll be out for another week.

Good news is that they haven't noticed any changes from the build I built from master which has your changes @tpodowd. They tested it on Windows 10 and Windows 10 with Windows Terminal (internal development on their machines).

The feedback was:

  • existing workflow continues, so nothing broke - spaces etc remain.
  • seems like the redraw is faster in master than previous builds (TF: Is this something that was expected? noticed on the Windows 10 + Windows Terminal build, but unsure if it's due to the new build or update to WT)

@wader
Copy link
Owner Author

wader commented May 26, 2022

@thushan thanks, any idea what features they usually use/try? i'm mostly concern about interactive REPL usage, editing, line wrap, resize, completion etc and maybe colors.

@tpodowd
Copy link

tpodowd commented May 26, 2022

Hi @thushan - yeah, it should be faster. Previously, every time you typed a character, readline would clean up everything, then rewrite everything and then adjust the cursor using back spaces. New version, just appends characters and doesn't redraw unless it you are typing in the middle and uses 2 control sequences to jump to the correct cursor point. I don't expect anything to break regarding colour or spacing. One difference I noticed between windows code and linux code was that if you resize the window, the windows readline doesn't pick up the new screen size so its not going to work well. This is not a new issue though and was in the previous build. I just didn't fix it as my use case is linux.

@thushan
Copy link
Contributor

thushan commented May 28, 2022

@wader
They mostly use it to analyse video files and some image files. the build agents are picky about parsing the outputs of fq and it hasn't broken anything with this master release. the QA folks do use REPL to analyse things within the file during times we mess things up (which currently is quite regularly). Colours, wrapping, behaviours are the same. Unsure if they edit or use completion tho. Will email Monday.

@tpodowd
Ah brilliant, I did notice the resize issue (just now when I went to resize a Windows Terminal window running Powershell).

v0.0.7 (latest)
image

v0.0.7.x (master)
image

Same behaviour when windows resized thrice. Looks good to me.

@wader
Copy link
Owner Author

wader commented May 28, 2022

@thushan Thanks for checking. Sorry by edit i just ment line editing, backspace, move around text cursor and so on ... but if they use the REPL i guess so :)

@wader
Copy link
Owner Author

wader commented May 28, 2022

@thushan btw i gave a talk about fq some months ago, some history, implementation details and such but there is also a demo at the end how i use it to poke around and query things, also shows some cli and REPL tricks https://www.youtube.com/watch?v=GJOq_b0eb-s

@thushan
Copy link
Contributor

thushan commented May 28, 2022

also shows some cli and REPL tricks https://www.youtube.com/watch?v=GJOq_b0eb-s

Aha, I did pass that on to the team, i was one of the 7 that liked that video too :) Very detailed overview!

@wader
Copy link
Owner Author

wader commented May 28, 2022

Aha, I did pass that on to the team, i was one of the 7 that liked that video too :) Very detailed overview!

👍 😄 Thanks!

@tpodowd
Copy link

tpodowd commented May 31, 2022

Sounds like the patch is working well for you guys on Windows. That's great. I've been testing it quite a lot too on linux and seems to be solid right now for me. I'm going to ping the main pull request again so see if it can be merged. Perhaps you guys can also add a comment.

@tpodowd
Copy link

tpodowd commented Jun 29, 2022

Hi @wader - do you use tab completion in readline? I just added another PR which fixes some completion crashes and adds a Pager Mode for when there are too many completion candidates (similar to bash). Also added some more aggregate completion so that tab completes the common part of any candidates returned. Anyway - thought you might be interested. My other PR is not merged yet so the new PR includes the redraw patch also.

@wader
Copy link
Owner Author

wader commented Jun 29, 2022

Nice, yeap fq:s REPL uses readline completion quite extensively. Will take your PR for spinn later today or tomorrow

@tpodowd
Copy link

tpodowd commented Jun 29, 2022

ok great. Sorry to ping you on this old thread. If you have any feedback, hit me up on the PR.

@wader
Copy link
Owner Author

wader commented Jun 29, 2022

Will do, and no worries, email, twitter DM or whatnot work fine for me also if want discuss something

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.

3 participants