Find in repl #439

lsmor · 2022-06-19T17:33:36Z

This PR solves #85. It's still a little bit rough since it only shows the last entry in history with the given text.

The solution if found is making the prompt a sum type. This allow to handle reple events differently depending on which mode the repl is. I've found this more difficult to implement than expected. The main problems I've found are

UIState is a huge data type. Maybe putting al repl-related stuff within the same record could improve things a little bit
I think we can define a more complex Lenses, like promptUpdateL which can handle many parts of the logic. I think this is better than having a huge handleREPLEvent function.

All of this would imply a big refactor tough.

…ut don't implement search functionality

src/Swarm/TUI/Controller.hs

src/Swarm/TUI/Model.hs

byorgey

Great work, I'm excited to have this feature. A few changes I'd like to see:

Esc and/or ^G (ideally both) should cancel the search and return to the regular prompt. I could not figure out any way to cancel the search other than deleting what I had typed and then hitting Enter. Since then, I figured out that ^R actually cancels the search, but I would expect hitting ^R to find the next matching item in the history, not cancel.
Making the prompt less verbose, as explained in my other comment.
After finding something with a search and hitting Enter, the text of that history entry is placed in the REPL prompt, but it is not checked. In other words, if you select something from the history that has an error, it will appear in white letters, and will not turn red until you type something else or move the cursor. Ideally, the prompt entry would be checked immediately.

Making REPLPrompt a sum type seems like exactly the right way to handle this. The refactoring you talked about sounds great, and we should definitely be looking for ways to clean up and simplify the code, but I don't think we need to do that big refactoring before merging this.

src/Swarm/TUI/Model.hs

lsmor · 2022-06-20T15:51:15Z

Esc and/or ^G (ideally both) should cancel the search and return to the regular prompt. I could not figure out any way to cancel the search other than deleting what I had typed and then hitting Enter. Since then, I figured out that ^R actually cancels the search, but I would expect hitting ^R to find the next matching item in the history, not cancel.

Yes, I think all can be done easily except for the "next matching item" thing. I'd tried that before, but maybe I can workaround it... Probably some nasty code would be required...

Making the prompt less verbose, as explained in my other comment.

Agree

After finding something with a search and hitting Enter, the text of that history entry is placed in the REPL prompt, but it is not checked. In other words, if you select something from the history that has an error, it will appear in white letters, and will not turn red until you type something else or move the cursor. Ideally, the prompt entry would be checked immediately.

Oh! didn't spot that out. I'll change it

byorgey · 2022-06-20T19:25:58Z

Yes, I think all can be done easily except for the "next matching item" thing. I'd tried that before, but maybe I can workaround it... Probably some nasty code would be required...

Oh, to be clear, I was not saying you have to do that for this PR. I was just saying that's what I would expect ^R to do, if it does anything at all. But for this PR let's not worry about it.

byorgey · 2022-06-20T19:28:11Z

Also, I just went and made #450 which uses ^G to display a dialog with the scenario goal. So forget what I said about ^G, just Esc can cancel the search. 😄 Or else we could think of a different shortcut to display the goal.

lsmor · 2022-06-20T19:48:03Z

Oh, to be clear, I was not saying you have to do that for this PR. I was just saying that's what I would expect ^R to do, if it does anything at all. But for this PR let's not worry about it.

Well It turned out to be easier than expected

Also, I just went and made #450 which uses ^G to display a dialog with the scenario goal. So forget what I said about ^G, just Esc can cancel the search. smile Or else we could think of a different shortcut to display the goal.

I read that 10 minutes later hahaha also added ^G as a command. I'm removing it

lsmor · 2022-06-20T20:04:08Z

The new commits:

implements the "keep searching as you press ^R"
reduce verbosity of search prompt
Introduce Escape key to reset the prompt. Notice this works in Search prompt by canceling the search, and also in regular prompt by erasing it

src/Swarm/TUI/Model.hs

TristanCacqueray · 2022-06-21T12:10:12Z

src/Swarm/TUI/Model.hs

+lastEntry :: Text -> REPLHistory -> Maybe Text
+lastEntry t h = case filter (liftA2 (&&) matchesText isREPLEntry) $ toList $ h ^. replSeq of
+  [] -> Nothing
+  rh : _ -> Just $ replItemText rh


Not sure if the issue is here, but it seems like only the first command of a multi-command (separated by ;) line is returned. For example:

enter create "tree"; make "log"

search for create

The prompt contains [find create: "create "tree""] create. I would have expect the full command to be displayed, e.g. [find create: "create "tree"; make "log""]

instead of full line that was entered.

No, the problem is that it is backwards. 😄 @TristanCacqueray , I assume that you typed create "tree" some time previously in your history, before you typed create "tree"; make "log". The REPL history is stored as a Data.Sequence.Seq with the newest entry at the end. For example, see

swarm/src/Swarm/TUI/Model.hs

Lines 264 to 268 in 7c9e46d

addREPLItem :: REPLHistItem -> REPLHistory -> REPLHistory

addREPLItem t h =

h

& replSeq %~ (|> t)

& replIndex .~ 1 + replLength h

. My REPL history is extremely long so it is very obvious when I search for something: the first results that come up are so old that they are not even syntactically valid Swarm code any more. 😆

Ha yes, that is it :-)

Yes, I did interpret the repl history backwards 😵‍💫

TristanCacqueray · 2022-06-21T12:16:51Z

src/Swarm/TUI/Model.hs

+
+-- | Creates the repl form as a decorated form.
+mkReplForm :: REPLPrompt -> Form REPLPrompt AppEvent Name
+mkReplForm r = newForm [(replPromptAsWidget r <+>) @@= editTextField promptTextL REPLInput (Just 1)] r


Perhaps we should not repeat the search input, it is already displayed in the [find XX]. For example, in bash:

$ CTRL-R

(reverse-i-search)``': swarm

(reverse-i-search``swarm': ./dist-newstyle/build/...

Yes, the thing happening is that the form display the text, and also does the decorator. Should be only one of both

TristanCacqueray · 2022-06-21T12:17:58Z

src/Swarm/TUI/Controller.hs

@@ -590,25 +612,49 @@ handleREPLEvent s (Key V.KUp) =
  continue $ s & adjReplHistIndex Older
 handleREPLEvent s (Key V.KDown) =
  continue $ s & adjReplHistIndex Newer
+handleREPLEvent s (ControlKey 'r') = continue $


pressing CTRL-R should scroll back in the history, to access previous matching entry.

I think that's what it is doing. See the SearchPrompt case below.

As suggested, drop null appending Co-authored-by: Tristan de Cacqueray <[email protected]>

byorgey · 2022-06-21T12:39:09Z

src/Swarm/TUI/Controller.hs

+        case lastEntry t hist of
+          Nothing ->
+            (uiState . uiReplForm .~ mkReplForm (CmdPrompt ""))
+              . (uiState . uiReplType .~ Nothing)
+              . (uiState . uiError .~ Nothing)
+              $ s
+          Just found
+            | T.null t ->
+              (uiState . uiReplForm .~ mkReplForm (CmdPrompt ""))
+                . (uiState . uiReplType .~ Nothing)
+                . (uiState . uiError .~ Nothing)
+                $ s
+            | otherwise ->
+              validateREPLForm
+                . (uiState . uiReplForm .~ mkReplForm (CmdPrompt found))
+                . (uiState . uiReplType .~ Nothing)
+                . (uiState . uiError .~ Nothing)
+                $ s


. (uiState . uiReplType .~ Nothing) . (uiState . uiError .~ Nothing)

is repeated in every branch here, can we factor it out?

byorgey · 2022-06-21T12:40:14Z

src/Swarm/TUI/Controller.hs

@@ -590,25 +612,49 @@ handleREPLEvent s (Key V.KUp) =
  continue $ s & adjReplHistIndex Older
 handleREPLEvent s (Key V.KDown) =
  continue $ s & adjReplHistIndex Newer
+handleREPLEvent s (ControlKey 'r') = continue $


I think that's what it is doing. See the SearchPrompt case below.

byorgey · 2022-06-21T12:50:25Z

src/Swarm/TUI/Controller.hs

+       in s & uiState . uiReplForm .~ newform
+    SearchPrompt ftext rh -> case toList $ rh ^. replSeq of
+      [] -> s
+      _ : rhis ->


If I understand correctly, when pressing Ctrl-R repeatedly this simply removes one element from the history and reconstructs the SearchPrompt with the new history which is now one element shorter. The problem is that the removed element may or may not have matched the search text. So you have to press Ctrl-R repeatedly to get rid of all the non-matching history items (during which time it looks like nothing is happening) until finally reaching the one that matched; then pressing Ctrl-R again finally causes it to switch to the next matching history item.

Also, it is not very efficient to convert the Data.Seq to a list, pattern-match on it, then reconstruct a Seq from the tail of the list. Instead you should use something like Data.Sequence.viewl or viewr.

Actually, I think the problem is that if you type many times the same command, It will appear many times in the history and you al filtering out once at time.

Using Data.Seq directly is by far a better solution... you still have the problem of repeated elements in history I'll work that around

byorgey · 2022-06-21T12:54:41Z

src/Swarm/TUI/Controller.hs

+         in s & uiState . uiReplForm .~ newform
+handleREPLEvent s EscapeKey =
+  continue $
+    (uiState . uiReplForm .~ mkReplForm (CmdPrompt ""))


It makes sense for Esc to exit a search. However, I do not like the idea of Esc also clearing the normal prompt. Here is the scenario I am imagining: the player types a long and complex input, which does not type check. They want to find out why, so they hit Enter to see the pop-up error message. Once they are done reading the error message, they hit Esc to close it, intending to go back to edit their input, but oops, they accidentally hit Esc twice --- and now the input they worked so hard on is completely gone! We had talked about having Ctrl-C clear the input (see #65), but that is much harder to do accidentally.

Oh that's a good point!

byorgey · 2022-06-21T12:59:27Z

src/Swarm/TUI/Model.hs

+lastEntry :: Text -> REPLHistory -> Maybe Text
+lastEntry t h = case filter (liftA2 (&&) matchesText isREPLEntry) $ toList $ h ^. replSeq of
+  [] -> Nothing
+  rh : _ -> Just $ replItemText rh


No, the problem is that it is backwards. 😄 @TristanCacqueray , I assume that you typed create "tree" some time previously in your history, before you typed create "tree"; make "log". The REPL history is stored as a Data.Sequence.Seq with the newest entry at the end. For example, see

swarm/src/Swarm/TUI/Model.hs

Lines 264 to 268 in 7c9e46d

addREPLItem :: REPLHistItem -> REPLHistory -> REPLHistory

addREPLItem t h =

h

& replSeq %~ (|> t)

& replIndex .~ 1 + replLength h

. My REPL history is extremely long so it is very obvious when I search for something: the first results that come up are so old that they are not even syntactically valid Swarm code any more. 😆

src/Swarm/TUI/Model.hs

hlint suggestion Co-authored-by: Brent Yorgey <[email protected]>

… back and forth to list. Add new function for user friendly search

lsmor · 2022-06-21T18:04:22Z

New commits:

Change the search prompt so it is less verbose, and display the search word just one.
remove uses of toList therefore, only primitives from Data.Sequence are used.
Search in history from most recent to oldest.
Improves UX by not returning the same command twice on search.

byorgey

Thanks for the hard work getting this into shape! I am excited to use this feature. The code looks great now and everything seems to work as expected.

TristanCacqueray · 2022-06-22T11:40:38Z

Works great, thanks!

lsmor added 8 commits November 13, 2021 12:29

added REPLPrompt data type and modifed uiReplForm accordingly

e34f071

propagate the new replForm in the view and the controller

44b98a1

some old changes

2e0f618

fix conflicts

dd22731

comment changes to rebuild

ab3763b

change the type of repl. Adapt all function to work with that type, b…

3900cd0

…ut don't implement search functionality

build the change. Too verbose

b2f9055

Merge branch 'main' into find-in-repl

b7c3400

restyled-io bot mentioned this pull request Jun 19, 2022

Restyle Find in repl #440

Closed

format with formulu

661e734

TristanCacqueray reviewed Jun 19, 2022

View reviewed changes

src/Swarm/TUI/Controller.hs Outdated Show resolved Hide resolved

src/Swarm/TUI/Model.hs Outdated Show resolved Hide resolved

byorgey linked an issue Jun 19, 2022 that may be closed by this pull request

Add reverse-i-search to REPL #85

Closed

byorgey requested changes Jun 20, 2022

View reviewed changes

src/Swarm/TUI/Model.hs Outdated Show resolved Hide resolved

lsmor added 5 commits June 20, 2022 18:38

merge main. solve conflicts

0dc9c65

change find prompt to less verbose

46b24b5

remove debugging comments and factor out in reple handler

8c609f9

on Enter validate the found command

da09286

more

9619280

^r will find the next command, Esc or ^g will cancell the prompt

287b52d

restyled-io bot mentioned this pull request Jun 20, 2022

Restyle Find in repl #453

Closed

lsmor added 2 commits June 20, 2022 21:40

Merge remote-tracking branch 'origin/main' into find-in-repl

4713107

wrong code formatting

15f97c4

lsmor added 3 commits June 20, 2022 21:50

delete ^G keybinding. Also fix small bug on Escape

9a1e5ee

simplify Escape to complately reset the prompt.

92740f8

again format code

703f06c

TristanCacqueray requested changes Jun 21, 2022

View reviewed changes

Update src/Swarm/TUI/Model.hs

d649bd0

As suggested, drop null appending Co-authored-by: Tristan de Cacqueray <[email protected]>

byorgey requested changes Jun 21, 2022

View reviewed changes

byorgey reviewed Jun 21, 2022

View reviewed changes

src/Swarm/TUI/Model.hs Outdated Show resolved Hide resolved

lsmor and others added 6 commits June 21, 2022 16:43

Update src/Swarm/TUI/Model.hs

b7c1dc2

hlint suggestion Co-authored-by: Brent Yorgey <[email protected]>

Merge remote-tracking branch 'origin/main' into find-in-repl

93af591

change search prompt

0e8d650

clean lastEntry to usee Data.Sequence primitive instead of converting…

bc81208

… back and forth to list. Add new function for user friendly search

forgot to export new function

6a8d827

factor out some common logic

20830a5

byorgey approved these changes Jun 22, 2022

View reviewed changes

TristanCacqueray added the merge me Trigger the merge process of the Pull request. label Jun 22, 2022

TristanCacqueray approved these changes Jun 22, 2022

View reviewed changes

Merge branch 'main' into find-in-repl

765dc0b

mergify bot merged commit 23817a5 into main Jun 22, 2022

mergify bot deleted the find-in-repl branch June 22, 2022 11:48

lsmor mentioned this pull request Jun 22, 2022

Restore REPL on Ctr+C #65

Closed

	addREPLItem :: REPLHistItem -> REPLHistory -> REPLHistory
	addREPLItem t h =
	h
	& replSeq %~ (\|> t)
	& replIndex .~ 1 + replLength h

Find in repl #439

Find in repl #439

Uh oh!

Conversation

lsmor commented Jun 19, 2022

Uh oh!

Uh oh!

Uh oh!

byorgey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lsmor commented Jun 20, 2022

Uh oh!

byorgey commented Jun 20, 2022

Uh oh!

byorgey commented Jun 20, 2022

Uh oh!

lsmor commented Jun 20, 2022

Uh oh!

lsmor commented Jun 20, 2022

Uh oh!

Uh oh!

TristanCacqueray Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lsmor commented Jun 21, 2022

Uh oh!

byorgey left a comment

Choose a reason for hiding this comment

Uh oh!

TristanCacqueray commented Jun 22, 2022

Uh oh!

Uh oh!

TristanCacqueray Jun 21, 2022 •

edited

Loading