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

Initial version of height option for Windows #1341

Closed
wants to merge 2 commits into from

Conversation

kelleyma49
Copy link
Contributor

--height works under Windows versions that support vt100 (https://twitter.com/shanselman/status/695738082209853440).
Mouse support doesn't work.

@kelleyma49
Copy link
Contributor Author

Hmm, found an issue with the arrow keys. I need to look into why it's exiting if you bang on the up/down arrow keys.

@kelleyma49
Copy link
Contributor Author

kelleyma49 commented Jul 30, 2018

I found another issue with calling fzf --height from within a PsReadline key handler. So I still have some work to do.

@junegunn, at least look at how I separated out the light renderer to see if you have any issues with how I implemented the separation.

@junegunn
Copy link
Owner

junegunn commented Aug 1, 2018

Thanks for working on this. The code separation looks good to me.

/cc @janlazo

func IsLightRendererSupported() bool {
var oldState uint32
// enable vt100 emulation (https://docs.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences)
if err := windows.GetConsoleMode(windows.Stderr, &oldState); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is err set here for debugging only?

return false
}
canSetVt100 := windows.SetConsoleMode(windows.Stderr, windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING) == nil
windows.SetConsoleMode(windows.Stderr, oldState)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a temporary set/unset to test windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the goal is to check if the mode is allowed.

The other option is to explicitly check the version of WIndows is a minimum version. This is always tricky to get right across Windows 10 and server versions.

src/tui/light_windows.go Outdated Show resolved Hide resolved
src/tui/light_windows.go Outdated Show resolved Hide resolved
@janlazo janlazo mentioned this pull request Aug 3, 2018
16 tasks
@kelleyma49
Copy link
Contributor Author

This commit is ready to be reviewed. Please review and approve #1350 as that PR fixes that people might experience more frequently under Windows.

src/tui/light.go Outdated Show resolved Hide resolved
@janlazo
Copy link
Contributor

janlazo commented Dec 2, 2018

Can we use Travis to test this on Windows?

@kelleyma49 kelleyma49 force-pushed the WindowsHeightFix branch 2 times, most recently from 6d002c5 to bf79b69 Compare April 14, 2019 01:03
@kelleyma49
Copy link
Contributor Author

@janlazo: please take a look at the latest version when you get a chance.

@janlazo
Copy link
Contributor

janlazo commented Apr 14, 2019

@kelleyma49 Sure. Does this feature support Windows 10 only? I don't have my own Windows machine for Windows 7,8 so I can test it only in the VM.

@kelleyma49
Copy link
Contributor Author

kelleyma49 commented Apr 15, 2019

@kelleyma49 Sure. Does this feature support Windows 10 only? I don't have my own Windows machine for Windows 7,8 so I can test it only in the VM.

It only works in Windows 10, Anniversary Update and newer (https://devblogs.microsoft.com/commandline/new-experimental-console-features/). There's code in my commit to detect if vt100 sequences are supported.

kelleyma49 added a commit to kelleyma49/fzf that referenced this pull request May 20, 2019
A newer version of the sys module is
needed for pull request junegunn#1341
kelleyma49 added a commit to kelleyma49/fzf that referenced this pull request May 25, 2019
A newer version of the sys module is
needed for pull request junegunn#1341
junegunn pushed a commit that referenced this pull request May 26, 2019
A newer version of the sys module is needed for pull request #1341
- separate Unix & Windows code into platform specific files for light renderer
- height option works under WIndows platform
Fixes terminal codes from being interpreted
incorrectly
@wjrogers
Copy link

wjrogers commented Mar 5, 2020

I am testing this patch locally after merging with master @ d9c6a03. My primary use case is CTRL+R command history using PSFzf (see also kelleyma49/PSFzf#30, #1766).

  • PowerShell 7.0
  • Windows Terminal (Preview) 0.9.433.0
  • go 1.14
  • $Env:FZF_DEFAULT_OPTS="--ansi --layout=reverse --height=40%"

I had no problem building or running fzf. At first glance, the height option works correctly. I will keep using this binary and report back if I encounter anything unexpected.

--

I had some hope that these changes (and updating the sys module) would also fix #1766, but they do not. Any portion of the terminal buffer erased by fzf is not restored. I also observed some additional weird interactions w/ PSFzf, which I mention here because the PR author is also the author of PSFzf:

  • The prompt still gets erased after using CTRL+R, even when height is set
  • When there are fewer blank lines remaining in the terminal viewport than the value of height, the selected command appears at the bottom of the terminal after invoking CTRL+R

@kelleyma49
Copy link
Contributor Author

kelleyma49 commented Mar 8, 2020

Thanks for testing this! Hopefully this will expedite the merging of this request. :)

  • The prompt still gets erased after using CTRL+R, even when height is set
  • When there are fewer blank lines remaining in the terminal viewport than the value of height, the selected command appears at the bottom of the terminal after invoking CTRL+R

I just submitted a workaround for the missing prompt issue in PSFzf. You can download a Prerelease version here: https://www.powershellgallery.com/packages/PSFzf/1.1.36-alpha.

It works around the issue by telling PsReadLine to redraw the prompt. This could be an issue if you have a slow prompt function.

Note that if you call fzf.exe directly, the screen buffer is correctly displayed after you fzf.exe, so the issues you're seeing seem to be only when fzf is executed through PSReadLine.

junegunn pushed a commit that referenced this pull request Mar 9, 2020
Separate Unix & Windows code into platform specific files for light renderer
@junegunn
Copy link
Owner

junegunn commented Mar 9, 2020

I'm very sorry for keeping this open for so long. I finally had a chance to test this on Windows Server 2019 and it works really well. Rebased and merged to master. Thank you very much for your contribution! 🎉

I'll be releasing a new version pretty soon.

@kelleyma49
Copy link
Contributor Author

Rebased and merged to master.

Woo hoo! 🎉

I'm very sorry for keeping this open for so long

No need to apologize. Thank you for creating such an awesome tool! 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants