Highlight the background of the statement sent to REPL #1382
Conversation
This patch highlights the background of the statement that was sent to REPL. To set the color, ocaml.repl.evalTextBackgroundColor is newly added as a configuration. To remove the highlighted background, selecting an empty range and sending it to REPL works.
|
ping |
There was a problem hiding this comment.
To remove the highlighted background, selecting an empty range and sending it to REPL works.
This seems a bit unintuitive since doing Shift+Enter without a selection will select the entire line. That means removing the highlight specifically requires selecting whitespace or doing Shift+Enter on a blank line.
Keeping this functionality is fine, but I think there should also be an additional command to remove the highlighting.
|
Hi, thanks for your comments! I could not manage to find a time for addressing your comments today. I will reflect them tomorrow. |
| @@ -199,4 +251,22 @@ let register extension instance = | |||
| Extension_instance.close_repl instance) | |||
| () | |||
| in | |||
| Vscode.ExtensionContext.subscribe extension ~disposable; | |||
| let disposable = | |||
| Vscode.Workspace.onDidChangeTextDocument () ~listener:(fun event -> | |||
There was a problem hiding this comment.
I added this onDidChangeTextDocument that unsets the highlighted color if there is any update inside or before the evaluated region.
|
Hi, sorry for my delay. I reflected the comments so far. |
|
Could you please add a change log entry? |
There was a problem hiding this comment.
Frankly, I'm not sure it's useful to color the background for the current behavior of the REPL since the text sent is based either on the selection which is already colored or the current line if selection is empty. The sent text is also visible in the REPL. Do you have cases when such background would be useful?
do others have opinion on this?
src/repl.ml
Outdated
| let prevEvalTextDecorationType : TextEditorDecorationType.t option ref = | ||
| ref None |
There was a problem hiding this comment.
to my knowledge, we don't put mutable state in a module but put it in ExtensionContext, possibly within a record that's related to repl functionality.
There was a problem hiding this comment.
Thanks for your comments. I addressed this and two following comments in the newest commit.
src/repl.ml
Outdated
| let prevEvalTextDecorationType : TextEditorDecorationType.t option ref = | ||
| ref None | ||
|
|
||
| let prevEvalTextDecorationRange : Range.t option ref = ref None |
There was a problem hiding this comment.
camelCase is used only for vscode bindings (only symbols defined at src-bindings/)
src/repl.ml
Outdated
| let h = List.hd poss in | ||
| match h with | ||
| | None -> () | ||
| | Some h -> |
There was a problem hiding this comment.
this could be directly a pattern match on the list
match poss with
| [] -> ()
| h::_ -> ...
Yes, I felt this feature was necessary when I was evaluating the text in REPL which is taking a long time. Specifically, I am using vscode-ocaml-platform to write an OCaml program that uses HOL Light (https://github.com/jrh13/hol-light/). An Emacs plugin called ProofGeneral has a similar feature. It is an editor for a language called Coq, which is similar to OCaml but for more mathematical purpose. |
|
Since this mode isn't enabled by default, I feel there's not much risk in accepting this PR. It doesn't seem like it will add much maintenance overhead or bugs. EDIT: I misread, only the color is configurable. Can you make this feature off by default and enabled via an option? That way we can merge it and continue the discussion about UX. |
This patch highlights the background of the statement that was sent to REPL.
To set the color, ocaml.repl.evalTextBackgroundColor is newly added as a configuration.
Modifying any character in the editor before or in the selected region will remove the highlighted background.