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 support for Win32 #35

Closed
wants to merge 3 commits into from
Closed

Initial support for Win32 #35

wants to merge 3 commits into from

Conversation

nojb
Copy link
Contributor

@nojb nojb commented Apr 27, 2024

See the discussion in #34.

The list of things that do not work is long: resize events, mouse events, completion (tab) makes it go completely haywire, etc. But the bare minimum works. I did it before @jonahbeckford revealed his previous work, so things are done somewhat differently and/or in an overly naïve way (but note that with this simple patch arrows keys do work).

I don't think this patch is good enough to claim that Down works on Windows, but may serve as a stepping stone for further improvements.

I leave some comments inline below.

@@ -994,7 +994,7 @@ let down_readline p =
external sigwinch : unit -> int = "ocaml_down_sigwinch"
let install_sigwinch_interrupt () =
(* Sufficient to interrupt the event loop on window size changes. *)
Sys.set_signal (sigwinch ()) (Sys.Signal_handle (fun _ -> ()))
if Sys.unix then Sys.set_signal (sigwinch ()) (Sys.Signal_handle (fun _ -> ()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no signals on Windows. But there is a way to receive windows resize events which could be piped back up to the OCaml code; it would just need a bit of work.

let mkdir_posix dir = ["mkdir"; "-p"; dir]
let mkdir = if Sys.win32 then mkdir_win32 else mkdir_posix
let create dir = Result.map_error snd @@ cmd_run (mkdir dir)
let mkdir_p dir =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mkdir -p does not exist in cmd.exe. But Sys.mkdir exists in the standard library, so perhaps some version of the code below could be used instead.

@nojb
Copy link
Contributor Author

nojb commented Apr 27, 2024

down

@nojb
Copy link
Contributor Author

nojb commented Apr 27, 2024

BTW, you will notice there is a lot of flickering at the moment; I'm not sure what to do about it. I found https://stackoverflow.com/questions/73800269/flicker-free-console-updates-with-virtual-terminal-sequences after some random googling, but am not sure it is applicable.

@nojb
Copy link
Contributor Author

nojb commented Apr 28, 2024

I pushed 9bc5120 to fix the flickering (it is not perfect but it gets rid of most of it).

flicker

@nojb
Copy link
Contributor Author

nojb commented Apr 28, 2024

completion (tab) makes it go completely haywire,

It turns out that when pressing TAB, Down invokes where.exe through Sys.command, ie through cmd.exe. The invocation completely messes up the rendering of Down (see "Before the fix" below). Through trial and error, I found that resetting the console to "raw" mode after the invocation fixes the issue. This kind of makes sense because console state is global (shared by all programs running on the console) and it is probably the case that cmd.exe messes with the state of the console when invoked. For good measure, I restore the console to its original state before invoking any external program and reset it afterwards (see b8987b4)

Before the fix:

bug

After the fix:

fix

@nojb
Copy link
Contributor Author

nojb commented Apr 28, 2024

Completion and doc lookup (using ocp-index) is now working, if one uses the patched OCamlPro/ocp-index#170.

completion

I think this makes Down usable on Windows (there are still things to improve of course, but it is a start). @dbuenzli let me know if you would like me to do any changes, or feel free to pick the fixes and integrate them directly after massaging them into a shape that you are happy with :)

@dbuenzli
Copy link
Owner

Altogether that looks quite an unintrusive Windows support to me which makes me happy. If you want to stop here (please confirm) I'm happy to merge this and make a release. If you have in your head things that can be improved for other people to grab, please don't hesitate to open more specific issues.

@dbuenzli
Copy link
Owner

If you have in your head things that can be improved for other people to grab, please don't hesitate to open more specific issues.

More precisely it would be nice to have an issue that records exactly what doesn't work as expected to avoid too much bug reporting noise and so that I can refer to it to warn people appropriately somewhere from the docs.

@nojb
Copy link
Contributor Author

nojb commented Apr 29, 2024

Altogether that looks quite an unintrusive Windows support to me which makes me happy. If you want to stop here (please confirm) I'm happy to merge this and make a release.

Yes, I confirm. I think this is a good check point.

If you have in your head things that can be improved for other people to grab, please don't hesitate to open more specific issues.

More precisely it would be nice to have an issue that records exactly what doesn't work as expected to avoid too much bug reporting noise and so that I can refer to it to warn people appropriately somewhere from the docs.

I played with it a bit more today and I couldn't find any obvious problems. I will open an issue if I come across anything. Looking forward, if you agree, I propose the following: whenever a Windows issue is reported here, assign it to me or just ping me, and I will take care of both triaging it and investigating possible fixes.

@jonahbeckford
Copy link

Wow. Thanks @nojb !

@dbuenzli
Copy link
Owner

I propose the following: whenever a Windows issue is reported here, assign it to me or just ping me, and I will take care of both triaging it and investigating possible fixes.

That's fine with me. Could you perhaps just make an issue about the status of window resize on windows.

@dbuenzli
Copy link
Owner

Your patches are in, thanks.

@dbuenzli dbuenzli closed this Apr 30, 2024
@nojb nojb deleted the win32 branch April 30, 2024 08:14
@nojb
Copy link
Contributor Author

nojb commented Apr 30, 2024

I will take a closer look at resizing later and open an issue, thanks!

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